Merge lp:~d.filoni/indicator-sound/pulse_fix_set_volume_role into lp:indicator-sound

Proposed by Devid Antonio Filoni on 2016-08-10
Status: Needs review
Proposed branch: lp:~d.filoni/indicator-sound/pulse_fix_set_volume_role
Merge into: lp:indicator-sound
Diff against target: 38 lines (+7/-1)
1 file modified
src/volume-control-pulse.vala (+7/-1)
To merge this branch: bzr merge lp:~d.filoni/indicator-sound/pulse_fix_set_volume_role
Reviewer Review Type Date Requested Status
Xavi Garcia 2016-08-15 Approve on 2016-11-02
Indicator Applet Developers 2016-08-10 Pending
Review via email: mp+302579@code.launchpad.net

Description of the change

ONLY FOR TOUCH

Trying to find a fix for bug #1544477 (this commit could fix it too), I found a behavior which is similar to what described in bug #1379618 . When you change too fast active role (for example typing a phone number in dialer-app when listening music in background) sometimes the volume of the old role is set to the new one. This is a concurrency issue with DBus async calls.

TEST CASE (mako):
0 - rm /home/phablet/.config/pulse/ubuntu-phablet-stream-volumes.tdb and restart (I don't know why looks like this step is required each time you reproduce the issue)
1 - Open dialer-app
2 - Type a digit (you should hear a sound)
3 - Set volume to low using buttons, do not set it to mute (you're changing alert role volume)
4 - Sound indicator icon should show two gray waves
5 - Open uRadio and play a station
6 - Set volume to a higher value using buttons (you're changing multimedia role volume)
7 - Sound indicator icon should show two white waves (maybe since step 5)
8 - Switch to dialer-app
9 - Type a digit (if you hear the digit sound then you reproduced the issue because alert role volume was the lowest)
10 - Switch to uRadio and stop and start the station (if notification icon doesn't change then you reproduced the issue, icon should change on play/stop action to show multimedia/alert role change, looks like volume levels are the same)
11 - Repeat step 9, 10 and 8 until issue id reproduced. You may have noticed when pressing the key in step 9 that uRadio volume is lower than before, it is the correct behaviour, music volume gets restored after few seconds, you could try type a digit in dialer-app (step 9) after volume is higher again, if volume is not lowered after typing, type another digit. You can also try skipping step 10 (and so 8).

I've uploaded the same patch on my mako PPA to test it and I'm not able to reproduce above issue anymore.

To post a comment you must log in.
Devid Antonio Filoni (d.filoni) wrote :

Please review this patch carefully, I'm not a vala developer and maybe there are better ways to fix the issue.

Jim Hodapp (jhodapp) wrote :

Thanks for the MR Devid. I'll ping the maintainer of indicator-sound to review this MR as soon as possible.

Xavi Garcia (xavi-garcia-mena) wrote :

Looks good to me

review: Approve

Unmerged revisions

540. By Devid Antonio Filoni on 2016-08-10

volume-control-pulse: handle a concurrency issue in role volume set. In particular circumstances indicator-sound changes the volume of a role using the previous set role one, this happens when it intercepts an external volume change signal (from example from AccountsService) while reading new role from pulse (which requires two DBus calls). Both Vala methods (set_volume_active_role and update_active_sink_input) are async.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/volume-control-pulse.vala'
2--- src/volume-control-pulse.vala 2016-03-04 15:25:39 +0000
3+++ src/volume-control-pulse.vala 2016-08-10 17:02:58 +0000
4@@ -40,6 +40,7 @@
5 private Gee.ArrayList<uint32> _sink_input_list = new Gee.ArrayList<uint32> ();
6 private HashMap<uint32, string> _sink_input_hash = new HashMap<uint32, string> ();
7 private bool _pulse_use_stream_restore = false;
8+ private bool _updating_active_sink_input = false;
9 private int32 _active_sink_input = -1;
10 private string[] _valid_roles = {"multimedia", "alert", "alarm", "phone"};
11 private string? _objp_role_multimedia = null;
12@@ -345,6 +346,7 @@
13 {
14 if ((index == -1) || (index != _active_sink_input && index in _sink_input_list)) {
15 string sink_input_objp = _objp_role_alert;
16+ _updating_active_sink_input = true;
17 if (index != -1)
18 sink_input_objp = _sink_input_hash.get (index);
19 _active_sink_input = index;
20@@ -384,6 +386,7 @@
21 } catch (GLib.Error e) {
22 warning ("unable to get volume for active role %s (%s)", sink_input_objp, e.message);
23 }
24+ _updating_active_sink_input = false;
25 }
26 }
27
28@@ -679,7 +682,10 @@
29 _volume.reason != VolumeControl.VolumeReasons.PULSE_CHANGE &&
30 volume_changed)
31 if (_pulse_use_stream_restore)
32- set_volume_active_role.begin ();
33+ {
34+ if (!_updating_active_sink_input)
35+ set_volume_active_role.begin ();
36+ }
37 else
38 context.get_server_info (server_info_cb_for_set_volume);
39

Subscribers

People subscribed via source and target branches