Merge lp:~rsalveti/indicator-sound/adding-support-for-sound-per-roles into lp:indicator-sound/14.10

Proposed by Ricardo Salveti
Status: Merged
Approved by: Ted Gould
Approved revision: 453
Merged at revision: 447
Proposed branch: lp:~rsalveti/indicator-sound/adding-support-for-sound-per-roles
Merge into: lp:indicator-sound/14.10
Diff against target: 400 lines (+312/-5)
2 files modified
src/CMakeLists.txt (+1/-0)
src/volume-control.vala (+311/-5)
To merge this branch: bzr merge lp:~rsalveti/indicator-sound/adding-support-for-sound-per-roles
Reviewer Review Type Date Requested Status
Ted Gould (community) Approve
PS Jenkins bot (community) continuous-integration Needs Fixing
Review via email: mp+235562@code.launchpad.net

Commit message

Adding support to change volumes per active audio role.

Description of the change

Adding support to change volumes per active audio role.

Some comments regarding the changeset:
* Ideally this would also be the default code path for the Desktop, but for that to happen we first need to revisit every media role used, and force one by default on pulse itself. As this requires some intrusive changes, and we're past over feature freeze for Utopic, this needs to be discussed again once the next cycle starts, as this would also be the desired behaviour when looking from the convergence point of view.
* The additional pulseaudio dbus module is not loaded by default on the desktop, but is on touch (touch got a different pulseaudio config). So in order to test this you need to make sure to load the dbus module (check https://code.launchpad.net/~rsalveti/ubuntu-touch-session/changing-config-allow-volume-per-role/+merge/235563).
* To avoid clashes with the desktop mode (that still wants to change the master volume), I decided to create a flag that basically controls which behaviour to use. If it finds the dbus module, and if it finds all the required roles in the stream/restore database, then the default behaviour is changed for the indicator to react and control volume per audio roles. If the dbus module is not available, or the user got one missing role, it gets back to work controlling only master.
* When testing with Ubuntu touch, make sure you're also using silo 18 for the other dependencies (https://launchpad.net/~ci-train-ppa-service/+archive/ubuntu/landing-018).
* We still need to see what should be exported for accountservice, as atm the volume from the current active role will propagate to accountservice. This is not yet an issue for Touch, but once we get back to have a separated greeter, we need to revisit this.
* I had to manually call the dbus methods because I can't easily abstract them with Vala, because not everything required is implemented from the other side. This pulse dbus module would need to be improved, but something to work with upstream later on.
* I can see the design also requiring us to notify the user about which volume role is active and changed when the user press volume up/down. Not yet sure how we can abstract that, but we can sync and see the details later on.
* For this MR to build it also needs latest pulseaudio that is available in the same landing silo (this is required because the vapi files from pulse were not exporting everything needed). Didn't yet bump debian/control pulseaudio build-dep because I first want to test everything to avoid extra version bump in pulse.

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
449. By Ricardo Salveti

Adding TODO entry

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
450. By Ricardo Salveti

volume-role: making sure we reflect changes done by pulse (via signals)

451. By Ricardo Salveti

volume-role: removing TODO

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Ted Gould (ted) wrote :

I think that adding a dep on gee-1.0 will need to be added to the packaging as well. Some clean up and other changes included inline.

review: Needs Fixing
Revision history for this message
Ricardo Salveti (rsalveti) wrote :

libgee-dev is already part of debian/control (as build-deps), so that should be fine.

Revision history for this message
Ricardo Salveti (rsalveti) wrote :

Reply in-line, will push the changes in a few.

452. By Ricardo Salveti

Review changes

Revision history for this message
Ricardo Salveti (rsalveti) wrote :

I decided to keep set_volume_active_role as sync, to avoid a huge number of callbacks and signals when the user is scrolling the volume indicator bar.

I believe the ideal fix here is changing the indicator (or probably unity8 in this case), to create steps that could be used when setting up the volume (to avoid a huge number of events, and we also don't care if the user wants to change the sound 0.10% higher or lower, we should probably instead just support 5% or 10% volume changes as steps). Want me to open a bug against Unity8 for that?

Added the following comment to the function:
/* This call can be async once we are sure we are not allowing multiple set volume calls
 * in sequence (with a minimal volume delta, not with steps), as this can cause issues
 * when handling the volume updated signal from PulseAudio (_volume can get set before
 * the signal from a previous set gets handled at pulse_dbus_filter, generating an extra
 * volume_updated signal) */
 private void set_volume_active_role ()

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Ted Gould (ted) wrote :

What's happening isn't that we're getting a value for every little change, we're getting periodic updates, but QML blocks on the call. So we end up creating a "jumpy" slider if we take too long to respond to it. This was happening with the AS support earlier. So it needs to be async not because we're getting every little change but more because we need to respond to the message as fast as possible to avoid the UI from blocking. (which it shouldn't, but apparently that's a difficult problem to tackle on the QML side)

Revision history for this message
Ricardo Salveti (rsalveti) wrote :

As there's no way to know from pulse who triggered the volume changed signal, making it async makes it harder to track down if the signal should or shouldn't be handled by our code (in order to avoid an extra round of signals).

The UI feedback with async is indeed a bit better, but didn't have any issue when using it with sync (still fine from a user perspective).

We can try suggesting a different way to track the volume changes, but as we talked over IRC, that would be ugly and probably not so efficient.

I still think the right way is to make unity8 to only give us step changes, and not minimal ones (so we can also reduce the amount of cpu/messages/signals used).

Revision history for this message
Ted Gould (ted) wrote :

So, the problem is that we keep discussing this as tracking the changes and whether or not they're coming from indicator-sound or from somewhere else. That makes the problem hard as we don't know that information. But instead I think we should look at it as trying to determine which values we care about and which data we can discard. If we send a value to PA we don't care about any values that it sends back until we get the echo about our setting back because we know that they're all old values at that point.

For the solution we can make a FIFO of values that we send to PA, and then when we get signaled about values we pop an item off of the FIFO when it matches. If the FIFO is not empty, we ignore the value coming back from PA. If the FIFO is empty we assume someone else changed it and we care about the value.

This handles the case of someone else setting the value and us not, as we'll not have any entries in the FIFO and respond immediately. If we're competing with someone we know that there'll be a bunch of races, so we remain stable until the last value is sent, whether it be ours (and that just empties the FIFO) or there's that comes when the FIFO is empty. For most cases it'll just result in an entry being put into the FIFO and then being removed on the echo so it's not very expensive.

453. By Ricardo Salveti

Using a signal counter so we can set the volume asynchronously

Revision history for this message
Ricardo Salveti (rsalveti) wrote :

We indeed don't need to care about every single signal we get (specially when we're still waiting for replies from pulse), so we can coordinate that with a simple counter.

We could think about handling this in a list, but I think a simple counter is more than enough.

I decided to just assume that if the counter is not 0, we shouldn't need to care if an external entity ended up changing the volume, as that is not necessarily relevant anyway (due this sort of races that we can't avoid). Let me know if that is enough for you as well.

