Media Hub

Merge lp:~justinmcp/media-hub/1603729 into lp:media-hub

  1. 1603729
  2. Merge into trunk
Proposed by Justin McPherson
Status: Approved
Approved by: Jim Hodapp
Approved revision: 208
Proposed branch: lp:~justinmcp/media-hub/1603729
Merge into: lp:media-hub
Diff against target: 206 lines (+36/-39)
5 files modified
src/core/media/engine.h (+1/-2)
src/core/media/gstreamer/engine.cpp (+2/-8)
src/core/media/gstreamer/engine.h (+1/-2)
src/core/media/player_implementation.cpp (+25/-20)
tests/unit-tests/test-gstreamer-engine.cpp (+7/-7)
To merge this branch: bzr merge lp:~justinmcp/media-hub/1603729
Reviewer Review Type Date Requested Status
Jim Hodapp (community) code Approve
Review via email: mp+303097@code.launchpad.net

Commit message

Fix use of custom headers when working with playlists.

Description of the change

Fix use of custom headers when working with playlists.

To post a comment you must log in.
Revision history for this message
Santosh (santoshbit2007) wrote :

I have just looked into code from coding prespective not other aspects. So I have one comment inline.
Its ok from my side if that is correct.

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

A couple of comments inline.

review: Needs Fixing (code)
Reply
lp:~justinmcp/media-hub/1603729 updated
206. By Justin McPherson

Fix typo.

Revision history for this message
Justin McPherson (justinmcp) :
Reply
Revision history for this message
Jim Hodapp (jhodapp) wrote :

Looks good to me. Let me know the silo number and I'll help test this out when it's ready. Thanks man.

review: Approve (code)
Reply
Revision history for this message
Jim Hodapp (jhodapp) wrote :

@Santosh, I'll let you top approve once you feel it's good.

Reply
Revision history for this message
David Barth (dbarth) wrote :

@jim: packages now in https://launchpad.net/~ci-train-ppa-service/+archive/ubuntu/landing-044/+packages

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

Thanks David. I tried to install this silo but it causes my turbo device to
not fully boot. It's most likely that media-hub isn't coming up quite right
which is usually the cause for Unity8 not starting which is what I'm seeing.

On Fri, Aug 19, 2016 at 10:37 AM, David Barth <email address hidden>
wrote:

> @jim: packages now in https://launchpad.net/~ci-
> train-ppa-service/+archive/ubuntu/landing-044/+packages
> --
> https://code.launchpad.net/~justinmcp/media-hub/1603729/+merge/303097
> You are reviewing the proposed merge of lp:~justinmcp/media-hub/1603729
> into lp:media-hub.
>

Reply
Revision history for this message
Justin McPherson (justinmcp) wrote :

I've tested this on the devices I have access to; a M10 and a N4. Both boot and work as expected. Both were using OTA-12. Can anyone confirm on other devices?

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

I'll test this on a freshly flashed krillin

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

This is consistent across both turbo and krillin for me. Both devices no longer boot into Unity8 after installing this silo.

review: Needs Fixing
Reply
Revision history for this message
Jim Hodapp (jhodapp) wrote :

This also needs to be re-synced against lp:media-hub as the latest version info has changed from other landings.

Reply
lp:~justinmcp/media-hub/1603729 updated
207. By Justin McPherson

Merge from trunk.

208. By Justin McPherson

Fix merge

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

LGTM

review: Approve (code)
Reply
lp:~justinmcp/media-hub/1603729 updated
209. By Justin McPherson

Merge from trunk.

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

What's the latest on this Justin? Do you plan to get this landed soon?

Reply
Revision history for this message
Justin McPherson (justinmcp) wrote :

There was a binary break on trunk, and Oxide needs to be pulled into the silo. I've been working on some urgent tasks, but I'll see if I can push this forward.

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

Sounds good Justin. Last I checked this was ready to simply hand over to QA to test for regressions and once they mark it as having passed, land it. Should be minimal work for you.

Reply
lp:~justinmcp/media-hub/1603729 updated
210. By Justin McPherson

Merge from trunk.

Unmerged revisions

210. By Justin McPherson

Merge from trunk.

209. By Justin McPherson

Merge from trunk.

208. By Justin McPherson

Fix merge

207. By Justin McPherson

Merge from trunk.

206. By Justin McPherson

Fix typo.

205. By Justin McPherson

Add an empty URI to avoid playlist playback when using custom headers

204. By Justin McPherson

Unify versions of open_resource_for_uri().

