Page MenuHomeFreeBSD

sound: Fix min/max sample rate assignment for VCHANs
AbandonedPublic

Authored by christos on Jun 20 2024, 5:39 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Feb 14, 5:28 PM
Unknown Object (File)
Thu, Feb 13, 5:25 AM
Unknown Object (File)
Fri, Jan 31, 9:21 PM
Unknown Object (File)
Dec 23 2024, 2:24 AM
Unknown Object (File)
Dec 18 2024, 12:22 AM
Unknown Object (File)
Dec 4 2024, 2:28 PM
Unknown Object (File)
Oct 2 2024, 4:32 AM
Unknown Object (File)
Sep 27 2024, 6:40 AM
Subscribers

Details

Summary

Currently the min/max sample rate reported for a given VCHAN is whatever
is specified in dev.pcm.X.[play|rec].vchanrate, and is the same for both
the min and the max. For example, if *.vchanrate is set to 48000, then
the min/max will be both 48000. Fix this by assigning the min/max to
either the physical channel's min/max, or, when hw.snd.feeder_rate_round
is set to 0, to feeder_rate_[min|max].

Sponsored by: The FreeBSD Foundation
MFC after: 2 days

Test Plan

Printing the oss_audioinfo->[min|max]_rate for a VCHAN when hw.snd.feeder_rate_round is not 0:

min_rate=44100
max_rate=96000

Which is the min/max of the physical channel.

And with hw.snd.feeder_rate_round=0:

min_rate=1
max_rate=2016000

Which takes the value from feeder_rate_[min|max]:

root@freebsd:~ # sysctl hw.snd.feeder_rate_min
hw.snd.feeder_rate_min: 1
root@freebsd:~ # sysctl hw.snd.feeder_rate_max
hw.snd.feeder_rate_max: 2016000

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 58386
Build 55274: arc lint + arc unit

Event Timeline

christos added inline comments.
sys/dev/sound/pcm/vchan.c
216

@dev_submerge.ch I am not really sure what values we must assign in the case of AFMT_PASSTHROUGH. Any ideas?

sys/dev/sound/pcm/vchan.c
216

The passthrough formats I know are all 48kHz on optical / SPDIF, but I think HDMI audio has more options now. This is supposedly going out bitperfect with no sample rate conversion, which suggests these should match the physical sample rates of the hardware.

sys/dev/sound/pcm/vchan.c
216

Then I can assign them to the parent channel's min/max as I do in the !feeder_rate_round case (will add appropriate AFMT_PASSTHROUGH check to make sure we never end up in the feeder_rate_round case for passthrough vchans).

Handle AFMT_PASSTHROUGH case: Set min/max to the parent's min/max.

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?

There's another thing to check here, with > 8 channels we set the bitperfect flag on the parent channel, and sample rate conversion is not possible to my knowledge. That case is similar but separate from AFMT_PASSTHROUGH, see the check in sys/dev/sound/pcm/feeder_chain.c:

/*
   * If channel is in bitperfect or passthrough mode, make it appear
   * that 'origin' and 'target' identical, skipping mostly chain
   * procedures.
   */
  if (CHN_BITPERFECT(c) || (c->format & AFMT_PASSTHROUGH)) {

AFAIK, current convention is for drivers to set the bitperfect flag when channel count exceeds 8 outputs / inputs. At least snd_uaudio and the snd_hdsp* drivers do so. There's also the possibility to set the bitperfect sysctl for pcm devices, but I'm not sure how that affects vchans.

sys/dev/sound/pcm/vchan.c
228–229

I get a panic here when running ossinfo, it says "mutex not owned", at sys/dev/sound/pcm/channel.c:2409. The vchan_getcaps() function is called from dsp_oss_audioinfo().

Sorry for the delay, had some serious outage due to virus infection (me, not my machines).

First of all, I hope you feel well now. :)

Just to make sure, this is for the SNDCTL_ENGINEINFO case, right?

I caught the bug with SNDCTL_ENGINEINFO, but any consumer that cares about those fields would also hit this.

There's another thing to check here, with > 8 channels we set the bitperfect flag on the parent channel, and sample rate conversion is not possible to my knowledge. That case is similar but separate from AFMT_PASSTHROUGH, see the check in sys/dev/sound/pcm/feeder_chain.c:

/*
   * If channel is in bitperfect or passthrough mode, make it appear
   * that 'origin' and 'target' identical, skipping mostly chain
   * procedures.
   */
  if (CHN_BITPERFECT(c) || (c->format & AFMT_PASSTHROUGH)) {

AFAIK, current convention is for drivers to set the bitperfect flag when channel count exceeds 8 outputs / inputs. At least snd_uaudio and the snd_hdsp* drivers do so. There's also the possibility to set the bitperfect sysctl for pcm devices, but I'm not sure how that affects vchans.

We could probably handle this case the same way as AFMT_PASSTHROUGH, since we care about what the hardware's min/max rates are.

sys/dev/sound/pcm/vchan.c
228–229

My bad. I forgot chn_getcaps() expects a locked channel.

Address Florian's comments.

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.

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.

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.

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?

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.

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.

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().

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.

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.

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.

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.

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.

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.

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.

Please double check that vchan_getcaps() gets used in chn_setparam() and feeder_chain(). If I read that correctly we have a semantic difference between the use cases, the capabilities returned here are treated as the possible end format / sample rate, not what the channel can receive as input.

Unless there's something wrong related to chn_setparam() or feeder_chain(), we shouldn't make any changes here IMHO.

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.

Please double check that vchan_getcaps() gets used in chn_setparam() and feeder_chain(). If I read that correctly we have a semantic difference between the use cases, the capabilities returned here are treated as the possible end format / sample rate, not what the channel can receive as input.

Unless there's something wrong related to chn_setparam() or feeder_chain(), we shouldn't make any changes here IMHO.

Since D45862 gives us the desired functionality, we can abandon this to avoid unexpected breakages.