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

[David Henningsson]

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.

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


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