Merge lp:~phablet-team/media-hub/fix-1596329 into lp:media-hub

[Jim Hodapp]

To avoid deadlocking, make sure that we don't ever change the pipeline's state on the streaming thread. It will always be set from the main thread context thanks to g_idle_add(). The deadlock that this fixes was when calling stop() in player_implementation.cpp in the on_end_of_tracklist() handler.

To avoid deadlocking, make sure that we don't ever change the pipeline's state on the streaming thread. It will always be set from the main thread context thanks to g_idle_add(). The deadlock that this fixes was when calling stop() in player_implementation.cpp in the on_end_of_tracklist() handler.

[Scott Sweeny (ssweeny)]

LGTM

review: Approve

[lp:~phablet-team/media-hub/fix-1596329]

197. By Jim Hodapp on 2016-06-30

Disable specific fragile unit test. Many of these tests need rewriting.

198. By Jim Hodapp on 2016-06-30

Only use the main thread context set_state_and_wait for stop(). Also re-enable the disabled unit test.

199. By Jim Hodapp on 2016-07-01

Further refine the fix so that the unit tests pass and we still don't get a deadlock

200. By Jim Hodapp on 2016-07-01

Don't break tracklist support

201. By Jim Hodapp on 2016-07-04

Patch up a memory leak

=== modified file 'src/core/media/engine.h'
--- src/core/media/engine.h	2015-04-03 22:52:08 +0000
+++ src/core/media/engine.h	2016-07-04 15:03:57 +0000
@@ -83,7 +83,7 @@
     virtual void create_video_sink(uint32_t texture_id) = 0;
 
     virtual bool play() = 0;
-    virtual bool stop()  = 0;
+    virtual bool stop(bool use_main_context = false)  = 0;
     virtual bool pause() = 0;
     virtual bool seek_to(const std::chrono::microseconds& ts) = 0;
 
 
=== modified file 'src/core/media/gstreamer/engine.cpp'
--- src/core/media/gstreamer/engine.cpp	2016-06-08 16:54:31 +0000
+++ src/core/media/gstreamer/engine.cpp	2016-07-04 15:03:57 +0000
@@ -446,7 +446,7 @@
     return result;
 }
 
-bool gstreamer::Engine::stop()
+bool gstreamer::Engine::stop(bool use_main_thread)
 {
     // No need to wait, and we can immediately return.
     if (d->state == media::Engine::State::stopped)
@@ -455,7 +455,7 @@
         return true;
     }
 