Create one code path so logic is easily applied to all.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'src/core/media/engine.h'
--- src/core/media/engine.h 2016-08-11 19:09:00 +0000
+++ src/core/media/engine.h 2016-11-07 23:52:53 +0000
@@ -76,8 +76,7 @@
7676
77 virtual const core::Property<State>& state() const = 0;77 virtual const core::Property<State>& state() const = 0;
7878
79 virtual bool open_resource_for_uri(const core::ubuntu::media::Track::UriType& uri, bool do_pipeline_reset) = 0;79 virtual bool open_resource_for_uri(const core::ubuntu::media::Track::UriType& uri, const Player::HeadersType&, bool do_pipeline_reset) = 0;
80 virtual bool open_resource_for_uri(const core::ubuntu::media::Track::UriType& uri, const Player::HeadersType&) = 0;
81 // Throws core::ubuntu::media::Player::Error::OutOfProcessBufferStreamingNotSupported if the implementation does not80 // Throws core::ubuntu::media::Player::Error::OutOfProcessBufferStreamingNotSupported if the implementation does not
82 // support this feature.81 // support this feature.
83 virtual void create_video_sink(uint32_t texture_id) = 0;82 virtual void create_video_sink(uint32_t texture_id) = 0;
8483
=== modified file 'src/core/media/gstreamer/engine.cpp'
--- src/core/media/gstreamer/engine.cpp 2016-08-23 08:26:11 +0000
+++ src/core/media/gstreamer/engine.cpp 2016-11-07 23:52:53 +0000
@@ -433,16 +433,10 @@
433}433}
434434
435bool gstreamer::Engine::open_resource_for_uri(const media::Track::UriType& uri,435bool gstreamer::Engine::open_resource_for_uri(const media::Track::UriType& uri,
436 const core::ubuntu::media::Player::HeadersType& headers,
436 bool do_pipeline_reset)437 bool do_pipeline_reset)
437{438{
438 d->playbin.set_uri(uri, core::ubuntu::media::Player::HeadersType{}, do_pipeline_reset);439 d->playbin.set_uri(uri, headers, do_pipeline_reset);
439 return true;
440}
441
442bool gstreamer::Engine::open_resource_for_uri(const media::Track::UriType& uri,
443 const core::ubuntu::media::Player::HeadersType& headers)
444{
445 d->playbin.set_uri(uri, headers);
446 return true;440 return true;
447}441}
448442
449443
=== modified file 'src/core/media/gstreamer/engine.h'
--- src/core/media/gstreamer/engine.h 2016-08-11 19:09:00 +0000
+++ src/core/media/gstreamer/engine.h 2016-11-07 23:52:53 +0000
@@ -33,8 +33,7 @@
3333
34 const core::Property<State>& state() const;34 const core::Property<State>& state() const;
3535
36 bool open_resource_for_uri(const core::ubuntu::media::Track::UriType& uri, bool do_pipeline_reset);36 bool open_resource_for_uri(const core::ubuntu::media::Track::UriType& uri, const core::ubuntu::media::Player::HeadersType& headers, bool do_pipeline_reset);
37 bool open_resource_for_uri(const core::ubuntu::media::Track::UriType& uri, const core::ubuntu::media::Player::HeadersType& headers);
38 void create_video_sink(uint32_t texture_id);37 void create_video_sink(uint32_t texture_id);
3938
40 // use_main_thread will set the pipeline's new state in the main thread context39 // use_main_thread will set the pipeline's new state in the main thread context
4140
=== modified file 'src/core/media/player_implementation.cpp'
--- src/core/media/player_implementation.cpp 2016-08-15 19:27:29 +0000
+++ src/core/media/player_implementation.cpp 2016-11-07 23:52:53 +0000
@@ -301,6 +301,26 @@
301 engine->reset();301 engine->reset();
302 }302 }
303303
304 bool open_uri(const Track::UriType& uri, const Player::HeadersType& headers = Player::HeadersType{})
305 {
306 track_list->reset();
307
308 // If empty uri, give the same meaning as QMediaPlayer::setMedia("")
309 if (uri.empty())
310 {
311 MH_DEBUG("Resetting current media");
312 return true;
313 }
314
315 static const bool do_pipeline_reset = false;
316 const bool ret = engine->open_resource_for_uri(uri, headers, do_pipeline_reset);
317 // Don't set new track as the current track to play since we're calling open_resource_for_uri above
318 static const bool make_current = false;
319 track_list->add_track_with_uri_at(headers.empty() ? uri : "", media::TrackList::after_empty_track(), make_current);
320
321 return ret;
322 }
323
304 void open_first_track_from_tracklist(const media::Track::Id& id)324 void open_first_track_from_tracklist(const media::Track::Id& id)
305 {325 {
306 const Track::UriType uri = track_list->query_uri_for_track(id);326 const Track::UriType uri = track_list->query_uri_for_track(id);
@@ -312,7 +332,7 @@
312 uri);332 uri);
313 MH_INFO("\twith a Track::Id: %s", id);333 MH_INFO("\twith a Track::Id: %s", id);
314 static const bool do_pipeline_reset = false;334 static const bool do_pipeline_reset = false;
315 engine->open_resource_for_uri(uri, do_pipeline_reset);335 engine->open_resource_for_uri(uri, Player::HeadersType{}, do_pipeline_reset);
316 }336 }
317 }337 }
318338
@@ -620,7 +640,7 @@
620 {640 {
621 MH_INFO("Advancing to next track on playbin: %s", uri);641 MH_INFO("Advancing to next track on playbin: %s", uri);
622 static const bool do_pipeline_reset = false;642 static const bool do_pipeline_reset = false;
623 d->engine->open_resource_for_uri(uri, do_pipeline_reset);643 d->engine->open_resource_for_uri(uri, Player::HeadersType{}, do_pipeline_reset);
624 }644 }
625645
626 d->doing_go_to_track.unlock();646 d->doing_go_to_track.unlock();
@@ -700,7 +720,7 @@
700 MH_INFO("Setting next track on playbin (on_go_to_track signal): %s", uri);720 MH_INFO("Setting next track on playbin (on_go_to_track signal): %s", uri);
701 MH_INFO("\twith a Track::Id: %s", id);721 MH_INFO("\twith a Track::Id: %s", id);
702 static const bool do_pipeline_reset = true;722 static const bool do_pipeline_reset = true;
703 d->engine->open_resource_for_uri(uri, do_pipeline_reset);723 d->engine->open_resource_for_uri(uri, Player::HeadersType{}, do_pipeline_reset);
704 }724 }
705725
706 if (auto_play)726 if (auto_play)
@@ -854,28 +874,13 @@
854template<typename Parent>874template<typename Parent>
855bool media::PlayerImplementation<Parent>::open_uri(const Track::UriType& uri)875bool media::PlayerImplementation<Parent>::open_uri(const Track::UriType& uri)
856{876{
857 d->track_list->reset();877 return d->open_uri(uri);
858
859 // If empty uri, give the same meaning as QMediaPlayer::setMedia("")
860 if (uri.empty())
861 {
862 MH_DEBUG("Resetting current media");
863 return true;
864 }
865
866 static const bool do_pipeline_reset = false;
867 const bool ret = d->engine->open_resource_for_uri(uri, do_pipeline_reset);
868 // Don't set new track as the current track to play since we're calling open_resource_for_uri above
869 static const bool make_current = false;
870 d->track_list->add_track_with_uri_at(uri, media::TrackList::after_empty_track(), make_current);
871
872 return ret;
873}878}
874879
875template<typename Parent>880template<typename Parent>
876bool media::PlayerImplementation<Parent>::open_uri(const Track::UriType& uri, const Player::HeadersType& headers)881bool media::PlayerImplementation<Parent>::open_uri(const Track::UriType& uri, const Player::HeadersType& headers)
877{882{
878 return d->engine->open_resource_for_uri(uri, headers);883 return d->open_uri(uri, headers);
879}884}
880885
881template<typename Parent>886template<typename Parent>
882887
=== modified file 'tests/unit-tests/test-gstreamer-engine.cpp'
--- tests/unit-tests/test-gstreamer-engine.cpp 2016-08-11 19:09:00 +0000
+++ tests/unit-tests/test-gstreamer-engine.cpp 2016-11-07 23:52:53 +0000
@@ -104,7 +104,7 @@
104 std::placeholders::_1));104 std::placeholders::_1));
105105
106 static const bool do_pipeline_reset = false;106 static const bool do_pipeline_reset = false;
107 EXPECT_TRUE(engine.open_resource_for_uri(test_file_uri, do_pipeline_reset));107 EXPECT_TRUE(engine.open_resource_for_uri(test_file_uri, core::ubuntu::media::Player::HeadersType{}, do_pipeline_reset));
108 static const bool use_main_context = true;108 static const bool use_main_context = true;
109 EXPECT_TRUE(engine.play(use_main_context));109 EXPECT_TRUE(engine.play(use_main_context));
110 EXPECT_TRUE(wst.wait_for_state_for(110 EXPECT_TRUE(wst.wait_for_state_for(
@@ -147,7 +147,7 @@
147 std::placeholders::_1));147 std::placeholders::_1));
148148
149 static const bool do_pipeline_reset = false;149 static const bool do_pipeline_reset = false;
150 EXPECT_TRUE(engine.open_resource_for_uri(test_file_uri, do_pipeline_reset));150 EXPECT_TRUE(engine.open_resource_for_uri(test_file_uri, core::ubuntu::media::Player::HeadersType{}, do_pipeline_reset));
151 static const bool use_main_context = true;151 static const bool use_main_context = true;
152 EXPECT_TRUE(engine.play(use_main_context));152 EXPECT_TRUE(engine.play(use_main_context));
153 EXPECT_TRUE(wst.wait_for_state_for(153 EXPECT_TRUE(wst.wait_for_state_for(
@@ -208,7 +208,7 @@
208 std::ref(wst),208 std::ref(wst),
209 std::placeholders::_1));209 std::placeholders::_1));
210210
211 EXPECT_TRUE(engine.open_resource_for_uri(test_audio_uri, headers));211 EXPECT_TRUE(engine.open_resource_for_uri(test_audio_uri, headers, false));
212 static const bool use_main_context = true;212 static const bool use_main_context = true;
213 EXPECT_TRUE(engine.play(use_main_context));213 EXPECT_TRUE(engine.play(use_main_context));
214 EXPECT_TRUE(wst.wait_for_state_for(214 EXPECT_TRUE(wst.wait_for_state_for(
@@ -240,7 +240,7 @@
240 std::placeholders::_1));240 std::placeholders::_1));
241241
242 static const bool do_pipeline_reset = true;242 static const bool do_pipeline_reset = true;
243 EXPECT_TRUE(engine.open_resource_for_uri(test_file_uri, do_pipeline_reset));243 EXPECT_TRUE(engine.open_resource_for_uri(test_file_uri, core::ubuntu::media::Player::HeadersType{}, do_pipeline_reset));
244 EXPECT_TRUE(engine.play());244 EXPECT_TRUE(engine.play());
245 EXPECT_TRUE(wst.wait_for_state_for(245 EXPECT_TRUE(wst.wait_for_state_for(
246 core::ubuntu::media::Engine::State::playing,246 core::ubuntu::media::Engine::State::playing,
@@ -289,7 +289,7 @@
289 std::placeholders::_1));289 std::placeholders::_1));
290290
291 static const bool do_pipeline_reset = true;291 static const bool do_pipeline_reset = true;
292 EXPECT_TRUE(engine.open_resource_for_uri(test_file_uri, do_pipeline_reset));292 EXPECT_TRUE(engine.open_resource_for_uri(test_file_uri, core::ubuntu::media::Player::HeadersType{}, do_pipeline_reset));
293 EXPECT_TRUE(engine.play());293 EXPECT_TRUE(engine.play());
294 EXPECT_TRUE(wst.wait_for_state_for(294 EXPECT_TRUE(wst.wait_for_state_for(
295 core::ubuntu::media::Engine::State::playing,295 core::ubuntu::media::Engine::State::playing,
@@ -336,7 +336,7 @@
336 std::placeholders::_1));336 std::placeholders::_1));
337337
338 static const bool do_pipeline_reset = true;338 static const bool do_pipeline_reset = true;
339 EXPECT_TRUE(engine.open_resource_for_uri(test_file_uri, do_pipeline_reset));339 EXPECT_TRUE(engine.open_resource_for_uri(test_file_uri, core::ubuntu::media::Player::HeadersType{}, do_pipeline_reset));
340 EXPECT_TRUE(engine.play());340 EXPECT_TRUE(engine.play());
341 EXPECT_TRUE(wst.wait_for_state_for(341 EXPECT_TRUE(wst.wait_for_state_for(
342 core::ubuntu::media::Engine::State::playing,342 core::ubuntu::media::Engine::State::playing,
@@ -372,7 +372,7 @@
372 std::ref(wst),372 std::ref(wst),
373 std::placeholders::_1));373 std::placeholders::_1));
374 static const bool do_pipeline_reset = true;374 static const bool do_pipeline_reset = true;
375 EXPECT_TRUE(engine.open_resource_for_uri(test_file_uri, do_pipeline_reset));375 EXPECT_TRUE(engine.open_resource_for_uri(test_file_uri, core::ubuntu::media::Player::HeadersType{}, do_pipeline_reset));
376 EXPECT_TRUE(engine.play());376 EXPECT_TRUE(engine.play());
377 EXPECT_TRUE(wst.wait_for_state_for(377 EXPECT_TRUE(wst.wait_for_state_for(
378 core::ubuntu::media::Engine::State::playing,378 core::ubuntu::media::Engine::State::playing,

Subscribers

People subscribed via source and target branches