Merge lp:~d.filoni/indicator-datetime/lp1364647 into lp:indicator-datetime

Proposed by Devid Antonio Filoni on 2016-08-21
Status: Needs review
Proposed branch: lp:~d.filoni/indicator-datetime/lp1364647
Merge into: lp:indicator-datetime
Diff against target: 478 lines (+380/-2)
5 files modified
CMakeLists.txt (+2/-1)
INSTALL (+1/-0)
debian/control (+1/-0)
include/notifications/sound.h (+1/-0)
src/sound.cpp (+375/-1)
To merge this branch: bzr merge lp:~d.filoni/indicator-datetime/lp1364647
Reviewer Review Type Date Requested Status
Charles Kerr 2016-08-22 Pending
Tiago Salem Herrmann 2016-08-22 Pending
Indicator Applet Developers 2016-08-21 Pending
Review via email: mp+303494@code.launchpad.net

Description of the change

Hi,
commit 460 MR introduces a libpulse implementation in order to set output port to both spearkers and headphones when playing an alarm as suggested by Pat in comment #17 (bug #1364647) and as already done in telepathy-ofono ( [1] )

commit 461 introduces a g_timeout_add_seconds call to set output port after 5 seconds since alarm start. I'm not sure 5 is a reasonable amount of time, in bug #1583981 (equivalent issue for ringtone) there is a linked spec but I don't have access to it.

This was tested on Nexus 4 (there is a build in my mako PPA of the same patch).

[1] http://bazaar.launchpad.net/~phablet-team/telepathy-ofono/trunk/view/head:/qpulseaudioengine.cpp

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

I found an issue testing commit 461 and fixed it in commit 462: I unplugged headphones after 5 seconds from alarm start (so when port switched to speakers) and closing the notification port was reset to headphones, so audio was not working. With commit 462 headphones port is not restored if it's not available anymore.

Charles Kerr (charlesk) wrote :

The patch looks OK and we could take it as an interim step, but IMO these sound issues would be better handled in unity8 / unity-notifications.

IMO what indicator-datetime should be doing is passng the 'sound-file' hint to notifications. The reason it's not doing that right now is the need to loop the sound, but if unity8/unity-notifications supported a 'loop' hint then we could do away with the custom sound code in indicator-datetime completely.

As for the code itself, looks like a pretty good start. Comments inline.

Devid Antonio Filoni (d.filoni) wrote :

IMHO pulse connections and methods should be handled in a common (for all Ubuntu apps/services) C++ library, looks like we are using too many types of APIs (pulse itself, qmultimedia, gstreamer, dbus), so it's easy to introduce bugs/unwanted behavior between apps...

Thank you Charles! I'm returning to C++ after many years so I really appreciate your detailed review, I won't repeat these mistakes again ;)
I'll try and commit all your suggestions ASAP, I commented on few of them

Unmerged revisions

464. By Devid Antonio Filoni on 2016-08-22

Apply code changes suggested by charlesk

463. By Devid Antonio Filoni on 2016-08-21

Remove useless g_critical call set during sink port change debug

462. By Devid Antonio Filoni on 2016-08-21

Do not restore original sink port on alarm end if port is not available anymore or already active.

461. By Devid Antonio Filoni on 2016-08-21