-    const auto result = d->playbin.set_state_and_wait(GST_STATE_NULL);
+    const auto result = d->playbin.set_state_and_wait(GST_STATE_NULL, use_main_thread);
     if (result)
     {
         d->state = media::Engine::State::stopped;
 
=== modified file 'src/core/media/gstreamer/engine.h'
--- src/core/media/gstreamer/engine.h	2015-04-03 22:52:08 +0000
+++ src/core/media/gstreamer/engine.h	2016-07-04 15:03:57 +0000
@@ -38,7 +38,8 @@
     void create_video_sink(uint32_t texture_id);
 
     bool play();
-    bool stop();
+    // use_main_thread will set the pipeline's new_state in the main thread context
+    bool stop(bool use_main_thread = false);
     bool pause();
     bool seek_to(const std::chrono::microseconds& ts);
 
 
=== modified file 'src/core/media/gstreamer/playbin.cpp'
--- src/core/media/gstreamer/playbin.cpp	2016-06-16 22:42:30 +0000
+++ src/core/media/gstreamer/playbin.cpp	2016-07-04 15:03:57 +0000
@@ -121,7 +121,8 @@
       is_missing_audio_codec(false),
       is_missing_video_codec(false),
       audio_stream_id(-1),
-      video_stream_id(-1)
+      video_stream_id(-1),
+      current_new_state(GST_STATE_NULL)
 {
     if (!pipeline)
         throw std::runtime_error("Could not create pipeline for playbin.");
@@ -524,7 +525,17 @@
     return result;
 }
 
-bool gstreamer::Playbin::set_state_and_wait(GstState new_state)
+gboolean gstreamer::Playbin::set_state_in_main_thread(gpointer user_data)
+{
+    MH_TRACE("");
+    auto thiz = static_cast<Playbin*>(user_data);
+    if (thiz and thiz->pipeline)
+        gst_element_set_state(thiz->pipeline, thiz->current_new_state);
+
+    // Always return false so this is a single shot function call
+    return false;
+}
+bool gstreamer::Playbin::set_state_and_wait(GstState new_state, bool use_main_thread)
 {
     static const std::chrono::nanoseconds state_change_timeout
     {
@@ -534,25 +545,44 @@
         std::chrono::milliseconds{5000}
     };
 
-    auto ret = gst_element_set_state(pipeline, new_state);
-
-    MH_DEBUG("Requested state change.");
-
-    bool result = false; GstState current, pending;
-    switch(ret)
+    bool result = false;
+    GstState current, pending;
+    if (use_main_thread)
     {
-    case GST_STATE_CHANGE_FAILURE:
-        result = false; break;
-    case GST_STATE_CHANGE_NO_PREROLL:
-    case GST_STATE_CHANGE_SUCCESS:
-        result = true; break;
-    case GST_STATE_CHANGE_ASYNC:
+        // Cache this value for the static g_idle_add handler function
+        current_new_state = new_state;
+        g_idle_add((GSourceFunc) gstreamer::Playbin::set_state_in_main_thread, (gpointer) this);
+
+        MH_DEBUG("Requested state change in main thread context.");
+
+        GstState current, pending;
         result = GST_STATE_CHANGE_SUCCESS == gst_element_get_state(
-                    pipeline,
-                    &current,
-                    &pending,
-                    state_change_timeout.count());
-        break;
+                pipeline,
+                &current,
+                &pending,
+                state_change_timeout.count());
+    }
+    else
+    {
+        const auto ret = gst_element_set_state(pipeline, new_state);
+
+        MH_DEBUG("Requested state change not using main thread context.");
+
+        switch (ret)
+        {
+            case GST_STATE_CHANGE_FAILURE:
+                result = false; break;
+            case GST_STATE_CHANGE_NO_PREROLL:
+            case GST_STATE_CHANGE_SUCCESS:
+                result = true; break;
+            case GST_STATE_CHANGE_ASYNC:
+                result = GST_STATE_CHANGE_SUCCESS == gst_element_get_state(
+                        pipeline,
+                        &current,
+                        &pending,
+                        state_change_timeout.count());
+                break;
+        }
     }
 
     // We only should query the pipeline if we actually succeeded in
 
=== modified file 'src/core/media/gstreamer/playbin.h'
--- src/core/media/gstreamer/playbin.h	2016-02-19 16:14:42 +0000
+++ src/core/media/gstreamer/playbin.h	2016-07-04 15:03:57 +0000
@@ -96,9 +96,12 @@
 
     void setup_source(GstElement *source);
 
-    // Sets the pipeline's state (stopped, playing, paused, etc). Optional parameter makes this call
-    // in the main_loop context.
-    bool set_state_and_wait(GstState new_state);
+    // Sets the pipeline state in the main thread context instead of the possibility of creating
+    // a deadlock in the streaming thread
+    static gboolean set_state_in_main_thread(gpointer user_data);
+    // Sets the pipeline's state (stopped, playing, paused, etc). use_main_thread will set the
+    // pipeline's new_state in the main thread context.
+    bool set_state_and_wait(GstState new_state, bool use_main_thread = false);
     bool seek(const std::chrono::microseconds& ms);
 
     core::ubuntu::media::video::Dimensions get_video_dimensions() const;
@@ -148,6 +151,7 @@
     bool is_missing_video_codec;
     gint audio_stream_id;
     gint video_stream_id;
+    GstState current_new_state;
 };
 }
 
 
=== modified file 'src/core/media/metadata.cpp'
--- src/core/media/metadata.cpp	2016-06-15 17:49:49 +0000
+++ src/core/media/metadata.cpp	2016-07-04 15:03:57 +0000
@@ -29,9 +29,19 @@
     if (not is_set(key))
         return std::string{};
 
-    return std::string{g_uri_escape_string(map.at(key).c_str(),
-                                            "!$&'()*+,;=:/?[]@", // Reserved chars
-                                            true)};              // Allow UTF-8 chars
+    char* escaped {g_uri_escape_string(map.at(key).c_str(),
+                    "!$&'()*+,;=:/?[]@", // Reserved chars
+                    true)};
+    if (!escaped)
+    {
+        return std::string{};
+    }
+    else
+    {
+        std::string s{escaped};
+        g_free(escaped);
+        return s;
+    }
 }
 
 const std::string& media::Track::MetaData::album() const
 
=== modified file 'src/core/media/player_implementation.cpp'
--- src/core/media/player_implementation.cpp	2016-06-23 00:17:16 +0000
+++ src/core/media/player_implementation.cpp	2016-07-04 15:03:57 +0000
@@ -665,7 +665,8 @@
                 && d->engine->state() != gstreamer::Engine::State::stopped)
         {
             MH_INFO("End of tracklist reached, stopping playback");
-            d->engine->stop();
+            const constexpr bool use_main_thread = true;
+            d->engine->stop(use_main_thread);
         }
     });