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

[Jim Hodapp]

Handle a wider array of GStreamer errors so that .ogv files are reported as failing to play.

Handle a wider array of GStreamer errors so that .ogv files are reported as failing to play.

[Nick Dedekind (nick-dedekind)]

Feels a bit strange that we return "no_error" if there is an error. Should we not add a "unknown_error" to the Player error api?
This seems likely to be the reason why this failed in the first place?

[Nick Dedekind (nick-dedekind)]

Still seems to be stalling in the dash usage, but not in the mediaplayer-app. Not sure why though; checking the differences but either way it shouldn't be happening.

[Jim Hodapp (jhodapp)]

> Feels a bit strange that we return "no_error" if there is an error. Should we
> not add a "unknown_error" to the Player error api?
> This seems likely to be the reason why this failed in the first place?

My limitation is the types of errors supported by QMediaPlayer: http://doc.qt.io/qt-5/qmediaplayer.html#Error-enum

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

144. By Jim Hodapp on 2015-05-29

Log any playbin errors to stderr

145. By Jim Hodapp on 2015-05-29

Merged with trunk

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:145
http://jenkins.qa.ubuntu.com/job/media-hub-ci/358/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-amd64-ci/198
    SUCCESS: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-armhf-ci/198
        deb: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-armhf-ci/198/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-i386-ci/198

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/media-hub-ci/358/rebuild

review: Approve (continuous-integration)

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

146. By Jim Hodapp on 2015-06-01

Don't use a sync_handler for GST_MESSAGE handling. Use an async one instead so that we aren't on the streaming thread's context. This prevents the pipeline from deadlocking after an error is reached.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:146
http://jenkins.qa.ubuntu.com/job/media-hub-ci/359/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-amd64-ci/199/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-armhf-ci/199/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-i386-ci/199/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/media-hub-ci/359/rebuild

review: Needs Fixing (continuous-integration)

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

147. By Jim Hodapp on 2015-06-01

Use both the sync_handler and async bus watch to handle GST_MESSAGES, but only using the sync one where absolutely necessary since it can cause deadlocks when used in general.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:147
http://jenkins.qa.ubuntu.com/job/media-hub-ci/360/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-amd64-ci/200
    SUCCESS: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-armhf-ci/200
        deb: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-armhf-ci/200/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-i386-ci/200

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/media-hub-ci/360/rebuild

review: Approve (continuous-integration)

[Nick Dedekind (nick-dedekind)]

Looks good and does as said on tin.

review: Approve

=== modified file 'src/core/media/gstreamer/bus.h'
--- src/core/media/gstreamer/bus.h	2014-10-23 14:42:59 +0000
+++ src/core/media/gstreamer/bus.h	2015-06-01 21:35:00 +0000
@@ -277,12 +277,38 @@
 
         auto thiz = static_cast<Bus*>(data);
         Message message(msg);
-        thiz->on_new_message(message);
-
-        return GST_BUS_DROP;
-    }
-
-    Bus(GstBus* bus) : bus(bus)
+        if (message.type == GST_MESSAGE_TAG || message.type == GST_MESSAGE_ASYNC_DONE)
+            thiz->on_new_message(message);
+
+        return GST_BUS_PASS;
+    }
+
+    static gboolean bus_watch_handler(
+            GstBus* bus,
+            GstMessage* msg,
+            gpointer data)
+    {
+        (void) bus;
+
+        auto thiz = static_cast<Bus*>(data);
+        Message message(msg);
+        thiz->on_new_message_async(message);
+
+        return true;
+    }
+
+    Bus(GstBus* bus) : bus(bus), bus_watch_id(0)
+    {
+        set_bus(bus);
+    }
+
+    ~Bus()
+    {
+        g_source_remove(bus_watch_id);
+        gst_object_unref(bus);
+    }
+
+    void set_bus(GstBus* bus)
     {
         if (!bus)
             throw std::runtime_error("Cannot create Bus instance if underlying instance is NULL.");
@@ -292,15 +318,20 @@
                     Bus::sync_handler,
                     this,
                     nullptr);
-    }
 
-    ~Bus()
-    {
-        gst_object_unref(bus);
+        // Use a watch for most messages instead of the sync handler so that our context is not
+        // the same as the streaming thread, which can cause deadlocks in GStreamer
+        // if, for example, attempting to change the pipeline state from this context.
+        bus_watch_id = gst_bus_add_watch(
+                            bus,
+                            Bus::bus_watch_handler,
+                            this);
     }
 
     GstBus* bus;
     core::Signal<Message> on_new_message;
+    core::Signal<Message> on_new_message_async;
+    guint bus_watch_id;
 };
 }
 

