[pulseaudio-discuss] [PATCH 4/7] card: move profile selection after pa_card_new()

[David Henningsson]

On 2015-10-27 07:27, Tanu Kaskinen wrote:
> On Mon, 2015-10-26 at 15:15 +0100, David Henningsson wrote:
>>
>> On 2015-10-23 12:56, Tanu Kaskinen wrote:
>>> I want module-alsa-card to set the availability of unavailable
>>> profiles before the initial card profile gets selected, so that the
>>> selection logic can use correct availability information.
>>> module-alsa-card initializes the jack state after calling
>>> pa_card_new(), however, and the profile selection happens in
>>> pa_card_new(). This patch solves that by introducing pa_card_put() and
>>> moving the profile selection code there.
>>
>> I think it's a good idea to split card_new into card_new and card_put.
>>
>>> An alternative solution would have been to move the jack
>>> initialization to happen before pa_card_new() and use pa_card_new_data
>>> instead of pa_card in the jack initialization code, but I disliked
>>> that idea (I want to get rid of the "new data" pattern eventually).
>>>
>>> The CARD_NEW hook is used when applying the initial profile policy, so
>>> that was moved to pa_card_put().
>>
>> So now CARD_NEW is called from CARD_PUT, and they are called almost
>> immediately after one another (the only difference is the card state).
>>
>> The only module using CARD_NEW and CARD_PUT hooks is
>> module-card-restore, so the only reason to move it is to accommodate the
>> needs of module-card-restore. And here I'm lost - I'm not understanding
>> why you need to move CARD_NEW.
>>
>> As an option, if we set the state to linked *after* the CARD_PUT hook,
>> would that help?
>
> The point of CARD_PUT is to fire after the card is fully initialized.
> It's used by module-dbus-protocol in addition to module-card-restore.
>
> I moved the "profile" modarg handling in alsa and bluetooth after the
> pa_card_put() call, which I now realize was a bad idea; the initial
> profile should be set before pa_card_put(), because the initial profile
> has to be finalized before CARD_PUT is fired.
>
> I don't really understand what you're proposing above,

I'm not sure I had a complete proposal, but I think CARD_NEW should be 
fired from pa_card_new, because firing CARD_NEW from pa_card_put would 
be confusing.

What I was also thinking was that maybe one could use the 
PA_HOOK_EARLY/LATE to do different stages of the process, so that 3.2 
below could be done on CARD_PUT + PA_HOOK_EARLY and what you call 4. 
below could be done on CARD_PUT + PA_HOOK_LATE. But maybe that would be 
confusing too.

> but I can
> explain why CARD_NEW had to be moved: CARD_NEW is part of the "select
> initial profile" process, and selecting the initial profile needs to
> happen after the profile availability is figured out. The profile
> availability is figured out after pa_card_new(), and therefore CARD_NEW
> had to be moved out of pa_card_new().
>
> Things should happen in this order:
> 1. pa_card is allocated
> 2. profile availability is figured out
> 3. the initial profile is selected
> 3.1. the core chooses the default profile
> 3.2. policy modules override the core (CARD_NEW)
> 3.3. backend overrides the policy modules based on user input (the
> "profile" modarg)
> 4. CARD_PUT is fired

So with the old order, it's natural to set the "profile" modarg between 
pa_card_new and pa_card_put. Where is it supposed to be set with the new 
order?

Also, should a "profile" modarg reset save_profile? Otherwise we might 
end up saving something that was not a user interaction?

>
>>> That required changing the hook data
>>> from pa_card_new_data to pa_card. module-card-restore now uses
>>> pa_card_set_profile() instead of pa_card_new_data_set_profile(). That
>>> required adding a state variable to pa_card, because
>>> pa_card_set_profile() needs to distinguish between setting the initial
>>> profile and setting the profile in other situations.
>>>
>>> The order in which the initial profile policy is applied is reversed
>>> in this patch. Previously the first one to set it won, now the last
>>> one to set it wins. I think this is better, because if you have N
>>> parties that want to set the profile, we avoid checking N times
>>> whether someone else has already set the profile.
>>
>> Is this just a matter of taste or would it be difficult to retain the
>> old order? The old order seems more consistent with e g setting ports on
>> sinks.
>
> This is primarily a matter of taste. I'd like to use this order also
> e.g. when setting ports on sinks. I haven't thought about how to
> implement this with the old order, but let's do that now.
>
> So, the order would look like this:
> 1. pa_card is allocated
> 2. profile availability is figured out
> 3. the initial profile is selected
> 3.1. backend sets the profile based on user input (the "profile"
> modarg)
> 3.2. policy modules set the profile if not already set (CARD_NEW)
> 3.3. the core sets the profile if not already set
> 4. CARD_PUT is fired
>
> Steps 3.2 and 3.3 need to check whether the profile has already been
> set. This checking is easily done, since NULL profile means that the
> profile hasn't been set yet. So there aren't any technical problems
> with implementing the old order.
>
> The only problem is that this isn't generalizable as a pattern. If the
> variable being set isn't a pointer, there may not be a value that can
> indicate that the variable hasn't been set yet. foo_new_data solves
> this by having fields like mute_is_set. I wouldn't want to add
> pa_sink.mute_is_set, if it's only needed during initialization.

Ok, I can somewhat buy this argument. But since you said that there was 
something wrong with where the current patch sets the "profile" modarg, 
I don't see a better way.


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