Play alarms through speakers after 5 seconds when speakers are not active (LP: #1364647)

460. By Devid Antonio Filoni on 2016-08-21

Prefer speakers over headphones when playing alarm (LP: #1364647)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'CMakeLists.txt'
2--- CMakeLists.txt 2016-05-14 02:07:10 +0000
3+++ CMakeLists.txt 2016-08-22 19:08:58 +0000
4@@ -51,7 +51,8 @@
5 libaccounts-glib>=1.18
6 messaging-menu>=12.10
7 uuid>=2.25
8- ubuntu-app-launch-2)
9+ ubuntu-app-launch-2
10+ libpulse)
11 include_directories (SYSTEM ${SERVICE_DEPS_INCLUDE_DIRS})
12
13 ##
14
15=== modified file 'INSTALL'
16--- INSTALL 2013-10-29 16:50:36 +0000
17+++ INSTALL 2016-08-22 19:08:58 +0000
18@@ -27,6 +27,7 @@
19 * libnotify >= 0.7.6
20 * url-dispatcher-1 >= 1
21 * json-glib-1.0 >= 0.16.2
22+ * libpulse
23
24
25 Additional build dependencies for the gnome-control-center panel:
26
27=== modified file 'debian/control'
28--- debian/control 2016-07-01 19:46:40 +0000
29+++ debian/control 2016-08-22 19:08:58 +0000
30@@ -14,6 +14,7 @@
31 libedataserver1.2-dev (>= 3.5),
32 liburl-dispatcher1-dev,
33 libproperties-cpp-dev,
34+ libpulse-dev,
35 # for com.ubuntu.notifications.hub schema to pick up blacklist of apps that can't show notifications
36 gsettings-ubuntu-schemas (>= 0.0.7),
37 # for the test harness:
38
39=== modified file 'include/notifications/sound.h'
40--- include/notifications/sound.h 2015-10-13 16:06:35 +0000
41+++ include/notifications/sound.h 2016-08-22 19:08:58 +0000
42@@ -45,6 +45,7 @@
43 ~Sound();
44
45 private:
46+ class PulseImpl;
47 class Impl;
48 std::unique_ptr<Impl> impl;
49 };
50
51=== modified file 'src/sound.cpp'
52--- src/sound.cpp 2015-01-22 01:57:59 +0000
53+++ src/sound.cpp 2016-08-22 19:08:58 +0000
54@@ -23,6 +23,11 @@
55
56 #include <mutex> // std::call_once()
57
58+/* PulseImpl */
59+#include <sys/types.h>
60+#include <unistd.h>
61+#include <pulse/pulseaudio.h>
62+
63 namespace unity {
64 namespace indicator {
65 namespace notifications {
66@@ -32,6 +37,348 @@
67 ***/
68
69 /**
70+ * Changes pulseaudio output to speakers when needed
71+ */
72+class Sound::PulseImpl
73+{
74+public:
75+
76+ PulseImpl()
77+ {
78+ m_mainloop = pa_threaded_mainloop_new();
79+ if (m_mainloop == nullptr) {
80+ g_warning("Unable to create pulseaudio mainloop");
81+ m_mainloop = nullptr;
82+ return;
83+ }
84+
85+ if (pa_threaded_mainloop_start(m_mainloop) != 0) {
86+ g_warning("Unable to start pulseaudio mainloop");
87+ pa_threaded_mainloop_free(m_mainloop);
88+ m_mainloop = nullptr;
89+ return;
90+ }
91+
92+ create_context();
93+ }
94+
95+ bool set_preferred_sink_port()
96+ {
97+ m_set_preferred_sink_port = true;
98+
99+ if (!create_context())
100+ return false;
101+
102+ pa_threaded_mainloop_lock(m_mainloop);
103+
104+ pa_operation *o;
105+
106+ /* Get default sink name */
107+ o = pa_context_get_server_info(m_context, get_server_info_callback, this);
108+ if (!handle_operation(o, "pa_context_get_server_info"))
109+ return false;
110+
111+ /* Get default sink info */
112+ o = pa_context_get_sink_info_by_name(m_context,
113+ m_default_sink_name.c_str(),
114+ get_sink_info_by_name_callback,
115+ this);
116+ if (!handle_operation(o, "pa_context_get_sink_info_by_name"))
117+ return false;
118+
119+ /* If needed, change default sink output port */
120+ if (!m_preferred_port_name.empty())
121+ {
122+ g_debug("Setting pulseaudio sink '%s' port from '%s' to '%s'", m_default_sink_name.c_str(), m_old_active_port_name.c_str(), m_preferred_port_name.c_str());
123+ o = pa_context_set_sink_port_by_name(m_context,
124+ m_default_sink_name.c_str(),
125+ m_preferred_port_name.c_str(),
126+ success_callback, this);
127+ if (handle_operation(o, "pa_context_set_sink_port_by_name"))
128+ {
129+ /* Discard m_preferred_port_name to avoid previous operation when port is already set */
130+ m_preferred_port_name = "";
131+ }
132+ else
133+ return false;
134+ }
135+
136+ pa_threaded_mainloop_unlock(m_mainloop);
137+
138+ return true;
139+ }
140+
141+ ~PulseImpl()
142+ {
143+ restore_sink_port();
144+ release_context();
145+
146+ if (m_mainloop) {
147+ pa_threaded_mainloop_stop(m_mainloop);
148+ pa_threaded_mainloop_free(m_mainloop);
149+ m_mainloop = nullptr;
150+ }
151+ }
152+
153+
154+private:
155+ static void success_callback(pa_context* /*context*/, int /*success*/, void *userdata)
156+ {
157+ auto self = static_cast<PulseImpl*>(userdata);
158+ pa_threaded_mainloop_signal(self->mainloop(), 0);
159+ }
160+
161+ static void set_state_callback(pa_context* /*context*/, void *userdata)
162+ {
163+ auto self = static_cast<PulseImpl*>(userdata);
164+ pa_threaded_mainloop_signal(self->mainloop(), 0);
165+ }
166+
167+ static void get_server_info_callback(pa_context* /*context*/, const pa_server_info *info, void *userdata)
168+ {
169+ auto self = static_cast<PulseImpl*>(userdata);
170+ self->handle_server_info(info);
171+ pa_threaded_mainloop_signal(self->mainloop(), 0);
172+ }
173+
174+ static void get_sink_info_by_name_callback(pa_context* /*context*/, const pa_sink_info *info, int eol, void *userdata)
175+ {
176+ if (eol)
177+ return;
178+
179+ auto self = static_cast<PulseImpl*>(userdata);
180+ self->handle_sink_info(info);
181+ pa_threaded_mainloop_signal(self->mainloop(), 0);
182+ }
183+
184+ pa_threaded_mainloop *mainloop() { return m_mainloop; } // this can be made public
185+
186+ void handle_server_info(const pa_server_info *info) // this can be made public
187+ {
188+ m_default_sink_name = info->default_sink_name;
189+ }
190+
191+ void handle_sink_info(const pa_sink_info *info) // this can be made public
192+ {
193+ if (!m_old_active_port_name.empty())
194+ internal_reset_old_active_sink_port(info);
195+
196+ if (m_set_preferred_sink_port)
197+ internal_set_preferred_sink_port(info);
198+ }
199+
200+ bool restore_sink_port() // this can be made public
201+ {
202+ /* If default sink port was changed, restore it */
203+ if (m_context &&
204+ m_set_preferred_sink_port &&
205+ !m_old_active_port_name.empty())
206+ {
207+ m_set_preferred_sink_port = false;
208+
209+ pa_threaded_mainloop_lock(m_mainloop);
210+
211+ pa_operation *o;
212+
213+ /* Get default sink info */
214+ o = pa_context_get_sink_info_by_name(m_context,
215+ m_default_sink_name.c_str(),
216+ get_sink_info_by_name_callback,
217+ this);
218+ if (!handle_operation(o, "pa_context_get_sink_info_by_name"))
219+ return false;
220+
221+ if (!m_old_active_port_name.empty())
222+ {
223+ g_debug("Restoring pulseaudio sink '%s' port to '%s'", m_default_sink_name.c_str(), m_old_active_port_name.c_str());
224+
225+ o = pa_context_set_sink_port_by_name(m_context,
226+ m_default_sink_name.c_str(),
227+ m_old_active_port_name.c_str(),
228+ success_callback, this);
229+ if (!handle_operation(o, "pa_context_set_sink_port_by_name"))
230+ return false;
231+
232+ m_old_active_port_name = "";
233+ }
234+
235+ pa_threaded_mainloop_unlock(m_mainloop);
236+ }
237+
238+ return true;
239+ }
240+
241+ bool create_context()
242+ {
243+ bool keepGoing = true;
244+ bool connected = false;
245+
246+ if (m_context)
247+ return true;
248+
249+ auto mainloop_api = pa_threaded_mainloop_get_api(m_mainloop);
250+
251+ pa_threaded_mainloop_lock(m_mainloop);
252+
253+ const auto name = "QtmPulseContext:" + std::to_string(::getpid());
254+ m_context = pa_context_new(mainloop_api, name.c_str());
255+ if (!m_context) {
256+ g_critical("Unable to create new pulseaudio context");
257+ pa_threaded_mainloop_unlock(m_mainloop);
258+ return false;
259+ }
260+
261+ pa_context_set_state_callback(m_context, set_state_callback, this);
262+ if (pa_context_connect(m_context, NULL, PA_CONTEXT_NOFLAGS, NULL) < 0) {
263+ g_critical("Unable to create a connection to the pulseaudio context");
264+ pa_threaded_mainloop_unlock(m_mainloop);
265+ release_context();
266+ return false;
267+ }
268+
269+ g_debug("Connecting to the pulseaudio context");
270+ pa_threaded_mainloop_wait(m_mainloop);
271+ while (keepGoing) {
272+ switch (pa_context_get_state(m_context)) {
273+ case PA_CONTEXT_CONNECTING:
274+ case PA_CONTEXT_AUTHORIZING:
275+ case PA_CONTEXT_SETTING_NAME:
276+ break;
277+
278+ case PA_CONTEXT_READY:
279+ g_debug("Pulseaudio connection established.");
280+ keepGoing = false;
281+ connected = true;
282+ break;
283+
284+ case PA_CONTEXT_TERMINATED:
285+ g_critical("Pulseaudio context terminated.");
286+ keepGoing = false;
287+ break;
288+
289+ case PA_CONTEXT_FAILED:
290+ default:
291+ g_critical("Pulseaudio connection failure: %s", pa_strerror(pa_context_errno(m_context)));
292+ keepGoing = false;
293+ }
294+
295+ if (keepGoing) {
296+ pa_threaded_mainloop_wait(m_mainloop);
297+ }
298+ }
299+
300+ if (!connected) {
301+ if (m_context) {
302+ pa_context_unref(m_context);
303+ m_context = nullptr;
304+ }
305+ }
306+
307+ pa_threaded_mainloop_unlock(m_mainloop);
308+
309+ return connected;
310+ }
311+
312+ bool handle_operation(pa_operation *operation, const char *func_name)
313+ {
314+ if (!operation) {
315+ g_critical("'%s' failed (lost pulseaudio connection?)", func_name);
316+ /* Free resources so it can retry a new connection during next operation */
317+ pa_threaded_mainloop_unlock(m_mainloop);
318+ release_context();
319+ return false;
320+ }
321+
322+ while (pa_operation_get_state(operation) == PA_OPERATION_RUNNING)
323+ pa_threaded_mainloop_wait(m_mainloop);
324+
325+ pa_operation_unref(operation);
326+
327+ return true;
328+ }
329+
330+ void release_context()
331+ {
332+ if (m_context) {
333+ pa_threaded_mainloop_lock(m_mainloop);
334+ pa_context_disconnect(m_context);
335+ pa_context_unref(m_context);
336+ pa_threaded_mainloop_unlock(m_mainloop);
337+ m_context = nullptr;
338+ }
339+ }
340+
341+ void internal_set_preferred_sink_port(const pa_sink_info *info)
342+ {
343+ /* Prefer speakers over headphones when playing audio (LP: #1364647) */
344+ pa_sink_port_info* active_port = info->active_port;
345+ pa_sink_port_info* speaker_port = nullptr;
346+ pa_sink_port_info* speaker_wired_headphone_port = nullptr;
347+
348+ for (unsigned int i = 0; i < info->n_ports; i++)
349+ {
350+ if (info->ports[i]->available != PA_PORT_AVAILABLE_NO)
351+ {
352+ if (g_strcmp0(info->ports[i]->name, "output-speaker") == 0)
353+ speaker_port = info->ports[i];
354+ else if (g_strcmp0(info->ports[i]->name, "output-speaker+wired_headphone") == 0)
355+ speaker_wired_headphone_port = info->ports[i];
356+ }
357+ }
358+
359+ pa_sink_port_info* preferred_port = nullptr;
360+ if (speaker_wired_headphone_port != nullptr)
361+ preferred_port = speaker_wired_headphone_port;
362+ else
363+ preferred_port = speaker_port;
364+
365+ if (preferred_port != nullptr &&
366+ g_strcmp0(active_port->name, "output-speaker") != 0 &&
367+ active_port != preferred_port)
368+ {
369+ m_old_active_port_name = active_port->name;
370+ m_preferred_port_name = preferred_port->name;
371+ }
372+ }
373+
374+ void internal_reset_old_active_sink_port(const pa_sink_info *info)
375+ {
376+ /* Check if old active port is still available and inactive */
377+ bool old_active_port_found = false;
378+ for (unsigned int i = 0; i < info->n_ports; i++)
379+ {
380+ if (info->ports[i]->available != PA_PORT_AVAILABLE_NO)
381+ {
382+ if (g_strcmp0(info->ports[i]->name, m_old_active_port_name.c_str()) == 0)
383+ old_active_port_found = true;
384+ }
385+ }
386+ if (!old_active_port_found ||
387+ g_strcmp0(info->active_port->name, m_old_active_port_name.c_str()) == 0)
388+ {
389+ m_old_active_port_name = "";
390+ }
391+ else if (g_strcmp0(info->active_port->name, "output-speaker") != 0 &&
392+ g_strcmp0(info->active_port->name, "output-speaker+wired_headphone") != 0)
393+ {
394+ // looks like port changed while playing
395+ m_old_active_port_name = "";
396+ }
397+ }
398+
399+ /***
400+ ****
401+ ***/
402+
403+ pa_threaded_mainloop *m_mainloop = nullptr;
404+ pa_context *m_context = nullptr;
405+ std::string m_default_sink_name = "";
406+ std::string m_old_active_port_name = "";
407+ std::string m_preferred_port_name = "";
408+ bool m_set_preferred_sink_port = false;
409+};
410+
411+/**
412 * Plays a sound, possibly looping.
413 */
414 class Sound::Impl
415@@ -73,6 +420,12 @@
416
417 ~Impl()
418 {
419+ if (m_pulse_timeout != 0)
420+ g_source_remove(m_pulse_timeout);
421+
422+ if (m_pulse != nullptr)
423+ delete m_pulse;
424+
425 g_source_remove(m_watch_source);
426
427 if (m_play != nullptr)
428@@ -97,6 +450,20 @@
429 return out_range_lo + (pct * (out_range_hi - out_range_lo));
430 }
431
432+ static gboolean pulse_timer_callback(gpointer gself)
433+ {
434+ auto self = static_cast<Impl*>(gself);
435+
436+ if (self->m_pulse == nullptr)
437+ self->m_pulse = new PulseImpl();
438+
439+ /* Set preferred sink port */
440+ self->m_pulse->set_preferred_sink_port();
441+
442+ self->m_pulse_timeout = 0;
443+ return G_SOURCE_REMOVE;
444+ }
445+
446 static gboolean bus_callback(GstBus*, GstMessage* msg, gpointer gself)
447 {
448 auto self = static_cast<Impl*>(gself);
449@@ -115,7 +482,7 @@
450 }
451 else if (message_type == GST_MESSAGE_STREAM_START)
452 {
453- /* Set the media role if audio sink is pulsesink */
454+ /* Set the media role and pulse timer if audio sink is pulsesink */
455 GstElement *audio_sink = nullptr;
456 g_object_get(self->m_play, "audio-sink", &audio_sink, nullptr);
457 if (audio_sink)
458@@ -129,6 +496,11 @@
459 g_object_set(audio_sink, "stream-properties", props, nullptr);
460 gst_structure_free(props);
461 g_free(role_str);
462+
463+ /* Set preferred sink port after 5 seconds */
464+ if (self->m_pulse == nullptr &&
465+ self->m_pulse_timeout == 0)
466+ self->m_pulse_timeout = g_timeout_add_seconds(5, pulse_timer_callback, self);
467 }
468 gst_object_unref(audio_sink);
469 }
470@@ -147,6 +519,8 @@
471 const bool m_loop;
472 guint m_watch_source = 0;
473 GstElement* m_play = nullptr;
474+ guint m_pulse_timeout = 0;
475+ PulseImpl* m_pulse = nullptr;
476 };
477
478 Sound::Sound(const std::string& role, const std::string& uri, unsigned int volume, bool loop):

Subscribers

People subscribed via source and target branches