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

Proposed by Jim Hodapp
Status: Merged
Approved by: Nick Dedekind
Approved revision: 147
Merged at revision: 145
Proposed branch: lp:~phablet-team/media-hub/fix-1458040
Merge into: lp:media-hub
Prerequisite: lp:~phablet-team/media-hub/fix-1457129
Diff against target: 318 lines (+121/-36)
5 files modified
src/core/media/gstreamer/bus.h (+41/-10)
src/core/media/gstreamer/engine.cpp (+56/-12)
src/core/media/gstreamer/playbin.cpp (+15/-12)
src/core/media/gstreamer/playbin.h (+4/-1)
tests/unit-tests/test-gstreamer-engine.cpp (+5/-1)
To merge this branch: bzr merge lp:~phablet-team/media-hub/fix-1458040
Reviewer Review Type Date Requested Status
Nick Dedekind (community) Approve
PS Jenkins bot continuous-integration Approve
Review via email: mp+260507@code.launchpad.net

Commit message

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

Description of the change

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

To post a comment you must log in.
Revision history for this message
Nick Dedekind (nick-dedekind) wrote :

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?

Revision history for this message
Nick Dedekind (nick-dedekind) wrote :

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.

Revision history for this message
Jim Hodapp (jhodapp) wrote :

> 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

144. By Jim Hodapp

Log any playbin errors to stderr

145. By Jim Hodapp

Merged with trunk

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
146. By Jim Hodapp

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.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
147. By Jim Hodapp

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.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Nick Dedekind (nick-dedekind) wrote :

Looks good and does as said on tin.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
277277
278 auto thiz = static_cast<Bus*>(data);278 auto thiz = static_cast<Bus*>(data);
279 Message message(msg);279 Message message(msg);
280 thiz->on_new_message(message);280 if (message.type == GST_MESSAGE_TAG || message.type == GST_MESSAGE_ASYNC_DONE)
281281 thiz->on_new_message(message);
282 return GST_BUS_DROP;282
283 }283 return GST_BUS_PASS;
284284 }
285 Bus(GstBus* bus) : bus(bus)285
286 static gboolean bus_watch_handler(
287 GstBus* bus,
288 GstMessage* msg,
289 gpointer data)
290 {
291 (void) bus;
292
293 auto thiz = static_cast<Bus*>(data);
294 Message message(msg);
295 thiz->on_new_message_async(message);
296
297 return true;
298 }
299
300 Bus(GstBus* bus) : bus(bus), bus_watch_id(0)
301 {
302 set_bus(bus);
303 }
304
305 ~Bus()
306 {
307 g_source_remove(bus_watch_id);
308 gst_object_unref(bus);
309 }
310
311 void set_bus(GstBus* bus)
286 {312 {
287 if (!bus)313 if (!bus)
288 throw std::runtime_error("Cannot create Bus instance if underlying instance is NULL.");314 throw std::runtime_error("Cannot create Bus instance if underlying instance is NULL.");
@@ -292,15 +318,20 @@
292 Bus::sync_handler,318 Bus::sync_handler,
293 this,319 this,
294 nullptr);320 nullptr);
295 }
296321
297 ~Bus()322 // Use a watch for most messages instead of the sync handler so that our context is not
298 {323 // the same as the streaming thread, which can cause deadlocks in GStreamer
299 gst_object_unref(bus);324 // if, for example, attempting to change the pipeline state from this context.
325 bus_watch_id = gst_bus_add_watch(
326 bus,
327 Bus::bus_watch_handler,
328 this);
300 }329 }
301330
302 GstBus* bus;331 GstBus* bus;
303 core::Signal<Message> on_new_message;332 core::Signal<Message> on_new_message;
333 core::Signal<Message> on_new_message_async;
334 guint bus_watch_id;
304};335};
305}336}
306337
307338
=== 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 @@
75 // Converts from a GStreamer GError to a media::Player:Error enum75 // Converts from a GStreamer GError to a media::Player:Error enum
76 media::Player::Error from_gst_errorwarning(const gstreamer::Bus::Message::Detail::ErrorWarningInfo& ewi)76 media::Player::Error from_gst_errorwarning(const gstreamer::Bus::Message::Detail::ErrorWarningInfo& ewi)
77 {77 {
78 media::Player::Error ret_error = media::Player::Error::no_error;
79
78 if (g_strcmp0(g_quark_to_string(ewi.error->domain), "gst-core-error-quark") == 0)80 if (g_strcmp0(g_quark_to_string(ewi.error->domain), "gst-core-error-quark") == 0)
79 {81 {
80 switch (ewi.error->code)82 switch (ewi.error->code)
81 {83 {
84 case GST_CORE_ERROR_FAILED:
85 std::cerr << "** Encountered a GST_CORE_ERROR_FAILED" << std::endl;
86 ret_error = media::Player::Error::resource_error;
87 break;
82 case GST_CORE_ERROR_NEGOTIATION:88 case GST_CORE_ERROR_NEGOTIATION:
83 return media::Player::Error::resource_error;89 std::cerr << "** Encountered a GST_CORE_ERROR_NEGOTIATION" << std::endl;
90 ret_error = media::Player::Error::resource_error;
91 break;
84 case GST_CORE_ERROR_MISSING_PLUGIN:92 case GST_CORE_ERROR_MISSING_PLUGIN:
85 return media::Player::Error::format_error;93 std::cerr << "** Encountered a GST_CORE_ERROR_MISSING_PLUGIN" << std::endl;
94 ret_error = media::Player::Error::format_error;
95 break;
86 default:96 default:
87 std::cerr << "Got an unhandled core error: '"97 std::cerr << "** Encountered an unhandled core error: '"
88 << ewi.debug << "' (code: " << ewi.error->code << ")" << std::endl;98 << ewi.debug << "' (code: " << ewi.error->code << ")" << std::endl;
89 return media::Player::Error::no_error;99 ret_error = media::Player::Error::no_error;
100 break;
90 }101 }
91 }102 }
92 else if (g_strcmp0(g_quark_to_string(ewi.error->domain), "gst-resource-error-quark") == 0)103 else if (g_strcmp0(g_quark_to_string(ewi.error->domain), "gst-resource-error-quark") == 0)
93 {104 {
94 switch (ewi.error->code)105 switch (ewi.error->code)
95 {106 {
107 case GST_RESOURCE_ERROR_FAILED:
108 std::cerr << "** Encountered a GST_RESOURCE_ERROR_FAILED" << std::endl;
109 ret_error = media::Player::Error::resource_error;
110 break;
96 case GST_RESOURCE_ERROR_NOT_FOUND:111 case GST_RESOURCE_ERROR_NOT_FOUND:
112 std::cerr << "** Encountered a GST_RESOURCE_ERROR_NOT_FOUND" << std::endl;
113 ret_error = media::Player::Error::resource_error;
114 break;
97 case GST_RESOURCE_ERROR_OPEN_READ:115 case GST_RESOURCE_ERROR_OPEN_READ:
116 std::cerr << "** Encountered a GST_RESOURCE_ERROR_OPEN_READ" << std::endl;
117 ret_error = media::Player::Error::resource_error;
118 break;
98 case GST_RESOURCE_ERROR_OPEN_WRITE:119 case GST_RESOURCE_ERROR_OPEN_WRITE:
120 std::cerr << "** Encountered a GST_RESOURCE_ERROR_OPEN_WRITE" << std::endl;
121 ret_error = media::Player::Error::resource_error;
122 break;
99 case GST_RESOURCE_ERROR_READ:123 case GST_RESOURCE_ERROR_READ:
124 std::cerr << "** Encountered a GST_RESOURCE_ERROR_READ" << std::endl;
125 ret_error = media::Player::Error::resource_error;
126 break;
100 case GST_RESOURCE_ERROR_WRITE:127 case GST_RESOURCE_ERROR_WRITE:
101 return media::Player::Error::resource_error;128 std::cerr << "** Encountered a GST_RESOURCE_ERROR_WRITE" << std::endl;
129 ret_error = media::Player::Error::resource_error;
130 break;
102 case GST_RESOURCE_ERROR_NOT_AUTHORIZED:131 case GST_RESOURCE_ERROR_NOT_AUTHORIZED:
103 return media::Player::Error::access_denied_error;132 std::cerr << "** Encountered a GST_RESOURCE_ERROR_NOT_AUTHORIZED" << std::endl;
133 ret_error = media::Player::Error::access_denied_error;
134 break;
104 default:135 default:
105 std::cerr << "Got an unhandled resource error: '"136 std::cerr << "** Encountered an unhandled resource error: '"
106 << ewi.debug << "' (code: " << ewi.error->code << ")" << std::endl;137 << ewi.debug << "' (code: " << ewi.error->code << ")" << std::endl;
107 return media::Player::Error::no_error;138 ret_error = media::Player::Error::no_error;
139 break;
108 }140 }
109 }141 }
110 else if (g_strcmp0(g_quark_to_string(ewi.error->domain), "gst-stream-error-quark") == 0)142 else if (g_strcmp0(g_quark_to_string(ewi.error->domain), "gst-stream-error-quark") == 0)
111 {143 {
112 switch (ewi.error->code)144 switch (ewi.error->code)
113 {145 {
146 case GST_STREAM_ERROR_FAILED:
147 std::cerr << "** Encountered a GST_STREAM_ERROR_FAILED" << std::endl;
148 ret_error = media::Player::Error::resource_error;
149 break;
114 case GST_STREAM_ERROR_CODEC_NOT_FOUND:150 case GST_STREAM_ERROR_CODEC_NOT_FOUND:
151 std::cerr << "** Encountered a GST_STREAM_ERROR_CODEC_NOT_FOUND" << std::endl;
152 ret_error = media::Player::Error::format_error;
153 break;
115 case GST_STREAM_ERROR_DECODE:154 case GST_STREAM_ERROR_DECODE:
116 return media::Player::Error::format_error;155 std::cerr << "** Encountered a GST_STREAM_ERROR_DECODE" << std::endl;
156 ret_error = media::Player::Error::format_error;
157 break;
117 default:158 default:
118 std::cerr << "Got an unhandled stream error: '"159 std::cerr << "** Encountered an unhandled stream error: '"
119 << ewi.debug << "' (code: " << ewi.error->code << ")" << std::endl;160 << ewi.debug << "' (code: " << ewi.error->code << ")" << std::endl;
120 return media::Player::Error::no_error;161 ret_error = media::Player::Error::no_error;
162 break;
121 }163 }
122 }164 }
123165
124 return media::Player::Error::no_error;166 std::cout << "Resetting playbin pipeline after unrecoverable error" << std::endl;
167 playbin.reset();
168 return ret_error;
125 }169 }
126170
127 void on_playbin_error(const gstreamer::Bus::Message::Detail::ErrorWarningInfo& ewi)171 void on_playbin_error(const gstreamer::Bus::Message::Detail::ErrorWarningInfo& ewi)
128172
=== 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 @@
92 bus{gst_element_get_bus(pipeline)},92 bus{gst_element_get_bus(pipeline)},
93 file_type(MEDIA_FILE_TYPE_NONE),93 file_type(MEDIA_FILE_TYPE_NONE),
94 video_sink(nullptr),94 video_sink(nullptr),
95 on_new_message_connection(95 on_new_message_connection_async(
96 bus.on_new_message.connect(96 bus.on_new_message_async.connect(
97 std::bind(97 std::bind(
98 &Playbin::on_new_message,98 &Playbin::on_new_message_async,
99 this,99 this,
100 std::placeholders::_1))),100 std::placeholders::_1))),
101 is_seeking(false),101 is_seeking(false),
102 previous_position(0),102 previous_position(0),
103 player_lifetime(media::Player::Lifetime::normal),103 player_lifetime(media::Player::Lifetime::normal),
104 is_eos(false)104 is_eos(false),
105 about_to_finish_handler_id(0),
106 source_setup_handler_id(0)
105{107{
106 if (!pipeline)108 if (!pipeline)
107 throw std::runtime_error("Could not create pipeline for playbin.");109 throw std::runtime_error("Could not create pipeline for playbin.");
108110
109 is_eos = false;
110
111 // Add audio and/or video sink elements depending on environment variables111 // Add audio and/or video sink elements depending on environment variables
112 // being set or not set112 // being set or not set
113 setup_pipeline_for_audio_video();113 setup_pipeline_for_audio_video();
114114
115 g_signal_connect(115 about_to_finish_handler_id = g_signal_connect(
116 pipeline,116 pipeline,
117 "about-to-finish",117 "about-to-finish",
118 G_CALLBACK(about_to_finish),118 G_CALLBACK(about_to_finish),
119 this119 this
120 );120 );
121121
122 g_signal_connect(122 source_setup_handler_id = g_signal_connect(
123 pipeline,123 pipeline,
124 "source-setup",124 "source-setup",
125 G_CALLBACK(source_setup),125 G_CALLBACK(source_setup),
@@ -129,6 +129,9 @@
129129
130gstreamer::Playbin::~Playbin()130gstreamer::Playbin::~Playbin()
131{131{
132 g_signal_handler_disconnect(pipeline, about_to_finish_handler_id);
133 g_signal_handler_disconnect(pipeline, source_setup_handler_id);
134
132 if (pipeline)135 if (pipeline)
133 gst_object_unref(pipeline);136 gst_object_unref(pipeline);
134}137}
@@ -167,7 +170,7 @@
167 file_type = MEDIA_FILE_TYPE_NONE;170 file_type = MEDIA_FILE_TYPE_NONE;
168}171}
169172
170void gstreamer::Playbin::on_new_message(const Bus::Message& message)173void gstreamer::Playbin::on_new_message_async(const Bus::Message& message)
171{174{
172 switch(message.type)175 switch(message.type)
173 {176 {
@@ -180,6 +183,9 @@
180 case GST_MESSAGE_INFO:183 case GST_MESSAGE_INFO:
181 signals.on_info(message.detail.error_warning_info);184 signals.on_info(message.detail.error_warning_info);
182 break;185 break;
186 case GST_MESSAGE_STATE_CHANGED:
187 signals.on_state_changed(std::make_pair(message.detail.state_changed, message.source));
188 break;
183 case GST_MESSAGE_TAG:189 case GST_MESSAGE_TAG:
184 {190 {
185 gchar *orientation;191 gchar *orientation;
@@ -193,9 +199,6 @@
193 signals.on_tag_available(message.detail.tag);199 signals.on_tag_available(message.detail.tag);
194 }200 }
195 break;201 break;
196 case GST_MESSAGE_STATE_CHANGED:
197 signals.on_state_changed(std::make_pair(message.detail.state_changed, message.source));
198 break;
199 case GST_MESSAGE_ASYNC_DONE:202 case GST_MESSAGE_ASYNC_DONE:
200 if (is_seeking)203 if (is_seeking)
201 {204 {
202205
=== 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 @@
71 void reset_pipeline();71 void reset_pipeline();
7272
73 void on_new_message(const Bus::Message& message);73 void on_new_message(const Bus::Message& message);
74 void on_new_message_async(const Bus::Message& message);
7475
75 gstreamer::Bus& message_bus();76 gstreamer::Bus& message_bus();
7677
@@ -112,12 +113,14 @@
112 gstreamer::Bus bus;113 gstreamer::Bus bus;
113 MediaFileType file_type;114 MediaFileType file_type;
114 GstElement* video_sink;115 GstElement* video_sink;
115 core::Connection on_new_message_connection;116 core::Connection on_new_message_connection_async;
116 bool is_seeking;117 bool is_seeking;
117 mutable uint64_t previous_position;118 mutable uint64_t previous_position;
118 core::ubuntu::media::Player::HeadersType request_headers;119 core::ubuntu::media::Player::HeadersType request_headers;
119 core::ubuntu::media::Player::Lifetime player_lifetime;120 core::ubuntu::media::Player::Lifetime player_lifetime;
120 bool is_eos;121 bool is_eos;
122 gulong about_to_finish_handler_id;
123 gulong source_setup_handler_id;
121 struct124 struct
122 {125 {
123 core::Signal<void> about_to_finish;126 core::Signal<void> about_to_finish;
124127
=== 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 @@
411 ASSERT_TRUE(test::copy_test_media_file_to("test.mp3", test_file));411 ASSERT_TRUE(test::copy_test_media_file_to("test.mp3", test_file));
412412
413 gstreamer::Engine engine;413 gstreamer::Engine engine;
414 auto md = engine.meta_data_extractor()->meta_data_for_track_with_uri(test_file_uri);414
415 core::ubuntu::media::Track::MetaData md;
416 ASSERT_NO_THROW({
417 md = engine.meta_data_extractor()->meta_data_for_track_with_uri(test_file_uri);
418 });
415419
416 if (0 < md.count(xesam::Album::name))420 if (0 < md.count(xesam::Album::name))
417 EXPECT_EQ("Test", md.get(xesam::Album::name));421 EXPECT_EQ("Test", md.get(xesam::Album::name));

Subscribers

People subscribed via source and target branches

to all changes: