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. >> + 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.) -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic
