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

[David Henningsson]

On 08/28/2013 07:15 PM, Felipe Tonello wrote:
> 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.

OTOH, if you're playing back through the speaker, and make a call, you
likely want the sound to switch, to go through the earpiece speaker
instead of the main speaker.

And where do you want your ringtone if you have headphones plugged in?
Perhaps through both the headphones and your speaker?

Etc. It's not that simple - which is why I want this functionality in
policy modules rather than hard coded.

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

But only if that port is currently unavailable (PA_AVAILABLE_NO), which
is the key point here.

> 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?

find_best_port will always find a port, otherwise the port has
disappeared between "if (!p) return NULL" and the call to find_best_port.

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

Do you mean "if (!p || p->available == PA_AVAILABLE_NO)" here?

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

But if we have a working module-switch-on-port-available, we don't need
any hint from the alsa module. Right?



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