[pulseaudio-discuss] [PATCH] alsa-ucm: setting default active port based on jacks state

[Felipe Tonello]

Hi David,

On Wed, Aug 28, 2013 at 5:38 AM, David Henningsson
<david.henningsson at canonical.com> wrote:
> On 08/27/2013 07:06 PM, Felipe Tonello wrote:
>> Hi David,
>>
>> On Tue, Aug 27, 2013 at 12:27 AM, David Henningsson
>> <david.henningsson at canonical.com> wrote:
>>> On 08/26/2013 08:15 PM, Felipe F. Tonello wrote:
>>>> From: "Felipe F. Tonello" <eu at felipetonello.com>
>>>>
>>>> This fixes a bug when switching profiles under ALSA UCM. If a jack, hence a
>>>> port, was previsouly active, when PA performed a profile switch, the jack
>>>> would continue to be available but not active.
>>>>
>>>> This patche ensures that it activates a port if a jack is previously connected.
>>>
>>> Thanks,
>>> but I'm not sure this is the right way to do it.
>>>
>>> It feels like setting the active port is more like something for a
>>> policy module (e g module-switch-on-port-available), rather than being
>>> hardcoded in the module itself.
>>
>> With today's implementation it doesn't work, since if you switch
>> profiles the jack won't change its state, so
>> module-switch-on-port-available will not notice that.
>>
>> What it could be done is to ensure when you create a sink/source, to
>> report jack state, but this is a local function in alsa-module-card.
>> It would need more hack to make it work.
>>
>> Also, this is not hardcode the port, since the sink/source it self
>> does that, it checks if someone else set active_port before, otherwise
>> sets itself.
>>
>> IHMO this patch is less intrusive.
>
> Even if a jack is available, it does not mean that the port should be
> activated, unless you have a policy module saying so. It's the policy
> module that causes port switches in the first place.

Ok. Then IMO the policy module should say so :) Because as a user, it
doesn't make sense when it's listening to a music with a headphone and
then when it places a call the sound goes to the speaker.

>
> There are already hooks for new sink/sources in
> module-switch-on-port-available which fill in the port that should be
> initially selected.

Sorry about that. I'm currently using v3.0 and there was no such implementation.

>
> Now that I've read your patch again, it looks like you're only making an
> initial suggestion for active_port, which later should get overwritten
> by module-switch-on-port-available or module-device-restore, so I don't
> understand why your patch makes a difference at all?

It makes the difference because the sink will select as active_port
the one with higher priority. In most ALSA UCM cases this will be the
Speaker. So with this "hint" the module-switch-on-port-available and
others will check for sink/source->active_port before and select it if
it still active, if not, then will iterate trough the list of ports.

OBS: The current implementation will still not work. (to be cont...)

static pa_device_port *new_sink_source(pa_hashmap *ports, const char *name) {

    void *state;
    pa_device_port *i, *p = NULL;

    if (!ports)
        return NULL;
    if (name)
        p = pa_hashmap_get(ports, name);
    if (!p)
        PA_HASHMAP_FOREACH(i, ports, state)
            if (!p || i->priority > p->priority)
                p = i;
    if (!p)
        return NULL;
    if (p->available != PA_AVAILABLE_NO)
        return NULL;

    pa_assert_se(p = find_best_port(ports));
    return p;
}

(cont...)

Because this function, which is called by the new sink/source call
back, gets the port the correct, based on the active_port name, it
calls for find_best_port(ports) which overwrites the previous port.
This find_best_port() only checks for availability and priority. So
the "hint" here doesn't work at all.

Some comments about this function:
 - This assert at the end doesn't make sense. Because if it's possible
to return NULL if no port was active, why we will abort the program if
find_best_port doesn't find any port as well?
 - This PA_HASHMAP_FOREACH is totally redundant since find_best_port
does the same.

I think this function could be something like:

static pa_device_port *new_sink_source(pa_hashmap *ports, const char *name) {

    pa_device_port *i, *p = NULL;

    if (!ports)
        return NULL;
    if (name)
        p = pa_hashmap_get(ports, name);
    if (!p || p->available != PA_AVAILABLE_NO)
        p = find_best_port(ports);

    return p;
}

>
> (And pa_alsa_ucm_set_active_port should be named
> pa_alsa_ucm_get_preferred_port or something like that, because the name
> is currently misleading.)

I agree.

Let me know if you want anything else? Because I can send a patch set
having both fixes.

Felipe Tonello