Could we add this field to the test in D45901?

Shouldn't we select the snd_dummy device for our mixer tests?
Current default could be set to any device, with not all features available that we want to test. Or am I missing something?

In D45973#1050056, @christos wrote:

In D45973#1049896, @dev_submerge.ch wrote:

For the deprecated argument interfaces of the mute (1, 0, ^) and recsrc
(+, -, =, ^) controls,

On a side note, how do people know that these are deprecated? The CLI interface of mixer(8) is used as an API by user scripts - there's been some resentment already due to the change from percentage to decimal values (difficult to parse and modify in shell script).

I think adding a man page notice would suffice for now.

Don't see anything suspicious, no regressions in my (brief) tests.

In D45982#1049978, @dev_submerge.ch wrote:

In D45982#1048497, @christos wrote:

@dev_submerge.ch You are the only one I know who owns a HDSP card, so could you give this a quick test?

I suppose it's ok, although the code change does not run in normal circumstances. I'll try to provoke that with a hack before I give you a go.

In D45982#1048497, @christos wrote:

@dev_submerge.ch You are the only one I know who owns a HDSP card, so could you give this a quick test?

For the deprecated argument interfaces of the mute (1, 0, ^) and recsrc
(+, -, =, ^) controls,

Did some brief tests, looks good to me.

I know it's WIP, but it would be nice to have a brief comment or description somewhere, to document what is and what is not implemented / working.

This file should probably also be added to share/examples/Makefile?

Fails to build - I think the path changes have to be reflected in share/examples/Makefile.

In D45951#1047858, @christos wrote:

In D45951#1047690, @dev_submerge.ch wrote:

If our library does the same thing as sndio, we might as well implement the sndio library API - more compatibility for free. The question is: Can our own library provide something for them that is not possible through the sndio API?

No, the library cannot provide something more than sndio, but I don't think that's the point of this library in the first place, at least for now. My initial goal behind it is to have an easier-to-use API for OSS, so applications that already (or in the rare case that someone wants to write a new one) use OSS, can work with something simpler. It doesn't have to do with providing something "better" compared to another API per se.

In D45951#1047559, @christos wrote:

In D45951#1047556, @dev_submerge.ch wrote:

Writing in smaller chunks doesn't work in conjunction with poll() or blocking write(). The application would have to apply a time-based wakeup approach, and thereby account for all the drift / loss introduced by the hardware. I do that in the sosso library, and it's quite difficult to get right. Another possibility is to set SNDCTL_DSP_LOW_WATER accordingly, but there are some caveats too.

I also advise against changing the buffer size, you can easily break playback on some sound cards, snd_hda(4) in particular. SNDCTL_DSP_SETFRAGMENT is conceptually broken (power of 2). One of the goals developing the sosso library was to not touch any buffer sizes, and work around that with the time-based approach.

As indirectly mentioned in my initial message, I am also against touching buffer sizes. Can you elaborate on the time-based approach? I have taken a look at sosso but I cannot say I fully understand the logic behind it.

In D45951#1047492, @christos wrote:

So, this is still a WIP patch, which means some things are intentionally left out or not properly implemented yet. Apart from the various XXX/TODO/FIXMEs throughout the source code, which are mostly minor things, there are a few things which I would really appreciate some discussion and ideas in order to continue.

1. Memory-mapped IO doesn't really work properly yet. I've been mostly testing playback, and stuff can be heard, but with lots of stuttering and noise in between. @dev_submerge.ch That being said, I am not decided whether providing mmaped IO is actually needed or worth the effort in the first place. I try to follow what the manual suggests, and it seems to be discouraging caring about mmap too much. http://manuals.opensound.com/developer/mmap.html

In D45901#1046543, @christos wrote:

In D45901#1046542, @dev_submerge.ch wrote:

In D45901#1046389, @christos wrote:

I am not really sure if it's meaningful to check each individual value returned by the nvlist_get_* calls. If any of these calls fails, the test will fail as well, so I think just calling them and making sure nothing fails is enough. Any opinions?

How exactly does it fail when a value is not available? Does it print a comprehensible error message? Otherwise we should test explicitly for the existence of the fields through nvlist_exists_*().

