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
