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