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

[Devid Antonio Filoni]

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

[Devid Antonio Filoni (d.filoni)]

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)]

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)]

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)

=== modified file 'CMakeLists.txt'
--- CMakeLists.txt	2016-05-14 02:07:10 +0000
+++ CMakeLists.txt	2016-08-22 19:08:58 +0000
@@ -51,7 +51,8 @@
                    libaccounts-glib>=1.18
                    messaging-menu>=12.10
                    uuid>=2.25
-                   ubuntu-app-launch-2)
+                   ubuntu-app-launch-2
+                   libpulse)
 include_directories (SYSTEM ${SERVICE_DEPS_INCLUDE_DIRS})
 
 ##

=== modified file 'INSTALL'
--- INSTALL	2013-10-29 16:50:36 +0000
+++ INSTALL	2016-08-22 19:08:58 +0000
@@ -27,6 +27,7 @@
  * libnotify >= 0.7.6
  * url-dispatcher-1 >= 1
  * json-glib-1.0 >= 0.16.2
+ * libpulse
 
 
 Additional build dependencies for the gnome-control-center panel:

=== modified file 'debian/control'
--- debian/control	2016-07-01 19:46:40 +0000
+++ debian/control	2016-08-22 19:08:58 +0000
@@ -14,6 +14,7 @@
                libedataserver1.2-dev (>= 3.5),
                liburl-dispatcher1-dev,
                libproperties-cpp-dev,
+               libpulse-dev,
 # for com.ubuntu.notifications.hub schema to pick up blacklist of apps that can't show notifications
                gsettings-ubuntu-schemas (>= 0.0.7),
 # for the test harness:

=== modified file 'include/notifications/sound.h'
--- include/notifications/sound.h	2015-10-13 16:06:35 +0000
+++ include/notifications/sound.h	2016-08-22 19:08:58 +0000
@@ -45,6 +45,7 @@
     ~Sound();
 
 private:
+    class PulseImpl;
     class Impl;
     std::unique_ptr<Impl> impl;
 };

=== modified file 'src/sound.cpp'
--- src/sound.cpp	2015-01-22 01:57:59 +0000
+++ src/sound.cpp	2016-08-22 19:08:58 +0000
@@ -23,6 +23,11 @@
 
 #include <mutex> // std::call_once()
 
