[pulseaudio-discuss] [PATCH 5/8] card-restore: Save and restore "preferred profile" of port

[David Henningsson]

On 2015-10-21 17:26, Tanu Kaskinen wrote:
> On Tue, 2015-10-20 at 15:25 +0200, David Henningsson wrote:
>>
>> On 2015-10-20 06:20, Tanu Kaskinen wrote:
>>> On Tue, 2015-05-05 at 17:01 +0200, David Henningsson wrote:
>>>> diff --git a/src/modules/module-card-restore.c b/src/modules/module-card-restore.c
>>>> index 5c55cec..5d278c1 100644
>>>> --- a/src/modules/module-card-restore.c
>>>> +++ b/src/modules/module-card-restore.c
>>>> @@ -375,8 +385,44 @@ finish:
>>>>        return PA_HOOK_OK;
>>>>    }
>>>>
>>>> +static const char* profile_name_for_dir(pa_card_profile *cp, pa_direction_t dir) {
>>>> +    if (dir == PA_DIRECTION_OUTPUT && cp->output_name)
>>>> +        return cp->output_name;
>>>> +    if (dir == PA_DIRECTION_INPUT && cp->input_name)
>>>> +        return cp->input_name;
>>>> +    return cp->name;
>>>> +}
>>>> +
>>>> +static void update_profile_for_port(struct entry *entry, pa_card *card, pa_device_port *p) {
>>>> +    struct port_info *p_info;
>>>> +    const char *profilename;
>>>> +
>>>> +    if (p == NULL)
>>>> +        return;
>>>> +
>>>> +    if (!(p_info = pa_hashmap_get(entry->ports, p->name))) {
>>>> +        p_info = port_info_new(p);
>>>> +        pa_assert_se(pa_hashmap_put(entry->ports, p_info->name, p_info) >= 0);
>>>> +    }
>>>> +
>>>> +    profilename = profile_name_for_dir(card->active_profile, p->direction);
>>>> +    if (pa_safe_streq(p_info->profile, profilename))
>>>> +        return;
>>>> +
>>>> +    pa_xfree(p->preferred_profile);
>>>> +    p->preferred_profile = pa_xstrdup(profilename);
>>>
>>> I don't think updating the preferred sink or source belongs in module-
>>> card-restore. module-card-restore should only concern itself with
>>> updating the database, and restoring things when new cards appear.
>>> pa_card_set_profile() seems like a better place to me to set
>>> pa_device_port.preferred_device (my previous suggestion to name the
>>> field "preferred_sink" was obviously bad, since we need to cover
>>> sources too). I'd also use add a function to the pa_device_port API to
>>> set the preferred device name, to keep writes to the struct
>>> encapsulated within device-port.c.
>>
>> Trying to summarize the IRC discussion and add my own reflections:
>>
>> Moving the assignment to the preferred_profile variable is okay, I don't
>> mind either way. (That said, I have a slight preference for keeping it
>> as it is because it opens up for other modules using the field in other
>> ways.)
>> I assume that "restoring things when new cards appear" means that the
>> code in card_new_hook_callback remains as it is (modulo a wrapper
>> function in device-port.c)?
>
> Yes.
>
>> The rest: changing input_name / output_name to hashmaps of
>> sinks/sources, and/or changing preferred_profile to
>> preferred_sink/source (and have correct sink/source names) seems to grow
>> out of proportion, to the extent that we need to change namereg to
>> reserve all possible sink names at card creation time.
>> While the comment that a port prefers a sink/source rather than a
>> profile is technically correct, I don't see an easy split-up of all this
>> that can fix some of your comments and doing everything is probably too
>> much effort for little gain IMO.
>>
>> Would you like me to send out a new patch set with the assignment moved?
>> Would it be okay to let the rest of the patches in as they are?
>
> So you think it's ok to have pa_device_port.preferred_profile, even if
> the field doesn't actually refer to a profile? I'm definitely not ok
> with that.

The field refers to either:
  1) one or more profiles' input_name, or
  2) one or more profiles' output_name, or
  3) a profile's name

Hence, in its current form, it does refer to one or more profiles. Given 
that 
"preferred_profile_name_or_profile_input_name_or_profile_output_name" 
was too long, I shortened it to "preferred_profile". Do you have a 
better name suggestion for the field name?

> But if you think it's "too much effort for little gain" to
> make the code correct,it's probably the least hassle if we apply your
> patches, and I fix it up, since I have more motivation, assuming that
> you won't be blocking patches that add name reservation to the namereg
> etc.

Ok, that sounds like a compromise I can agree on.

(Even though I disagree with your wording - I don't think my code is 
incorrect just because it doesn't improve the situation for multi-sink 
profiles.)

> (By the way, name reservation is something that I have wanted on
> several occasions, because it has caused trouble that we can't rely on
> the name in the foo_new_data struct to be the final object name.)

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