Hi David,
On Fri, Aug 30, 2013 at 11:22 PM, David Henningsson
<david.henningsson at canonical.com> wrote:
> 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.
Are those devices using PA vanilla? Which version? If so how can I
make good usage of PA with Voice Call mode? I'm using UCM today to
make it easier. So I have a profile with QT VoiceCall (which it seems
that PA doesn't care) that opens a dumb pcm device that does the
routing in the codec level. Then I have to write dumb data to this
sink so it doesn't suspend. I know that's not the appropriate
solutaion and I believe is causing problems with the
module-stream-restore.
(...cut...)
>
> 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?
Yes, I understand that. It's true but don't you agree with me that if
you have a Headphone and Speaker port, if the jack is connected, both
ports will be available, right? If so, we should give priority to the
headphone. Thus we need to check if the port has a jack associated
with.
>>>>>
>>>>> 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.
Ok. Thanks for sharing that. So I would say that we can try to modify
module-switch-on-port-available to give priority to ports which as
associated with jacks. This will make sure it works on both platforms.
(...cut...)
>>>>
>>>> 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.
Which already does, right?
>
> 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).
Let's do a priority list here so we can implement that properly. I
would say the following:
1: port availability
2: is port associated with a jack?
3: priority
That's for the use cases we have for today.
What do you mean by speaker-phone mode?
Regards,
Felipe Tonello
