[pulseaudio-discuss] [PATCH v4] Make module loopback honor requested latency

[Georg Chini]

On 25.02.2015 17:51, Tanu Kaskinen wrote:
> On Mon, 2015-02-23 at 18:56 +0100, Georg Chini wrote:
>> On 23.02.2015 08:02, Georg Chini wrote:
>>> On 22.02.2015 23:25, Alexander E. Patrakov wrote:
>>>
>>>> Anyway, the original submission (i.e. the patch that I am replying
>>>> to) has a bug: it crashes PulseAudio in the Bluetooth A2DP -> HDA
>>>> test if one changes the sink profile from Stereo Output to 2.1
>>>> Surround Output while the loopback is active. Before the patch, there
>>>> was no such crash.
>>>>
>>> I do not think that is a bug in the module. It looks like the sink has
>>> already changed when
>>> pa_sink_input_may_move_to_cb() is called and that should definitely
>>> not be the case.
>>> The original version does not show that problem because it never
>>> resets the rate when source
>>> or sink changes.
>>>
>>> Not too sure, but maybe this code in card_set_profile in
>>> module_alsa_card.c, line 223 already frees
>>> the sink before it should?
>>>
>>>              sink_inputs = pa_sink_move_all_start(am->sink, sink_inputs);
>>>              pa_alsa_sink_free(am->sink);
>>>              am->sink = NULL;
>>>
>>>
>> I have looked into this somewhat more, and it seems like the
>> may_move_to() function of the
>> sink input cannot hold what it promises. In sink-input.h the description is:
>>
>>       /* If non-NULL this function is called before this sink input is
>>        * move to a sink and if it returns false the move will not
>>        * be allowed */
>>       bool (*may_move_to) (pa_sink_input *i, pa_sink *s); /* may be NULL */
>>
>> When the profile changes, the old sink does no longer exist when
>> may_move_to() is called,
>> because it must be destroyed to create a new sink with the different
>> profile. So it is obviously
>> too late to cancel the move here (and module-loopback cannot reset the
>> rate).
>> Unfortunately in this situation there is also no way to call
>> may_move_to() before the old sink is
>> destroyed because the new sink is not known before it is created.
>>
>> So my question: Should there be a function may_move() that is called
>> before anything is done?
>>
>>       /* If non-NULL this function is called before this sink input is
>>        * moved anywhere and if it returns false the move will not
>>        * be allowed */
>>        bool (*may_move) (pa_sink_input *i); /* may be NULL */
>>
>> The call to this function could be added to pa_sink_input_may_move().
>>
>> The same probably applies for the source output.
> Why is sink_input_may_move_to_cb() in module-loopback.c calling
> pa_sink_input_set_rate()? That doesn't sound like a proper place to do
> such a thing.

When module-loopback is adjusting, it changes the rate of the sink
input. So if you switch source or sink it may be possible that the
rate of he sink input is far away from the expected rate. This should
be corrected before the sink or source is moved, otherwise you will
risk underruns.

> If there was the kind of function that you propose, would it return
> false in this scenario? If so, that would just make module-alsa-card.c
> kill the sink input, because it's not going to cancel the profile change
> just because some sink input refuses to be moved.
>
No, it would not return false in this situation, but I when you read
the description of the may_move_to() function it says:

1) this function is called before the sink input is moved away
2) if it returns false the move will not be allowed

Both is wrong. During profile switching the sink is already invalid
when the function is called and the profile will be switched anyway,
regardless what the function returns.

That's why I thought maybe there should be a function for which both
is true. But never mind - it was just a thought and I have worked around
the issue meanwhile. But maybe the description of the may_move_to()
function should be modified.

Regards
             Georg