After some accidental failures I had when testing, I saw that if an nvlist call fails, the test will fail with it (because it core dumps, @markj is this acceptable in a test case?), and if you run kyua report --verbose -r ... you can see what was printed to stderr. Alternatively, if we want to be extra safe, we can actually call nvlist_exists() for every single field, I just found it a bit tedious.

In D45901#1046389, @christos wrote:

I am not really sure if it's meaningful to check each individual value returned by the nvlist_get_* calls. If any of these calls fails, the test will fail as well, so I think just calling them and making sure nothing fails is enough. Any opinions?

This fixes the panic for me, thank you.

Ok, it's not a regression - same panic on the main branch as of yesterday:

In D45872#1046199, @christos wrote:

Can you try testing this on main with (I know this is boring) all the open patches here applied? I have you as a reviewer in all of them so they should be easy to find and apply.

I get the following panic, but I may be missing some commit - any prerequisites for this?

Is there a test / application available to run this? I'd like to check the return values for my setup.

Looks good to me, in my tests with ossinfo this returns the expected sample rate values.

You may want to simplify the direction check and error handling on occasion and consolidate it with DSP_FIXUP_ERROR(), but that's not strictly necessary.

In D45664#1045639, @christos wrote:

In D45664#1045603, @dev_submerge.ch wrote:

In D45664#1045582, @christos wrote:

The issue is not that we can feed any rate/format, this is of course the point of VCHANs. The issue is that it seems wrong to me (maybe I am missing something?) to feed a rate of 100, and report a min/max range of 44100/96000 in SNDCTL_ENGINEINFO. Someone who doesn't know how these values get assigned would have a hard time understanding why the min/max range is 44100/96000 but yet we could feed a value outside the range. I think what we should report in SNDCTL_ENGINEINFO as the min/max range is feeder_rate_min and feeder_rate_max.

Agreed, but vchan_getcaps() seems to be the wrong place to fix this. As explained above, I think it is used in chn_setparam() and changing it may have severe consequences.

I think this patch still is needed, no? What we are currently reporting is definitely wrong.

In D45835#1045602, @christos wrote:

What about the case where we request SIMPLEX (i.e only READ or WRITE) but the device is already playing in the opposite direction? Shouldn't we return an error at least here?

In D45664#1045582, @christos wrote:

In D45664#1045548, @dev_submerge.ch wrote:

But isn't that the whole point of vchans? Allow to feed any format / sample rate and let it convert to the common format set in vchanrate and vchanformat, so it can be mixed with other vchans?

Frankly I'm a bit puzzled now, what are we trying to fix here? If I'm not mistaken vchan_getcaps() is used in chn_setparam() (through chn_getcaps()), which is prerequisite for a working feeder chain. And the capabilities we report here are treated as the end of the feeder chain we create. Thus it has to be the common mixing format when vchans are enabled, otherwise we cannot mix the vchans.

I think if we want to fix values reported in SNDCTL_ENGINEINFO or elsewhere, we have to fix it there, not in vchan_getcaps().

The issue is not that we can feed any rate/format, this is of course the point of VCHANs. The issue is that it seems wrong to me (maybe I am missing something?) to feed a rate of 100, and report a min/max range of 44100/96000 in SNDCTL_ENGINEINFO. Someone who doesn't know how these values get assigned would have a hard time understanding why the min/max range is 44100/96000 but yet we could feed a value outside the range. I think what we should report in SNDCTL_ENGINEINFO as the min/max range is feeder_rate_min and feeder_rate_max.

Also, in case the device is playback only or recording only, we don't return an error either for a duplex request. The channel allocation part in dsp_open() is explicitly designed to only return an error when there would be no channel at all.

In D45835#1045583, @christos wrote:

Hmm I think I get what you mean.. That in case the device is SIMPLEX but we request DUPLEX, instead of failing, we should create _one_ channel only, either in a direction that already exists, or choose a default (playback) in case no channels are playing already.

Further discussion about the simplex check in D45835.

In D45835#1045573, @christos wrote:

In D45835#1045553, @dev_submerge.ch wrote:

