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.
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.
>
>>
>>>
>>> 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? :)
>
>> - 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?
Felipe Tonello
