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

[David Henningsson]

On 08/30/2013 06:54 PM, Felipe Tonello wrote:
> Hi David,
> 
> On Fri, Aug 30, 2013 at 6:00 AM, David Henningsson
> <david.henningsson at canonical.com> wrote:
>> 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.
> 
> Ok, it makes sense. But to be realistic, PA doesn't support Voice Call
> mode smoothly anyway. At least when using ALSA UCM.

We have voice call support in Ubuntu Touch, Arun is doing Voice Call
support in another way, and PulseAudio has been used in phone projects
in the past (Nokia N9). I'm not sure which implementation(s) that will
make it in, but I'm happy to upstream mine if there is interest from others.

However, module-switch-on-port-available was more or less written with
just desktop in mind, perhaps just because phones are more complex. And
it's expected to be superseded/obsolete once we have the node routing
system in place and working, but nobody knows how long that will take.

> Then module-switch-on-port-available, probably find_best_port(),
> should be more smart about choosing ports. It should use
> sink/source->active_port just as an hint, testing other stuff (like is
> a jack connected?) and if no other port was found, use active_port.

You seem to distinguish between "jack connected" status and "port
available" status? But the port availability status should always
reflect whether the jack is connected or not, so I don't understand why
you want to go after the "jack connected" status from the policy module.
Or am I misunderstanding you?

>>>> 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.
> 
> Well, why not return NULL as the other statements then? :)

So, the logic of this function is
 1) Figure out which port would be chosen if I don't do anything
 2) Is the port to be chosen unavailable?
 3) If so, choose another port.

That's all it does. And it works for desktop, because the speaker
actually becomes unavailable when headphones are plugged in (reflects
automuting done in the kernel). For phones, maybe this is not true, so
we might have to rethink the logic of this function, and do so in a way
that does not screw up for any desktop use case. Or possibly, replace
module-switch-on-port-available with something completely different.

> 
>>
>>>  - 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?
> 
> Yes :)
> 
>>
>>>
>>>     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?
> 
> Yes, I think. So at a first point I would like to add jack support in
> module-switch-on-port-available. I know that are probably much other
> use cases, but those can be added later, right? We just need to do
> something that can easily scale.
> 
> What I suggest, like I said above, is to use active_port as a hint
> only. It will check for other policies, such as jack is connected, and
> then make a decision. What do you think?

I think that the ALSA module should set the port's availability based on
jack connection status.

The policy module should set active_port based on port availability,
priority, and possibly other stuff (like e g, if the user has selected
speaker-phone mode or not).

There's no need for the ALSA module to make any active_port hint,
because the policy module has all the info necessary to make a decision.


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