Overall approach is good, except we shouldn't fail for simplex when duplex is requested. Generic applications will not be prepared to handle this error, but will usually check the results of other operations (ioctl etc). Just search the channels for an already used direction in that case, or decide for one. And then make sure we don't try to allocate the opposite direction.

What do you mean by "fail for simplex when duplex is requested"? This check happens only if the device has SD_F_SIMPLEX.

Overall approach is good, except we shouldn't fail for simplex when duplex is requested. Generic applications will not be prepared to handle this error, but will usually check the results of other operations (ioctl etc). Just search the channels for an already used direction in that case, or decide for one. And then make sure we don't try to allocate the opposite direction.

In D45664#1045171, @christos wrote:

In D45664#1044988, @dev_submerge.ch wrote:

I think both the sysctl_dev_pcm_vchanrate and the checks in vchan_create are related to the primary channel mixer / multiplexer, not relevant for the application end of the vchans. De facto you can feed any sample rate supported by the conversion chain into the vchan, and that should be reflected in SNDCTL_ENGINEINFO, right?

If you set the rate through SNDCTL_DSP_SPEED, yes, you can feed any rate within the [feeder_rate_min, feeder_rate_max] range. However this is not reflected in SNDCTL_ENGINEINFO's min_rate and max_rate. Without the patch it reports whatever is set in *vchanrate, and with the patch it reports either the parent's or feeder_rate's min/max. This is wrong in both cases if we feed a rate outside the min/max.

In D45664#1044039, @christos wrote:

In D45664#1044037, @dev_submerge.ch wrote:

Could you give some explanation for the hw.snd.feeder_rate_round logic?
I don't think that value affects the vchans capability for sample rate conversions. When I test with ossinfo now, it always returns the hardware sample rates for all of SNDCTL_AUDIOINFO, SNDCTL_AUDIOINFO_EX and SNDCTL_ENGINEINFO, since feeder_rate_round > 0 by default. And I can play a 96kHz sound just fine on a 48kHz only hardware, so vchan sample rate conversion works with feeder_rate_round > 0.

feeder_rate_round seems to be used as a flag to cap the minimum and maximum VCHAN rates to whatever the hardware's ones are. See sysctl_dev_pcm_vchanrate and vchan_create. This is the pattern I followed here.

In D45775#1044931, @christos wrote:

In D45775#1044473, @dev_submerge.ch wrote:

See the inline comment. I found only one hardware driver that sets the SD_F_SIMPLEX, the snd_solo(8) which is quite "retro", and the driver is compiled in duplex mode by default. Other uses of SD_F_SIMPLEX do not require any tie-breaking I think, but please double-check.

This suggests that we can remove the tie-breaking part. About the release of the opposite direction channel, I'd expect pcm_chnalloc() or chn_reset() to fail anyway in the fixed direction simplex cases. But we may want to handle this more explicitly in dsp_open() for safety and robustness, e.g. not try to allocate a channel at all in the wrong direction.

Doesn't the second case in DSP_FIXUP_ERROR check this already? For example, if the device supports either only playback or recording, dsp_open() will fail with ENOTSUP if we try to open it in the opposite direction.

See the inline comment. I found only one hardware driver that sets the SD_F_SIMPLEX, the snd_solo(8) which is quite "retro", and the driver is compiled in duplex mode by default. Other uses of SD_F_SIMPLEX do not require any tie-breaking I think, but please double-check.

What are the requirements for the unit argument to make_dev()? Should it be unique? We're always producing the same unit number in seq_addunit() now.

Could you give some explanation for the hw.snd.feeder_rate_round logic?
I don't think that value affects the vchans capability for sample rate conversions. When I test with ossinfo now, it always returns the hardware sample rates for all of SNDCTL_AUDIOINFO, SNDCTL_AUDIOINFO_EX and SNDCTL_ENGINEINFO, since feeder_rate_round > 0 by default. And I can play a 96kHz sound just fine on a 48kHz only hardware, so vchan sample rate conversion works with feeder_rate_round > 0.

Sorry for the delay, had some serious outage due to virus infection (me, not my machines). Just to make sure, this is for the SNDCTL_ENGINEINFO case, right?