Revision history for this message
Ricardo Salveti (rsalveti) wrote :

Tested with the other packages and the UI is indeed a bit faster, and everything else working as expected.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Ted Gould (ted) wrote :

Cool, I think that works for now. It doesn't handle the case of someone constantly changing the volume, but that should never really happen -- or the device needs to be restarted anyway. Volume is the least of our problems ;-)

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/CMakeLists.txt'
2--- src/CMakeLists.txt 2014-03-04 21:40:28 +0000
3+++ src/CMakeLists.txt 2014-10-01 03:41:32 +0000
4@@ -21,6 +21,7 @@
5 accounts-service
6 PACKAGES
7 config
8+ gee-1.0
9 gio-2.0
10 gio-unix-2.0
11 libxml-2.0
12
13=== modified file 'src/volume-control.vala'
14--- src/volume-control.vala 2014-04-18 17:41:44 +0000
15+++ src/volume-control.vala 2014-10-01 03:41:32 +0000
16@@ -19,6 +19,7 @@
17 */
18
19 using PulseAudio;
20+using Gee;
21
22 [CCode(cname="pa_cvolume_set", cheader_filename = "pulse/volume.h")]
23 extern unowned PulseAudio.CVolume? vol_set (PulseAudio.CVolume? cv, uint channels, PulseAudio.Volume v);
24@@ -43,6 +44,21 @@
25 private double _volume = 0.0;
26 private double _mic_volume = 0.0;
27
28+ /* Used by the pulseaudio stream restore extension */
29+ private DBusConnection _pconn;
30+ /* Need both the list and hash so we can retrieve the last known sink-input after
31+ * releasing the current active one (restoring back to the previous known role) */
32+ private Gee.ArrayList<uint32> _sink_input_list = new Gee.ArrayList<uint32> ();
33+ private HashMap<uint32, string> _sink_input_hash = new HashMap<uint32, string> ();
34+ private bool _pulse_use_stream_restore = false;
35+ private uint32 _active_sink_input = -1;
36+ private string[] _valid_roles = {"multimedia", "alert", "alarm", "phone"};
37+ private string? _objp_role_multimedia = null;
38+ private string? _objp_role_alert = null;
39+ private string? _objp_role_alarm = null;
40+ private string? _objp_role_phone = null;
41+ private uint _pa_volume_sig_count = 0;
42+
43 private DBusProxy _user_proxy;
44 private GreeterListInterface _greeter_proxy;
45 private Cancellable _mute_cancellable;
46@@ -92,6 +108,26 @@
47 update_sink ();
48 break;
49
50+ case Context.SubscriptionEventType.SINK_INPUT:
51+ switch (t & Context.SubscriptionEventType.TYPE_MASK)
52+ {
53+ case Context.SubscriptionEventType.NEW:
54+ c.get_sink_input_info (index, handle_new_sink_input_cb);
55+ break;
56+
57+ case Context.SubscriptionEventType.CHANGE:
58+ c.get_sink_input_info (index, handle_changed_sink_input_cb);
59+ break;
60+
61+ case Context.SubscriptionEventType.REMOVE:
62+ remove_sink_input_from_list (index);
63+ break;
64+ default:
65+ debug ("Sink input event not known.");
66+ break;
67+ }
68+ break;
69+
70 case Context.SubscriptionEventType.SOURCE:
71 update_source ();
72 break;
73@@ -129,7 +165,8 @@
74 this.notify_property ("is-playing");
75 }
76
77- if (_volume != volume_to_double (i.volume.values[0]))
78+ if (_pulse_use_stream_restore == false &&
79+ _volume != volume_to_double (i.volume.values[0]))
80 {
81 _volume = volume_to_double (i.volume.values[0]);
82 volume_changed (_volume);
83@@ -170,6 +207,153 @@
84 context.get_server_info (update_source_get_server_info_cb);
85 }
86
87+ private DBusMessage pulse_dbus_filter (DBusConnection connection, owned DBusMessage message, bool incoming)
88+ {
89+ if (message.get_message_type () == DBusMessageType.SIGNAL) {
90+ string active_role_objp = _objp_role_alert;
91+ if (_active_sink_input != -1)
92+ active_role_objp = _sink_input_hash.get (_active_sink_input);
93+
94+ if (message.get_path () == active_role_objp && message.get_member () == "VolumeUpdated") {
95+ uint sig_count = 0;
96+ lock (_pa_volume_sig_count) {
97+ sig_count = _pa_volume_sig_count;
98+ if (_pa_volume_sig_count > 0)
99+ _pa_volume_sig_count--;
100+ }
101+
102+ /* We only care about signals if our internal count is zero */
103+ if (sig_count == 0) {
104+ /* Extract volume and make sure it's not a side effect of us setting it */
105+ Variant body = message.get_body ();
106+ Variant varray = body.get_child_value (0);
107+
108+ uint32 type = 0, volume = 0;
109+ VariantIter iter = varray.iterator ();
110+ iter.next ("(uu)", &type, &volume);
111+ /* Here we need to compare integer values to avoid rounding issues, so just
112+ * using the volume values used by pulseaudio */
113+ PulseAudio.Volume cvolume = double_to_volume (_volume);
114+ if (volume != cvolume) {
115+ /* Someone else changed the volume for this role, reflect on the indicator */
116+ _volume = volume_to_double (volume);
117+ volume_changed (_volume);
118+ }
119+ }
120+ }
121+ }
122+
123+ return message;
124+ }
125+
126+ private async void update_active_sink_input (uint32 index)
127+ {
128+ if ((index == -1) || (index != _active_sink_input && index in _sink_input_list)) {
129+ string sink_input_objp = _objp_role_alert;
130+ if (index != -1)
131+ sink_input_objp = _sink_input_hash.get (index);
132+ _active_sink_input = index;
133+
134+ /* Listen for role volume changes from pulse itself (external clients) */
135+ try {
136+ var builder = new VariantBuilder (new VariantType ("ao"));
137+ builder.add ("o", sink_input_objp);
138+
139+ yield _pconn.call ("org.PulseAudio.Core1", "/org/pulseaudio/core1",
140+ "org.PulseAudio.Core1", "ListenForSignal",
141+ new Variant ("(sao)", "org.PulseAudio.Ext.StreamRestore1.RestoreEntry.VolumeUpdated", builder),
142+ null, DBusCallFlags.NONE, -1);
143+ } catch (GLib.Error e) {
144+ warning ("unable to listen for pulseaudio dbus signals (%s)", e.message);
145+ }
146+
147+ try {
148+ var props_variant = yield _pconn.call ("org.PulseAudio.Ext.StreamRestore1.RestoreEntry",
149+ sink_input_objp, "org.freedesktop.DBus.Properties", "Get",
150+ new Variant ("(ss)", "org.PulseAudio.Ext.StreamRestore1.RestoreEntry", "Volume"),
151+ null, DBusCallFlags.NONE, -1);
152+ Variant tmp;
153+ props_variant.get ("(v)", out tmp);
154+ uint32 type = 0, volume = 0;
155+ VariantIter iter = tmp.iterator ();
156+ iter.next ("(uu)", &type, &volume);
157+
158+ _volume = volume_to_double (volume);
159+ volume_changed (_volume);
160+ } catch (GLib.Error e) {
161+ warning ("unable to get volume for active role %s (%s)", sink_input_objp, e.message);
162+ }
163+ }
164+ }
165+
166+ private void add_sink_input_into_list (SinkInputInfo sink_input)
167+ {
168+ /* We're only adding ones that are not corked and with a valid role */
169+ var role = sink_input.proplist.gets (PulseAudio.Proplist.PROP_MEDIA_ROLE);
170+
171+ if (role != null && role in _valid_roles) {
172+ if (sink_input.corked == 0 || role == "phone") {
173+ _sink_input_list.insert (0, sink_input.index);
174+ switch (role)
175+ {
176+ case "multimedia":
177+ _sink_input_hash.set (sink_input.index, _objp_role_multimedia);
178+ break;
179+ case "alert":
180+ _sink_input_hash.set (sink_input.index, _objp_role_alert);
181+ break;
182+ case "alarm":
183+ _sink_input_hash.set (sink_input.index, _objp_role_alarm);
184+ break;
185+ case "phone":
186+ _sink_input_hash.set (sink_input.index, _objp_role_phone);
187+ break;
188+ }
189+ /* Only switch the active sink input in case a phone one is not active */
190+ if (_active_sink_input == -1 ||
191+ _sink_input_hash.get (_active_sink_input) != _objp_role_phone)
192+ update_active_sink_input.begin (sink_input.index);
193+ }
194+ }
195+ }
196+
197+ private void remove_sink_input_from_list (uint32 index)
198+ {
199+ if (index in _sink_input_list) {
200+ _sink_input_list.remove (index);
201+ _sink_input_hash.unset (index);
202+ if (index == _active_sink_input) {
203+ if (_sink_input_list.size != 0)
204+ update_active_sink_input.begin (_sink_input_list.get (0));
205+ else
206+ update_active_sink_input.begin (-1);
207+ }
208+ }
209+ }
210+
211+ private void handle_new_sink_input_cb (Context c, SinkInputInfo? i, int eol)
212+ {
213+ if (i == null)
214+ return;
215+
216+ add_sink_input_into_list (i);
217+ }
218+
219+ private void handle_changed_sink_input_cb (Context c, SinkInputInfo? i, int eol)
220+ {
221+ if (i == null)
222+ return;
223+
224+ if (i.index in _sink_input_list) {
225+ /* Phone stream is always corked, so handle it differently */
226+ if (i.corked == 1 && _sink_input_hash.get (i.index) != _objp_role_phone)
227+ remove_sink_input_from_list (i.index);
228+ } else {
229+ if (i.corked == 0)
230+ add_sink_input_into_list (i);
231+ }
232+ }
233+
234 private void source_output_info_cb (Context c, SourceOutputInfo? i, int eol)
235 {
236 if (i == null)
237@@ -184,9 +368,16 @@
238 {
239 switch (c.get_state ()) {
240 case Context.State.READY:
241- c.subscribe (PulseAudio.Context.SubscriptionMask.SINK |
242- PulseAudio.Context.SubscriptionMask.SOURCE |
243- PulseAudio.Context.SubscriptionMask.SOURCE_OUTPUT);
244+ if (_pulse_use_stream_restore) {
245+ c.subscribe (PulseAudio.Context.SubscriptionMask.SINK |
246+ PulseAudio.Context.SubscriptionMask.SINK_INPUT |
247+ PulseAudio.Context.SubscriptionMask.SOURCE |
248+ PulseAudio.Context.SubscriptionMask.SOURCE_OUTPUT);
249+ } else {
250+ c.subscribe (PulseAudio.Context.SubscriptionMask.SINK |
251+ PulseAudio.Context.SubscriptionMask.SOURCE |
252+ PulseAudio.Context.SubscriptionMask.SOURCE_OUTPUT);
253+ }
254 c.set_subscribe_callback (context_events_cb);
255 update_sink ();
256 update_source ();
257@@ -226,6 +417,8 @@
258 props.sets (Proplist.PROP_APPLICATION_ICON_NAME, "multimedia-volume-control");
259 props.sets (Proplist.PROP_APPLICATION_VERSION, "0.1");
260
261+ reconnect_pulse_dbus ();
262+
263 this.context = new PulseAudio.Context (loop.get_api(), null, props);
264 this.context.set_state_callback (context_state_callback);
265
266@@ -325,13 +518,47 @@
267 context.get_sink_info_by_name (i.default_sink_name, sink_info_set_volume_cb);
268 }
269
270+ private async void set_volume_active_role ()
271+ {
272+ string active_role_objp = _objp_role_alert;
273+
274+ if (_active_sink_input != -1 && _active_sink_input in _sink_input_list)
275+ active_role_objp = _sink_input_hash.get (_active_sink_input);
276+
277+ try {
278+ var builder = new VariantBuilder (new VariantType ("a(uu)"));
279+ builder.add ("(uu)", 0, double_to_volume (_volume));
280+ Variant volume = builder.end ();
281+
282+ /* Increase the signal counter so we can handle the callback */
283+ lock (_pa_volume_sig_count) {
284+ _pa_volume_sig_count++;
285+ }
286+
287+ yield _pconn.call ("org.PulseAudio.Ext.StreamRestore1.RestoreEntry",
288+ active_role_objp, "org.freedesktop.DBus.Properties", "Set",
289+ new Variant ("(ssv)", "org.PulseAudio.Ext.StreamRestore1.RestoreEntry", "Volume", volume),
290+ null, DBusCallFlags.NONE, -1);
291+
292+ volume_changed (_volume);
293+ } catch (GLib.Error e) {
294+ lock (_pa_volume_sig_count) {
295+ _pa_volume_sig_count--;
296+ }
297+ warning ("unable to set volume for stream obj path %s (%s)", active_role_objp, e.message);
298+ }
299+ }
300+
301 bool set_volume_internal (double volume)
302 {
303 return_val_if_fail (context.get_state () == Context.State.READY, false);
304
305 if (_volume != volume) {
306 _volume = volume;
307- context.get_server_info (server_info_cb_for_set_volume);
308+ if (_pulse_use_stream_restore)
309+ set_volume_active_role.begin ();
310+ else
311+ context.get_server_info (server_info_cb_for_set_volume);
312 return true;
313 } else {
314 return false;
315@@ -377,6 +604,85 @@
316 return _mic_volume;
317 }
318
319+ /* PulseAudio Dbus (Stream Restore) logic */
320+ private void reconnect_pulse_dbus ()
321+ {
322+ unowned string pulse_dbus_server_env = Environment.get_variable ("PULSE_DBUS_SERVER");
323+ string address;
324+
325+ /* In case of a reconnect */
326+ _pulse_use_stream_restore = false;
327+ _pa_volume_sig_count = 0;
328+
329+ if (pulse_dbus_server_env != null) {
330+ address = pulse_dbus_server_env;
331+ } else {
332+ DBusConnection conn;
333+ Variant props;
334+
335+ try {
336+ conn = Bus.get_sync (BusType.SESSION);
337+ } catch (GLib.IOError e) {
338+ warning ("unable to get the dbus session bus: %s", e.message);
339+ return;
340+ }
341+
342+ try {
343+ var props_variant = conn.call_sync ("org.PulseAudio1",
344+ "/org/pulseaudio/server_lookup1", "org.freedesktop.DBus.Properties",
345+ "Get", new Variant ("(ss)", "org.PulseAudio.ServerLookup1", "Address"),
346+ null, DBusCallFlags.NONE, -1);
347+ props_variant.get ("(v)", out props);
348+ address = props.get_string ();
349+ } catch (GLib.Error e) {
350+ warning ("unable to get pulse unix socket: %s", e.message);
351+ return;
352+ }
353+ }
354+
355+ stdout.printf ("PulseAudio dbus unix socket: %s\n", address);
356+ try {
357+ _pconn = new DBusConnection.for_address_sync (address, DBusConnectionFlags.AUTHENTICATION_CLIENT);
358+ } catch (GLib.Error e) {
359+ /* If it fails, it means the dbus pulse extension is not available */
360+ return;
361+ }
362+
363+ /* For pulse dbus related events */
364+ _pconn.add_filter (pulse_dbus_filter);
365+
366+ /* Check if the 4 currently supported media roles are already available in StreamRestore
367+ * Roles: multimedia, alert, alarm and phone */
368+ _objp_role_multimedia = stream_restore_get_object_path ("sink-input-by-media-role:multimedia");
369+ _objp_role_alert = stream_restore_get_object_path ("sink-input-by-media-role:alert");
370+ _objp_role_alarm = stream_restore_get_object_path ("sink-input-by-media-role:alarm");
371+ _objp_role_phone = stream_restore_get_object_path ("sink-input-by-media-role:phone");
372+
373+ /* Only use stream restore if every used role is available */
374+ if (_objp_role_multimedia != null && _objp_role_alert != null && _objp_role_alarm != null && _objp_role_phone != null) {
375+ stdout.printf ("Using PulseAudio DBUS Stream Restore module\n");
376+ /* Restore volume and update default entry */
377+ update_active_sink_input.begin (-1);
378+ _pulse_use_stream_restore = true;
379+ }
380+ }
381+
382+ private string? stream_restore_get_object_path (string name) {
383+ string? objp = null;
384+ try {
385+ Variant props_variant = _pconn.call_sync ("org.PulseAudio.Ext.StreamRestore1",
386+ "/org/pulseaudio/stream_restore1", "org.PulseAudio.Ext.StreamRestore1",
387+ "GetEntryByName", new Variant ("(s)", name), null, DBusCallFlags.NONE, -1);
388+ /* Workaround for older versions of vala that don't provide get_objv */
389+ VariantIter iter = props_variant.iterator ();
390+ iter.next ("o", &objp);
391+ stdout.printf ("Found obj path %s for restore data named %s\n", objp, name);
392+ } catch (GLib.Error e) {
393+ warning ("unable to find stream restore data for: %s", name);
394+ }
395+ return objp;
396+ }
397+
398 /* AccountsService operations */
399 private void accountsservice_props_changed_cb (DBusProxy proxy, Variant changed_properties, string[] invalidated_properties)
400 {

Subscribers

People subscribed via source and target branches