=== modified file 'src/core/media/gstreamer/engine.cpp'
--- src/core/media/gstreamer/engine.cpp	2015-05-01 19:44:11 +0000
+++ src/core/media/gstreamer/engine.cpp	2015-06-01 21:35:00 +0000
@@ -75,53 +75,97 @@
     // Converts from a GStreamer GError to a media::Player:Error enum
     media::Player::Error from_gst_errorwarning(const gstreamer::Bus::Message::Detail::ErrorWarningInfo& ewi)
     {
+        media::Player::Error ret_error = media::Player::Error::no_error;
+
         if (g_strcmp0(g_quark_to_string(ewi.error->domain), "gst-core-error-quark") == 0)
         {
             switch (ewi.error->code)
             {
+            case GST_CORE_ERROR_FAILED:
+                std::cerr << "** Encountered a GST_CORE_ERROR_FAILED" << std::endl;
+                ret_error = media::Player::Error::resource_error;
+                break;
             case GST_CORE_ERROR_NEGOTIATION:
-                return media::Player::Error::resource_error;
+                std::cerr << "** Encountered a GST_CORE_ERROR_NEGOTIATION" << std::endl;
+                ret_error = media::Player::Error::resource_error;
+                break;
             case GST_CORE_ERROR_MISSING_PLUGIN:
-                return media::Player::Error::format_error;
+                std::cerr << "** Encountered a GST_CORE_ERROR_MISSING_PLUGIN" << std::endl;
+                ret_error = media::Player::Error::format_error;
+                break;
             default:
-                std::cerr << "Got an unhandled core error: '"
+                std::cerr << "** Encountered an unhandled core error: '"
                     << ewi.debug << "' (code: " << ewi.error->code << ")" << std::endl;
-                return media::Player::Error::no_error;
+                ret_error = media::Player::Error::no_error;
+                break;
             }
         }
         else if (g_strcmp0(g_quark_to_string(ewi.error->domain), "gst-resource-error-quark") == 0)
         {
             switch (ewi.error->code)
             {
+            case GST_RESOURCE_ERROR_FAILED:
+                std::cerr << "** Encountered a GST_RESOURCE_ERROR_FAILED" << std::endl;
+                ret_error = media::Player::Error::resource_error;
+                break;
             case GST_RESOURCE_ERROR_NOT_FOUND:
+                std::cerr << "** Encountered a GST_RESOURCE_ERROR_NOT_FOUND" << std::endl;
+                ret_error = media::Player::Error::resource_error;
+                break;
             case GST_RESOURCE_ERROR_OPEN_READ:
+                std::cerr << "** Encountered a GST_RESOURCE_ERROR_OPEN_READ" << std::endl;
+                ret_error = media::Player::Error::resource_error;
+                break;
             case GST_RESOURCE_ERROR_OPEN_WRITE:
+                std::cerr << "** Encountered a GST_RESOURCE_ERROR_OPEN_WRITE" << std::endl;
+                ret_error = media::Player::Error::resource_error;
+                break;
             case GST_RESOURCE_ERROR_READ:
+                std::cerr << "** Encountered a GST_RESOURCE_ERROR_READ" << std::endl;
+                ret_error = media::Player::Error::resource_error;
+                break;
             case GST_RESOURCE_ERROR_WRITE:
-                return media::Player::Error::resource_error;
+                std::cerr << "** Encountered a GST_RESOURCE_ERROR_WRITE" << std::endl;
+                ret_error = media::Player::Error::resource_error;
+                break;
             case GST_RESOURCE_ERROR_NOT_AUTHORIZED:
-                return media::Player::Error::access_denied_error;
+                std::cerr << "** Encountered a GST_RESOURCE_ERROR_NOT_AUTHORIZED" << std::endl;
+                ret_error = media::Player::Error::access_denied_error;
+                break;
             default:
-                std::cerr << "Got an unhandled resource error: '"
+                std::cerr << "** Encountered an unhandled resource error: '"
                     << ewi.debug << "' (code: " << ewi.error->code << ")" << std::endl;
-                return media::Player::Error::no_error;
+                ret_error = media::Player::Error::no_error;
+                break;
             }
         }
         else if (g_strcmp0(g_quark_to_string(ewi.error->domain), "gst-stream-error-quark") == 0)
         {
             switch (ewi.error->code)
             {
+            case GST_STREAM_ERROR_FAILED:
+                std::cerr << "** Encountered a GST_STREAM_ERROR_FAILED" << std::endl;
+                ret_error = media::Player::Error::resource_error;
+                break;
             case GST_STREAM_ERROR_CODEC_NOT_FOUND:
+                std::cerr << "** Encountered a GST_STREAM_ERROR_CODEC_NOT_FOUND" << std::endl;
+                ret_error = media::Player::Error::format_error;
+                break;
             case GST_STREAM_ERROR_DECODE:
-                return media::Player::Error::format_error;
+                std::cerr << "** Encountered a GST_STREAM_ERROR_DECODE" << std::endl;
+                ret_error = media::Player::Error::format_error;
+                break;
             default:
-                std::cerr << "Got an unhandled stream error: '"
+                std::cerr << "** Encountered an unhandled stream error: '"
                     << ewi.debug << "' (code: " << ewi.error->code << ")" << std::endl;
-                return media::Player::Error::no_error;
+                ret_error = media::Player::Error::no_error;
+                break;
             }
         }
 
-        return media::Player::Error::no_error;
+        std::cout << "Resetting playbin pipeline after unrecoverable error" << std::endl;
+        playbin.reset();
+        return ret_error;
     }
 
     void on_playbin_error(const gstreamer::Bus::Message::Detail::ErrorWarningInfo& ewi)

