[pulseaudio-discuss] [PATCH 4/8] sink, source: Fix default and alternate rate assertions

[David Henningsson]

On 08/12/2013 01:28 PM, Tanu Kaskinen wrote:
> On Mon, 2013-08-12 at 13:18 +0200, David Henningsson wrote:
>> On 08/12/2013 12:27 PM, Tanu Kaskinen wrote:
>>> On Mon, 2013-08-12 at 09:20 +0200, David Henningsson wrote:
>>>> On 08/09/2013 08:57 AM, Tanu Kaskinen wrote:
>>>>> ---
>>>>>  src/pulsecore/sink.c   | 4 ++--
>>>>>  src/pulsecore/source.c | 4 ++--
>>>>>  2 files changed, 4 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/src/pulsecore/sink.c b/src/pulsecore/sink.c
>>>>> index a4ad21a..ae6ae8f 100644
>>>>> --- a/src/pulsecore/sink.c
>>>>> +++ b/src/pulsecore/sink.c
>>>>> @@ -1412,8 +1412,8 @@ bool pa_sink_update_rate(pa_sink *s, uint32_t rate, bool passthrough) {
>>>>>          return false;
>>>>>  
>>>>>      if (!passthrough) {
>>>>> -        pa_assert(default_rate % 4000 || default_rate % 11025);
>>>>> -        pa_assert(alternate_rate % 4000 || alternate_rate % 11025);
>>>>> +        pa_assert((default_rate % 4000 == 0) || (default_rate % 11025 == 0));
>>>>> +        pa_assert((alternate_rate % 4000 == 0) || (alternate_rate % 11025 == 0));
>>>>
>>>> Looks correct, even though I don't understand why we need these
>>>> constraints in the first place. (Why can't we just allow any sample rate
>>>> as default or alternate?)
>>>
>>> I don't know. I would guess that this is a simple way to optimize
>>> multiple common cases (anything that is divisible by 4000 or 11025),
>>> while the general solution would be to optimize only two cases (the
>>> exact default and alternate rates).
>>>
>>
>> I remember a weird USB interface (in some launchpad bug) that only
>> accepted a non-standard rate, I think it was around 46 kHz or something.
>>
>> In case there is only one rate, I'm not sure that the rate can ever
>> update, but if it does (or in case a device would support two
>> non-standard rates), you have effectively enabled an assertion crash
>> that should perhaps not have been there in the first place. So I'm a bit
>> worried about the above.
> 
> daemon.conf doesn't allow rates that aren't multiples of 4000 or 11025,
> so there shouldn't be risk of crashing with the current code.

Ok.

> It should be possible to have the best of the both worlds, though: we
> could support arbitrary default and alternate sample rate, and if those
> happen to be multiples of 4000 and 11025, then we could enable the
> additional optimizations. I don't know if it's my job to implement that,
> though - this patch just fixes the assertions to match the (assumed)
> original intent.

I'm happy that you try to fix bugs in PulseAudio. I just want to make
sure that the fix isn't worse than the bug. :-)



-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic