[pulseaudio-discuss] [PATCH] sink/source: Initialize port before fixate hook (fixes volume/mute not restored)

[David Henningsson]

On 03/24/2014 09:51 AM, Tanu Kaskinen wrote:
> On Sat, 2014-03-22 at 18:39 +0100, David Henningsson wrote:
>> On 03/21/2014 01:51 PM, Tanu Kaskinen wrote:
>>> On Fri, 2014-03-21 at 10:27 +0100, David Henningsson wrote:
>>>> In case a port has not yet been saved, which is e g often the case
>>>> if a sink/source has only one port, reading volume/mute will be done
>>>> without port, whereas writing volume/mute will be done with port.
>>>>
>>>> Work around this by setting a default port before the fixate hook,
>>>> so module-device-restore can read volume/mute for the correct port.
>>>>
>>>> BugLink: https://bugs.launchpad.net/bugs/1289515
>>>> Signed-off-by: David Henningsson <david.henningsson at canonical.com>
>>>
>>> Thanks, this also fixes this bug:
>>> https://bugs.freedesktop.org/show_bug.cgi?id=55262
>>>
>>> A couple of comments below.
>>>
>>>> ---
>>>>  src/pulsecore/sink.c   |   11 +++++++++++
>>>>  src/pulsecore/source.c |   11 +++++++++++
>>>>  2 files changed, 22 insertions(+)
>>>>
>>>> diff --git a/src/pulsecore/sink.c b/src/pulsecore/sink.c
>>>> index 08143e9..9c4b0c3 100644
>>>> --- a/src/pulsecore/sink.c
>>>> +++ b/src/pulsecore/sink.c
>>>> @@ -235,6 +235,17 @@ pa_sink* pa_sink_new(
>>>>      pa_device_init_icon(data->proplist, true);
>>>>      pa_device_init_intended_roles(data->proplist);
>>>>  
>>>> +    if (!data->active_port && !data->save_port) {
>>>
>>> How is data->save_port relevant here? If active_port is NULL, save_port
>>> should always be false (true wouldn't have any meaning).
>>
>> It just means I haven't thought through the case where active_port is
>> NULL and save_port is true, so better avoid it.
>>
>> I could have done:
>>
>> if (!data->active_port) {
>>    assert(!data->save_port);
>>
>> ...but as you know, I prefer when PulseAudio is not crashing.
> 
> You can just drop the assertion, which will avoid any speculative
> crashes.

Ok, done in v2.

> 
> You said that you hadn't thought through the case where active_port is
> NULL and save_port is true, which I assume means that you're not sure if
> it's a valid operation to set save_port to true while leaving
> active_port to NULL. Are you still unsure? Does it help if I say that I
> am 100% sure that it's not a valid operation?
> 
>>
>>>> +        void *state;
>>>> +        pa_device_port *p, *p2 = NULL;
>>>> +
>>>> +        PA_HASHMAP_FOREACH(p, data->ports, state)
>>>> +            if (!p2 || p->priority > p2->priority) {
>>>> +                p2 = p;
>>>> +                pa_sink_new_data_set_port(data, p2->name);
>>>
>>> I'd prefer calling pa_sink_new_data_set_port() only once, i.e. outside
>>> the loop.
>>
>> Ok, makes sense.
>>
>>>
>>>> +            }
>>>> +    }
>>>
>>> This partially duplicates the code that is run later:
>>>
>>>     if (!s->active_port) {
>>>         void *state;
>>>         pa_device_port *p;
>>>
>>>         PA_HASHMAP_FOREACH(p, s->ports, state) {
>>>             if (p->available == PA_AVAILABLE_NO)
>>>                 continue;
>>>
>>>             if (!s->active_port || p->priority > s->active_port->priority)
>>>                 s->active_port = p;
>>>         }
>>>         if (!s->active_port) {
>>>             PA_HASHMAP_FOREACH(p, s->ports, state)
>>>                 if (!s->active_port || p->priority > s->active_port->priority)
>>>                     s->active_port = p;
>>>         }
>>>     }
>>>
>>> I think we should do the fallback port initialization only once, in the
>>> location where you added the new code. We should use the full logic of
>>> the old fallback initialization code.
>>>
>>
>> How about I take the code above, make a function out of it, and call it
>> in both places? Maybe there's some module hook somewhere in between that
>> can set active port to null. (And I don't like PulseAudio crashing even
>> if a module hook does stupid things.)
> 
> How about introducing hook PA_CORE_HOOK_SINK_SET_INITIAL_PORT? That
> would then be the only valid place to apply initial port policy from
> modules. Having a hook with clearer semantics would lower the
> probability that modules do stupid things.
> 
> If you don't like that idea, then go ahead and make a function that is
> called from two places. Please add a warning to the second call that
> makes it possible to catch broken modules.

I did so but skipped the warning: it's still valid to have sinks without
ports, so the warning would have been printed in cases where it
shouldn't have been printed.

> Apropos SINK_SET_INITIAL_PORT, I plan to gradually replace the various
> NEW and FIXATE hooks with SET_INITIAL_FOO hooks. The NEW and FIXATE
> hooks are mainly used for applying object initialization policies, and
> it's not clearly defined what should be done in NEW and what should be
> done in FIXATE, so I think the SET_INITIAL_FOO pattern is better for
> code clarity. If this sounds like a bad plan to you, please let me know.

I think SET_INITIAL_FOO is okay, but it's difficult to tell for sure
without seeing the full picture.


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