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