I couldn't build the test program, missing some symbols like FreeBSD_nvlist_exists? How is an application supposed to consume this?

In D45164#1033838, @christos wrote:

Tests with ossinfo(1) run fine.

In my tests this worked as expected with unavailable devices, both in mixer(8) and audio/oss ossinfo.

All in all the output shows up correctly now in ossinfo, except for the issues already mentioned here.

In D45256#1032314, @christos wrote:

@dev_submerge.ch I am not really sure if we can implement this in SNDCTL_ENGINEINFO as well. In SNDCTL_ENGINEINFO we choose an "engine"/channel on a device that is already opened, so in the case of an unavailable device, we won't be able to open it in the first place.

The patch is fine as is, but it does not fully address the issue described in the commit message, only D45256 does actually resolve it. You could either merge the two commits (and commit messages) when D45256 is ready, or adapt the commit messages to describe what part of the issue is addressed in each.

Uhm, sorry again, how do I take back the review accepted and change requested flags?

Sorry, this was meant for D45150, "Login to comment" took me to the wrong page.

The patch is fine as is, but it does not fully address the issue described in the commit message, only D45256 does actually resolve it. You could either merge the two commits (and commit messages) when D45256 is ready, or adapt the commit messages to describe what part of the issue is addressed in each.

The current check is never false and If nvlist_unpack(), we might panic later down the road.

In D45150#1030167, @christos wrote:

In D45150#1029983, @dev_submerge.ch wrote:

We're missing a piece of the puzzle, as mixer_oss_mixerinfo() still skips unavailable devices. Which means SNDCTL_MIXERINFO returns an error instead of a device with enabled == 0, thus breaking the ossinfo utility from audio/oss, and probably other software using the same approach.

That's right. So before I implement this I want to make sure we agree on what each oss_mixerinfo field should contain:

dev	loop counter
id	mixer<loop counter> (unavailable)
name	Perhaps something like pcm<loop_counter>:mixer (unavailable)
modify_counter	0
card_number	loop counter
port_number	0
enabled	0
caps	Not supported.
nrext	Not supported.
priority	Not supported.
devnode	Nothing.
legacy_device	loop counter

I am not really sure whether we want to include the mixer name in id and name or simply leave it blank, or unavailable.

In D45150#1029901, @christos wrote:

@dev_submerge.ch Can you test that this and D45151 fix the problem? I cannot reproduce it right now.

In D45144#1029847, @christos wrote:

In D45144#1029831, @dev_submerge.ch wrote:

This is ok as is, but it has the same problem as SNDCTL_MIXERINFO: We should consider returning "blank" oss_card_info structs for unavailable devices / device indices, instead of an error. Current implementation breaks audio/oss and probably kodi when devices are unavailable, since they stop iterating the SNDCTL_CARDINFO indices if there's an error. There is no enabled in oss_card_info to signal that a device is unavailable, but the info is mostly descriptive. We could return "unavailable" or something like that in the strings, which would hopefully be picked up by the application and shown to the user.

You are talking about the !PCM_REGISTERED(d) case right?

This is ok as is, but it has the same problem as SNDCTL_MIXERINFO: We should consider returning "blank" oss_card_info structs for unavailable devices / device indices, instead of an error. Current implementation breaks audio/oss and probably kodi when devices are unavailable, since they stop iterating the SNDCTL_CARDINFO indices if there's an error. There is no enabled in oss_card_info to signal that a device is unavailable, but the info is mostly descriptive. We could return "unavailable" or something like that in the strings, which would hopefully be picked up by the application and shown to the user.

In D45135#1029526, @christos wrote:

mixer_get_nmixers() returns the oss_sysinfo->nummixers, which, in 277615's case at least, is 5, but we have 8 sound cards, just 3 of them do not have a mixer. So now in mixer(8) we are looping from 0 to 5 and we start opening from /dev/mixer0 to /dev/mixer4, and this is why we're failing to open /dev/mixer0 as his error message reports.

I think a solution can be to get the number of sound cards (i.e oss_sysinfo->numcards) as opposed to the number of mixers, loop through them and if we fail to open a mixer, continue to the next one without printing any error/warning. I can add a mixer_get_ncards() routine to do this.