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

[Tanu Kaskinen]

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

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

> +            }
> +    }

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.

-- 
Tanu