=== modified file 'src/core/media/gstreamer/playbin.cpp'
--- src/core/media/gstreamer/playbin.cpp	2015-05-01 19:44:11 +0000
+++ src/core/media/gstreamer/playbin.cpp	2015-06-01 21:35:00 +0000
@@ -92,34 +92,34 @@
       bus{gst_element_get_bus(pipeline)},
       file_type(MEDIA_FILE_TYPE_NONE),
       video_sink(nullptr),
-      on_new_message_connection(
-          bus.on_new_message.connect(
+      on_new_message_connection_async(
+          bus.on_new_message_async.connect(
               std::bind(
-                  &Playbin::on_new_message,
+                  &Playbin::on_new_message_async,
                   this,
                   std::placeholders::_1))),
       is_seeking(false),
       previous_position(0),
       player_lifetime(media::Player::Lifetime::normal),
-      is_eos(false)
+      is_eos(false),
+      about_to_finish_handler_id(0),
+      source_setup_handler_id(0)
 {
     if (!pipeline)
         throw std::runtime_error("Could not create pipeline for playbin.");
 
-    is_eos = false;
-
     // Add audio and/or video sink elements depending on environment variables
     // being set or not set
     setup_pipeline_for_audio_video();
 
-    g_signal_connect(
+    about_to_finish_handler_id = g_signal_connect(
                 pipeline,
                 "about-to-finish",
                 G_CALLBACK(about_to_finish),
                 this
                 );
 
-    g_signal_connect(
+    source_setup_handler_id = g_signal_connect(
         pipeline,
         "source-setup",
         G_CALLBACK(source_setup),
@@ -129,6 +129,9 @@
 
 gstreamer::Playbin::~Playbin()
 {
+    g_signal_handler_disconnect(pipeline, about_to_finish_handler_id);
+    g_signal_handler_disconnect(pipeline, source_setup_handler_id);
+
     if (pipeline)
         gst_object_unref(pipeline);
 }
@@ -167,7 +170,7 @@
     file_type = MEDIA_FILE_TYPE_NONE;
 }
 
-void gstreamer::Playbin::on_new_message(const Bus::Message& message)
+void gstreamer::Playbin::on_new_message_async(const Bus::Message& message)
 {
     switch(message.type)
     {
@@ -180,6 +183,9 @@
     case GST_MESSAGE_INFO:
         signals.on_info(message.detail.error_warning_info);
         break;
+    case GST_MESSAGE_STATE_CHANGED:
+        signals.on_state_changed(std::make_pair(message.detail.state_changed, message.source));
+        break;
     case GST_MESSAGE_TAG:
         {
             gchar *orientation;
@@ -193,9 +199,6 @@
             signals.on_tag_available(message.detail.tag);
         }
         break;
-    case GST_MESSAGE_STATE_CHANGED:
-        signals.on_state_changed(std::make_pair(message.detail.state_changed, message.source));
-        break;
     case GST_MESSAGE_ASYNC_DONE:
         if (is_seeking)
         {

=== modified file 'src/core/media/gstreamer/playbin.h'
--- src/core/media/gstreamer/playbin.h	2015-05-01 19:44:11 +0000
+++ src/core/media/gstreamer/playbin.h	2015-06-01 21:35:00 +0000
@@ -71,6 +71,7 @@
     void reset_pipeline();
 
     void on_new_message(const Bus::Message& message);
+    void on_new_message_async(const Bus::Message& message);
 
     gstreamer::Bus& message_bus();
 
@@ -112,12 +113,14 @@
     gstreamer::Bus bus;
     MediaFileType file_type;
     GstElement* video_sink;
-    core::Connection on_new_message_connection;
+    core::Connection on_new_message_connection_async;
     bool is_seeking;
     mutable uint64_t previous_position;
     core::ubuntu::media::Player::HeadersType request_headers;
     core::ubuntu::media::Player::Lifetime player_lifetime;
     bool is_eos;
+    gulong about_to_finish_handler_id;
+    gulong source_setup_handler_id;
     struct
     {
         core::Signal<void> about_to_finish;

=== modified file 'tests/unit-tests/test-gstreamer-engine.cpp'
--- tests/unit-tests/test-gstreamer-engine.cpp	2015-04-03 22:52:08 +0000
+++ tests/unit-tests/test-gstreamer-engine.cpp	2015-06-01 21:35:00 +0000
@@ -411,7 +411,11 @@
     ASSERT_TRUE(test::copy_test_media_file_to("test.mp3", test_file));
 
     gstreamer::Engine engine;
-    auto md = engine.meta_data_extractor()->meta_data_for_track_with_uri(test_file_uri);
+
+    core::ubuntu::media::Track::MetaData md;
+    ASSERT_NO_THROW({
+        md = engine.meta_data_extractor()->meta_data_for_track_with_uri(test_file_uri);
+    });
 
     if (0 < md.count(xesam::Album::name))
         EXPECT_EQ("Test", md.get(xesam::Album::name));