[pulseaudio-discuss] [PATCH 2/8] source: Fix monitor source rate changing

[David Henningsson]

On 08/12/2013 02:19 PM, Tanu Kaskinen wrote:
> On Mon, 2013-08-12 at 13:51 +0200, David Henningsson wrote:
>> On 08/12/2013 01:34 PM, Tanu Kaskinen wrote:
>>> On Mon, 2013-08-12 at 13:28 +0200, David Henningsson wrote:
>>>> On 08/12/2013 12:18 PM, Tanu Kaskinen wrote:
>>>>> On Mon, 2013-08-12 at 09:15 +0200, David Henningsson wrote:
>>>>>> On 08/09/2013 08:57 AM, Tanu Kaskinen wrote:
>>>>>>> Monitor sources don't have the update_rate() callback set, so their rate was
>>>>>>> not being changed when changing the sink rate.
>>>>>>
>>>>>> For better understanding (I was a little confused first), one could add
>>>>>> a sentence to the commit comment saying e g "This patch fixes this by
>>>>>> changing the rate correctly, even if the update_rate callback is not set".
>>>>>
>>>>> Ok.
>>>>>
>>>>>> Can this also happen to sinks, that there might be types of sinks that
>>>>>> do not have update_rate set, and might fall into the same bug?
>>>>>
>>>>> I don't think there are such sinks. Monitor sources are special, because
>>>>> they are automatically created by the core. All other sources and sinks
>>>>> are supposed to set update_rate() if they support rate switching.
>>>>>
>>>>
>>>> So, with this patch, it looks like sources not supporting rate
>>>> switching, will not set update_rate initially, and then crash in
>>>> assert(monitor_of) ?
>>>
>>> There's
>>>
>>> -    if (!s->update_rate)
>>> +    if (!s->update_rate && !s->monitor_of)
>>>          return false;
>>>
>>> in the beginning of the patch.
>>>
>>
>> Oh, missed that. So, would it not be easier just to add an update_rate
>> callback to monitor sources (that have a sink with an update_rate
>> callback?), if that is our indicator that a sink/source supports
>> updating rates?
> 
> I don't know if it would be easier. This patch is not complex either. I
> did briefly consider adding a callback, but I thought that implementing
> callbacks in the core is sort of ugly, because it adds indirection where
> indirection isn't needed. That's not a strong opinion, though - I see
> how it would make pa_source_update_rate() a bit easier to follow.
> However, now that I try to think how to implement patch 8 in a callback,
> I don't think it's possible to do cleanly. The passthrough flag is the
> problem: it's not passed to the callback. For that reason I'd like to
> keep this patch as it is.
> 

Ok, maybe this would look a little more elegant than the dual
comparisons of s->update_rate:

if (s->update_rate) {
    if (!s->update_rate(s, desired_rate))
        return false;
} else
    s->sample_spec.rate = desired_rate;

(We don't need the monitor_of assertion, we checked that earlier in the
function.)


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