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

[Tanu Kaskinen]

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

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

-- 
Tanu