On 03/24/2014 09:51 AM, Tanu Kaskinen wrote: > On Sat, 2014-03-22 at 18:39 +0100, David Henningsson wrote: >> On 03/21/2014 01:51 PM, Tanu Kaskinen wrote: >>> On Fri, 2014-03-21 at 10:27 +0100, David Henningsson wrote: >>>> In case a port has not yet been saved, which is e g often the case >>>> if a sink/source has only one port, reading volume/mute will be done >>>> without port, whereas writing volume/mute will be done with port. >>>> >>>> Work around this by setting a default port before the fixate hook, >>>> so module-device-restore can read volume/mute for the correct port. >>>> >>>> BugLink: https://bugs.launchpad.net/bugs/1289515 >>>> Signed-off-by: David Henningsson <david.henningsson at canonical.com> >>> >>> Thanks, this also fixes this bug: >>> https://bugs.freedesktop.org/show_bug.cgi?id=55262 >>> >>> A couple of comments below. >>> >>>> --- >>>> src/pulsecore/sink.c | 11 +++++++++++ >>>> src/pulsecore/source.c | 11 +++++++++++ >>>> 2 files changed, 22 insertions(+) >>>> >>>> diff --git a/src/pulsecore/sink.c b/src/pulsecore/sink.c >>>> index 08143e9..9c4b0c3 100644 >>>> --- a/src/pulsecore/sink.c >>>> +++ b/src/pulsecore/sink.c >>>> @@ -235,6 +235,17 @@ pa_sink* pa_sink_new( >>>> pa_device_init_icon(data->proplist, true); >>>> pa_device_init_intended_roles(data->proplist); >>>> >>>> + if (!data->active_port && !data->save_port) { >>> >>> How is data->save_port relevant here? If active_port is NULL, save_port >>> should always be false (true wouldn't have any meaning). >> >> It just means I haven't thought through the case where active_port is >> NULL and save_port is true, so better avoid it. >> >> I could have done: >> >> if (!data->active_port) { >> assert(!data->save_port); >> >> ...but as you know, I prefer when PulseAudio is not crashing. > > You can just drop the assertion, which will avoid any speculative > crashes. Ok, done in v2. > > You said that you hadn't thought through the case where active_port is > NULL and save_port is true, which I assume means that you're not sure if > it's a valid operation to set save_port to true while leaving > active_port to NULL. Are you still unsure? Does it help if I say that I > am 100% sure that it's not a valid operation? > >> >>>> + void *state; >>>> + pa_device_port *p, *p2 = NULL; >>>> + >>>> + PA_HASHMAP_FOREACH(p, data->ports, state) >>>> + if (!p2 || p->priority > p2->priority) { >>>> + p2 = p; >>>> + pa_sink_new_data_set_port(data, p2->name); >>> >>> I'd prefer calling pa_sink_new_data_set_port() only once, i.e. outside >>> the loop. >> >> Ok, makes sense. >> >>> >>>> + } >>>> + } >>> >>> This partially duplicates the code that is run later: >>> >>> if (!s->active_port) { >>> void *state; >>> pa_device_port *p; >>> >>> PA_HASHMAP_FOREACH(p, s->ports, state) { >>> if (p->available == PA_AVAILABLE_NO) >>> continue; >>> >>> if (!s->active_port || p->priority > s->active_port->priority) >>> s->active_port = p; >>> } >>> if (!s->active_port) { >>> PA_HASHMAP_FOREACH(p, s->ports, state) >>> if (!s->active_port || p->priority > s->active_port->priority) >>> s->active_port = p; >>> } >>> } >>> >>> I think we should do the fallback port initialization only once, in the >>> location where you added the new code. We should use the full logic of >>> the old fallback initialization code. >>> >> >> How about I take the code above, make a function out of it, and call it >> in both places? Maybe there's some module hook somewhere in between that >> can set active port to null. (And I don't like PulseAudio crashing even >> if a module hook does stupid things.) > > How about introducing hook PA_CORE_HOOK_SINK_SET_INITIAL_PORT? That > would then be the only valid place to apply initial port policy from > modules. Having a hook with clearer semantics would lower the > probability that modules do stupid things. > > If you don't like that idea, then go ahead and make a function that is > called from two places. Please add a warning to the second call that > makes it possible to catch broken modules. I did so but skipped the warning: it's still valid to have sinks without ports, so the warning would have been printed in cases where it shouldn't have been printed. > Apropos SINK_SET_INITIAL_PORT, I plan to gradually replace the various > NEW and FIXATE hooks with SET_INITIAL_FOO hooks. The NEW and FIXATE > hooks are mainly used for applying object initialization policies, and > it's not clearly defined what should be done in NEW and what should be > done in FIXATE, so I think the SET_INITIAL_FOO pattern is better for > code clarity. If this sounds like a bad plan to you, please let me know. I think SET_INITIAL_FOO is okay, but it's difficult to tell for sure without seeing the full picture. -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic
