Merge lp:~yuningdodo/unity-control-center/unity-control-center.lp1396464-prevent-update-sink-input-in-parallel into lp:unity-control-center

[Yu Ning]

Trigger the "value-changed" signal outside set_is_muted() when necessary.

This patch is for bug #1396464, with a further debugging I found it's a race condition issue.

in the function req_update_sink_info() we use pa_context_get_sink_info_*() to query the sink info, the info will be returned in the async callback _pa_context_get_sink_info_cb(). Now here is the problem, if req_update_sink_info() is called again before the previous callback is executed, then here is what will happen:

1. the mixer is changed to state 1, S1, in req_update_sink_info() an async query is performed.
2. the mixer is changed to state 2, S2, in req_update_sink_info() another async query is performed.
3. _pa_context_get_sink_info_cb() is called with S1, it update_sink() with S1
4. _pa_context_get_sink_info_cb() is called with S2, it update_sink() with S2

In step 3 the sink is updated with the out-of-date state S1, and this causes the mixer to be changed to S1 again, so another async query is performed in req_update_sink_info(). So we finally run into the endless loop 1->2->3->4->1->2->3->4... and the mixer is set between S1 and S2 repeatedly.

To fix this issue we should cancel any previous query in req_update_sink_info(), so update_sink() won't be called with an out-of-date state.

BTW, in the same source file there are functions with similar logic:

req_update_server_info()
req_update_client_info()
req_update_card()
req_update_sink_info()
req_update_source_info()
req_update_sink_input_info()
req_update_source_input_info()

Not sure if we should apply similar checking for them.

[David Henningsson (diwic)]

Cool, indeed this looks more reasonable to be the root cause.

But a question here - is it possible that the following can happen, at least in theory:
 1) req_update_sink_info(sink 1)
 2) req_update_sink_info(sink 2)
 3) update_sink(sink 1)
 4) update_sink(sink 2)

In this case, if it can happen, you would actually want them both to run in parallel.

Also, maybe upstream Gnome should be involved in this conversation. Not sure if they would like to break the loop using your suggestion, or rather break the loop at point 3) i e, if the change of mixer is initiated by a callback from PA, that should just update the GUI, not set the PA sink volume again to what it just received. Does that make sense?

[Yu Ning (yuningdodo)]

Sorry for the late reply, yes, that is possible to happen. Let me check how to solve it properly.

[lp:~yuningdodo/unity-control-center/unity-control-center.lp1396464-prevent-update-sink-input-in-parallel]

12803. By Yu Ning on 2015-01-28

Revert previous changes.

12804. By Yu Ning on 2015-01-28

Don't trigger the adjustment "value-changed" signal.

[Yu Ning (yuningdodo)]

I updated my patch. After more debugging I found there seem to be an unneeded trigger to the frontend update, this update will finally lead to the reported issue.

I reverted all my previous changes and then only disable this trigger, in my tests the issue is no longer reproduced.

[David Henningsson (diwic)]

Cool, indeed this is a tricky problem, and if you try to break the loop in the wrong place, something else will be broken. So the $1000 question - does this break anything else (except that the comment directly above is no longer valid)?

I'm not sure, but just to give you a hint of what to test:
 - Muting/unmuting from another application, e g from pavucontrol, is that still being picked up by sound settings?
 - Other mute/level combos such as the ones on input page and the one on the applications tab (in case something is currently recording or playing back).

Also, who calls this trigger? Maybe it is safer to temporarily disable the signal in the caller, e g using the g_signal_handlers_block_by_func and g_signal_handlers_unblock_by_func functions?

[Yu Ning (yuningdodo)]

Yes, I have made such kind of tests, open the u-c-c sound panel and move focus to another window, then I mute/unmute with a) the hotkey or b) the indicator menu or c) pavucontrol, the u-c-c sound panel can all get synced with the correct status.

And yes what I was trying to find out is just how to temporarily block the UI signals in the PA callbacks, however things can't be done easily: the PA callbacks are handled by GvcMixerControl, while the UI signals are handled in GvcChannelBar's internal logic, there is no interfaces to access each other in these two classes. So I finally give up and disabled that UI refresh trigger.

During my debugging I noticed that each time the user adjusts the sound setting via the sound panel it may causes several PA callbacks, and each PA callback may trigger a status sync to UI. So if this PA callback is not called with the final status, the UI change will trigger more PA callbacks and finally cause such kind of status ping-pong issue. Not only the u-c-c sound panel, I also met a similar issue in the bluetooth panel, bug #1248720, however for that bug it's possible for us to fix the issue by simply blocking the UI signals in BT callbacks.

[Yu Ning (yuningdodo)]

Oh, and for the question "who calls this trigger", it's called by two functions:

gvc_channel_bar_set_is_muted()
gvc_channel_bar_set_show_mute()

And who calls these two functions?

gvc_channel_bar_set_show_mute() is only called during UI constructions.

gvc_channel_bar_set_is_mute() is always called in a context that volume is also set before or after.

So this may explains why we shouldn't trigger this adjustment value-changed signal: when we trigger this event the adjustment is not set to the correct value yet, so if we trigger this signal then in the handlers we'll set the middle-stage values to PA and later when adjustment is actually set to the destination values PA is set again. It may also explains why my first patch could also workaround the issue by simply switch the order we call set_is_muted and set_volume().

[David Henningsson (diwic)]

If the thing is that set_is_muted is always called together with set_volume, and that in turn fires two signals, maybe one should add a gvc_channel_bar_set_volume_and_mute() function that would update both volume and mute and then fire signal(s)? Would that seem reasonable to you?

If not, I'm okay with merging your patch as it is, as you seem to have done your homework :-)

[lp:~yuningdodo/unity-control-center/unity-control-center.lp1396464-prevent-update-sink-input-in-parallel]

12805. By Yu Ning on 2015-01-30

Trigger the "value-changed" signal outside set_is_muted() when necessary.

[Yu Ning (yuningdodo)]

Thanks for the review and suggestions. To make things more clear let me list all the context where gvc_channel_bar_set_is_muted() is called, you can skip the boring description and go to the summary below.

1) gvc-channel-bar.c: on_scale_button_release_event(): an UI event handler, the adjustment "value-changed" signal is triggered separately, so no need to trigger it again in set_is_muted().

2) gvc-channel-bar.c: gvc_channel_bar_scroll(), which is only called by on_scale_scroll_event(): also an UI event handler, the adjustment "value-changed" signal is triggered separately. Only if the volume value need to be clamped to [min, max] it will adjust the volume again. Anyway, no need to trigger it again in set_is_muted().

3) gvc-channel-bar.c: on_zero_adjustment_value_changed(): volume is also adjusted here.

4) gvc-channel-bar.c: on_mute_check_toggled(): adjustment is switched between priv->adjustment and priv->zero_adjustment, so we should trigger the "value-changed" signal. However in my test the volume can still get updated even without a trigger here, by checking the code we know that a sync with PA is scheduled in GvcMixerStream "is-muted" property callback, so volume is also synced.

5) gvc-mixer-dialog.c: on_stream_is_muted_notify(): this will sync the is-muted status from a GvcMixerStream to the GvcChannelBar, the volume is adjusted in the GvcMixerStream and will also be synced to the GvcChannelBar, so we shouldn't trigger the adjustment "value-changed" signal here.

6) gvc-mixer-dialog.c: bar_set_stream(): volume is also adjusted here.

So as a summary:
* 1, 2, 5: set_volume() is not directly called in the same function, however since it will be called in other signal handlers before or after, no need to trigger the "value-changed" signal in set_is_muted().
* 3, 6: set_volume() is called directly in the same function, no need to trigger the "value-changed" signal in set_is_muted().
* 4: "value-changed" signal should be triggered logically speaking, although it's actually not necessary in current workflow.

For 3 & 6 we can provide a set_volume_and_mute() function, but anyway we should remove the explicit "value-changed" trigger in both set_is_muted() and set_volume_and_mute() since you know the signal will be auto triggered in gtk_adjustment_set_value(). I don't make this change to keep the patch short.

For 4 I have make tests both w/ and w/o the explicit "value-changed" trigger, both works, so I have updated my patch and added the trigger outside the set_is_muted() function to make the logic clear.

[Sebastien Bacher (seb128)]

David, Yu, what's the status of those changes?

[Yu Ning (yuningdodo)]

Sebastien, in my own test this patch works for myself, and my explanation to the patch can be found in previous comments. However I'm not an audio expert and don't have a deep enough understanding to u-c-c, I hope the patch can be reviewed by experienced expert.

[David Henningsson (diwic)]

I think it's okay to merge

review: Approve

=== modified file 'panels/sound/gvc-channel-bar.c'
--- panels/sound/gvc-channel-bar.c	2014-02-27 05:47:49 +0000
+++ panels/sound/gvc-channel-bar.c	2015-01-30 07:45:48 +0000
@@ -551,7 +551,6 @@
                  * and tell the front-end that the value changed */
                 gtk_range_set_adjustment (GTK_RANGE (bar->priv->scale),
                                           bar->priv->adjustment);
-                gtk_adjustment_value_changed (bar->priv->adjustment);
         }
 }
 
@@ -883,6 +882,7 @@
 
   is_muted = gtk_toggle_button_get_active (GTK_TOGGLE_BUTTON (object));
   gvc_channel_bar_set_is_muted (bar, is_muted);
+  gtk_adjustment_value_changed (bar->priv->adjustment);
 }
 
 static void