+/* PulseImpl */
+#include <sys/types.h>
+#include <unistd.h>
+#include <pulse/pulseaudio.h>
+
 namespace unity {
 namespace indicator {
 namespace notifications {
@@ -32,6 +37,348 @@
 ***/
 
 /**
+ * Changes pulseaudio output to speakers when needed
+ */
+class Sound::PulseImpl
+{
+public:
+
+    PulseImpl()
+    {
+        m_mainloop = pa_threaded_mainloop_new();
+        if (m_mainloop == nullptr) {
+            g_warning("Unable to create pulseaudio mainloop");
+            m_mainloop = nullptr;
+            return;
+        }
+
+        if (pa_threaded_mainloop_start(m_mainloop) != 0) {
+            g_warning("Unable to start pulseaudio mainloop");
+            pa_threaded_mainloop_free(m_mainloop);
+            m_mainloop = nullptr;
+            return;
+        }
+
+        create_context();
+    }
+
+    bool set_preferred_sink_port()
+    {
+        m_set_preferred_sink_port = true;
+
+        if (!create_context())
+            return false;
+
+        pa_threaded_mainloop_lock(m_mainloop);
+
+        pa_operation *o;
+
+        /* Get default sink name */
+        o = pa_context_get_server_info(m_context, get_server_info_callback, this);
+        if (!handle_operation(o, "pa_context_get_server_info"))
+            return false;
+
+        /* Get default sink info */
+        o = pa_context_get_sink_info_by_name(m_context,
+                                             m_default_sink_name.c_str(),
+                                             get_sink_info_by_name_callback,
+                                             this);
+        if (!handle_operation(o, "pa_context_get_sink_info_by_name"))
+            return false;
+
+        /* If needed, change default sink output port */
+        if (!m_preferred_port_name.empty())
+        {
+            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());
+            o = pa_context_set_sink_port_by_name(m_context,
+                                                 m_default_sink_name.c_str(),
+                                                 m_preferred_port_name.c_str(),
+                                                 success_callback, this);
+            if (handle_operation(o, "pa_context_set_sink_port_by_name"))
+            {
+                /* Discard m_preferred_port_name to avoid previous operation when port is already set */
+                m_preferred_port_name = "";
+            }
+            else
+                return false;
+        }
+
+        pa_threaded_mainloop_unlock(m_mainloop);
+
+        return true;
+    }
+
+    ~PulseImpl()
+    {
+        restore_sink_port();
+        release_context();
+
+        if (m_mainloop) {
+            pa_threaded_mainloop_stop(m_mainloop);
+            pa_threaded_mainloop_free(m_mainloop);
+            m_mainloop = nullptr;
+        }
+    }
+
+
+private:
+    static void success_callback(pa_context* /*context*/, int /*success*/, void *userdata)
+    {
+        auto self = static_cast<PulseImpl*>(userdata);
+        pa_threaded_mainloop_signal(self->mainloop(), 0);
+    }
+
+    static void set_state_callback(pa_context* /*context*/, void *userdata)
+    {
+        auto self = static_cast<PulseImpl*>(userdata);
+        pa_threaded_mainloop_signal(self->mainloop(), 0);
+    }
+
+    static void get_server_info_callback(pa_context* /*context*/, const pa_server_info *info, void *userdata)
+    {
+        auto self = static_cast<PulseImpl*>(userdata);
+        self->handle_server_info(info);
+        pa_threaded_mainloop_signal(self->mainloop(), 0);
+    }
+
+    static void get_sink_info_by_name_callback(pa_context* /*context*/, const pa_sink_info *info, int eol, void *userdata)
+    {
+        if (eol)
+            return;
+
+        auto self = static_cast<PulseImpl*>(userdata);
+        self->handle_sink_info(info);
+        pa_threaded_mainloop_signal(self->mainloop(), 0);
+    }
+
+    pa_threaded_mainloop *mainloop() { return m_mainloop; } // this can be made public
+
+    void handle_server_info(const pa_server_info *info) // this can be made public
+    {
+        m_default_sink_name = info->default_sink_name;
+    }
+
+    void handle_sink_info(const pa_sink_info *info) // this can be made public
+    {
+        if (!m_old_active_port_name.empty())
+            internal_reset_old_active_sink_port(info);
+
+        if (m_set_preferred_sink_port)
+            internal_set_preferred_sink_port(info);
+    }
+
+    bool restore_sink_port() // this can be made public
+    {
+        /* If default sink port was changed, restore it */
+        if (m_context &&
+            m_set_preferred_sink_port &&
+            !m_old_active_port_name.empty())
+        {
+            m_set_preferred_sink_port = false;
+
+            pa_threaded_mainloop_lock(m_mainloop);
+
+            pa_operation *o;
+
+            /* Get default sink info */
+            o = pa_context_get_sink_info_by_name(m_context,
+                                                 m_default_sink_name.c_str(),
+                                                 get_sink_info_by_name_callback,
+                                                 this);
+            if (!handle_operation(o, "pa_context_get_sink_info_by_name"))
+                return false;
+
+            if (!m_old_active_port_name.empty())
+            {
+                g_debug("Restoring pulseaudio sink '%s' port to '%s'", m_default_sink_name.c_str(), m_old_active_port_name.c_str());
+
+                o = pa_context_set_sink_port_by_name(m_context,
+                                                     m_default_sink_name.c_str(),
+                                                     m_old_active_port_name.c_str(),
+                                                     success_callback, this);
+                if (!handle_operation(o, "pa_context_set_sink_port_by_name"))
+                    return false;
+
+                m_old_active_port_name = "";
+            }
+
+            pa_threaded_mainloop_unlock(m_mainloop);
+        }
+
+        return true;
+    }
+
+    bool create_context()
+    {
+        bool keepGoing = true;
+        bool connected = false;
+
+        if (m_context)
+            return true;
+
+        auto mainloop_api = pa_threaded_mainloop_get_api(m_mainloop);
+
+        pa_threaded_mainloop_lock(m_mainloop);
+
+        const auto name = "QtmPulseContext:" + std::to_string(::getpid());
+        m_context = pa_context_new(mainloop_api, name.c_str());
+        if (!m_context) {
+            g_critical("Unable to create new pulseaudio context");
+            pa_threaded_mainloop_unlock(m_mainloop);
+            return false;
+        }
+
+        pa_context_set_state_callback(m_context, set_state_callback, this);
+        if (pa_context_connect(m_context, NULL, PA_CONTEXT_NOFLAGS, NULL) < 0) {
+            g_critical("Unable to create a connection to the pulseaudio context");
+            pa_threaded_mainloop_unlock(m_mainloop);
+            release_context();
+            return false;
+        }
+
+        g_debug("Connecting to the pulseaudio context");
+        pa_threaded_mainloop_wait(m_mainloop);
+        while (keepGoing) {
+            switch (pa_context_get_state(m_context)) {
+                case PA_CONTEXT_CONNECTING:
+                case PA_CONTEXT_AUTHORIZING:
+                case PA_CONTEXT_SETTING_NAME:
+                    break;
+
+                case PA_CONTEXT_READY:
+                    g_debug("Pulseaudio connection established.");
+                    keepGoing = false;
+                    connected = true;
+                    break;
+
+                case PA_CONTEXT_TERMINATED:
+                    g_critical("Pulseaudio context terminated.");
+                    keepGoing = false;
+                    break;
+
+                case PA_CONTEXT_FAILED:
+                default:
+                    g_critical("Pulseaudio connection failure: %s", pa_strerror(pa_context_errno(m_context)));
+                    keepGoing = false;
+            }
+
+            if (keepGoing) {
+                pa_threaded_mainloop_wait(m_mainloop);
+            }
+        }
+
+        if (!connected) {
+            if (m_context) {
+                pa_context_unref(m_context);
+                m_context = nullptr;
+            }
+        }
+
+        pa_threaded_mainloop_unlock(m_mainloop);
+
+        return connected;
+    }
+
+    bool handle_operation(pa_operation *operation, const char *func_name)
+    {
+        if (!operation) {
+            g_critical("'%s' failed (lost pulseaudio connection?)", func_name);
+            /* Free resources so it can retry a new connection during next operation */
+            pa_threaded_mainloop_unlock(m_mainloop);
+            release_context();
+            return false;
+        }
+
+        while (pa_operation_get_state(operation) == PA_OPERATION_RUNNING)
+            pa_threaded_mainloop_wait(m_mainloop);
+
+        pa_operation_unref(operation);
+
+        return true;
+    }
+
+    void release_context()
+    {
+        if (m_context) {
+            pa_threaded_mainloop_lock(m_mainloop);
+            pa_context_disconnect(m_context);
+            pa_context_unref(m_context);
+            pa_threaded_mainloop_unlock(m_mainloop);
+            m_context = nullptr;
+        }
+    }
+
+    void internal_set_preferred_sink_port(const pa_sink_info *info)
+    {
+        /* Prefer speakers over headphones when playing audio (LP: #1364647) */
+        pa_sink_port_info* active_port = info->active_port;
+        pa_sink_port_info* speaker_port = nullptr;
+        pa_sink_port_info* speaker_wired_headphone_port = nullptr;
+
+        for (unsigned int i = 0; i < info->n_ports; i++)
+        {
+            if (info->ports[i]->available != PA_PORT_AVAILABLE_NO)
+            {
+                if (g_strcmp0(info->ports[i]->name, "output-speaker") == 0)
+                    speaker_port = info->ports[i];
+                else if (g_strcmp0(info->ports[i]->name, "output-speaker+wired_headphone") == 0)
+                    speaker_wired_headphone_port = info->ports[i];
+            }
+        }
+
+        pa_sink_port_info* preferred_port = nullptr;
+        if (speaker_wired_headphone_port != nullptr)
+            preferred_port = speaker_wired_headphone_port;
+        else
+            preferred_port = speaker_port;
+
+        if (preferred_port != nullptr &&
+            g_strcmp0(active_port->name, "output-speaker") != 0 &&
+            active_port != preferred_port)
+        {
+            m_old_active_port_name = active_port->name;
+            m_preferred_port_name = preferred_port->name;
+        }
+    }
+
+    void internal_reset_old_active_sink_port(const pa_sink_info *info)
+    {
+        /* Check if old active port is still available and inactive */
+        bool old_active_port_found = false;
+        for (unsigned int i = 0; i < info->n_ports; i++)
+        {
+            if (info->ports[i]->available != PA_PORT_AVAILABLE_NO)
+            {
+                if (g_strcmp0(info->ports[i]->name, m_old_active_port_name.c_str()) == 0)
+                    old_active_port_found = true;
+            }
+        }
+        if (!old_active_port_found ||
+            g_strcmp0(info->active_port->name, m_old_active_port_name.c_str()) == 0)
+        {
+            m_old_active_port_name = "";
+        }
+        else if (g_strcmp0(info->active_port->name, "output-speaker") != 0 &&
+                 g_strcmp0(info->active_port->name, "output-speaker+wired_headphone") != 0)
+        {
+            // looks like port changed while playing
+            m_old_active_port_name = "";
+        }
+    }
+
+    /***
+    ****
+    ***/
+
+    pa_threaded_mainloop *m_mainloop = nullptr;
+    pa_context *m_context = nullptr;
+    std::string m_default_sink_name = "";
+    std::string m_old_active_port_name = "";
+    std::string m_preferred_port_name = "";
+    bool m_set_preferred_sink_port = false;
+};
+
+/**
  * Plays a sound, possibly looping.
  */
 class Sound::Impl
@@ -73,6 +420,12 @@
 
     ~Impl()
     {
+        if (m_pulse_timeout != 0)
+            g_source_remove(m_pulse_timeout);
+
+        if (m_pulse != nullptr)
+            delete m_pulse;
+
         g_source_remove(m_watch_source);
 
         if (m_play != nullptr)
@@ -97,6 +450,20 @@
         return out_range_lo + (pct * (out_range_hi - out_range_lo));
     }
 
+    static gboolean pulse_timer_callback(gpointer gself)
+    {
+        auto self = static_cast<Impl*>(gself);
+
+        if (self->m_pulse == nullptr)
+            self->m_pulse = new PulseImpl();
+
+        /* Set preferred sink port */
+        self->m_pulse->set_preferred_sink_port();
+
+        self->m_pulse_timeout = 0;
+        return G_SOURCE_REMOVE;
+    }
+
     static gboolean bus_callback(GstBus*, GstMessage* msg, gpointer gself)
     {
         auto self = static_cast<Impl*>(gself);
@@ -115,7 +482,7 @@
         }
         else if (message_type == GST_MESSAGE_STREAM_START)
         {
-            /* Set the media role if audio sink is pulsesink */
+            /* Set the media role and pulse timer if audio sink is pulsesink */
             GstElement *audio_sink = nullptr;
             g_object_get(self->m_play, "audio-sink", &audio_sink, nullptr);
             if (audio_sink)
@@ -129,6 +496,11 @@
                     g_object_set(audio_sink, "stream-properties", props, nullptr);
                     gst_structure_free(props);
                     g_free(role_str);
+
+                    /* Set preferred sink port after 5 seconds */
+                    if (self->m_pulse == nullptr &&
+                        self->m_pulse_timeout == 0)
+                        self->m_pulse_timeout = g_timeout_add_seconds(5, pulse_timer_callback, self);
                 }
                 gst_object_unref(audio_sink);
             }
@@ -147,6 +519,8 @@
     const bool m_loop;
     guint m_watch_source = 0;
     GstElement* m_play = nullptr;
+    guint m_pulse_timeout = 0;
+    PulseImpl* m_pulse = nullptr;
 };
 
 Sound::Sound(const std::string& role, const std::string& uri, unsigned int volume, bool loop):