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

[Justin McPherson]

Fix use of custom headers when working with playlists.

Fix use of custom headers when working with playlists.

[Santosh (santoshbit2007)]

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.

[Jim Hodapp (jhodapp)]

A couple of comments inline.

review: Needs Fixing (code)

[lp:~justinmcp/media-hub/1603729]

206. By Justin McPherson on 2016-08-17

Fix typo.

[Jim Hodapp (jhodapp)]

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)

[Jim Hodapp (jhodapp)]

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

[David Barth (dbarth)]

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

[Jim Hodapp (jhodapp)]

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.
>

[Justin McPherson (justinmcp)]

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?

[Jim Hodapp (jhodapp)]

I'll test this on a freshly flashed krillin

[Jim Hodapp (jhodapp)]

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

review: Needs Fixing

[Jim Hodapp (jhodapp)]

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

[lp:~justinmcp/media-hub/1603729]

207. By Justin McPherson on 2016-08-29

Merge from trunk.

208. By Justin McPherson on 2016-08-30

Fix merge

[Jim Hodapp (jhodapp)]

LGTM

review: Approve (code)

[lp:~justinmcp/media-hub/1603729]

209. By Justin McPherson on 2016-09-09

Merge from trunk.

[Jim Hodapp (jhodapp)]

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

[Justin McPherson (justinmcp)]

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.

[Jim Hodapp (jhodapp)]

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.

[lp:~justinmcp/media-hub/1603729]

210. By Justin McPherson on 2016-11-07

Merge from trunk.

Unmerged revisions

210. By Justin McPherson on 2016-11-07

Merge from trunk.

209. By Justin McPherson on 2016-09-09

Merge from trunk.

208. By Justin McPherson on 2016-08-30

Fix merge

207. By Justin McPherson on 2016-08-29

Merge from trunk.

206. By Justin McPherson on 2016-08-17

Fix typo.

205. By Justin McPherson on 2016-08-17

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

204. By Justin McPherson on 2016-08-17

Unify versions of open_resource_for_uri().

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

=== 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 @@
 
     virtual const core::Property<State>& state() const = 0;
 
-    virtual bool open_resource_for_uri(const core::ubuntu::media::Track::UriType& uri, bool do_pipeline_reset) = 0;
-    virtual bool open_resource_for_uri(const core::ubuntu::media::Track::UriType& uri, const Player::HeadersType&) = 0;
+    virtual bool open_resource_for_uri(const core::ubuntu::media::Track::UriType& uri, const Player::HeadersType&, bool do_pipeline_reset) = 0;
     // Throws core::ubuntu::media::Player::Error::OutOfProcessBufferStreamingNotSupported if the implementation does not
     // support this feature.
     virtual void create_video_sink(uint32_t texture_id) = 0;

=== 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 @@
 }
 
 bool gstreamer::Engine::open_resource_for_uri(const media::Track::UriType& uri,
+                                              const core::ubuntu::media::Player::HeadersType& headers,
                                               bool do_pipeline_reset)
 {
-    d->playbin.set_uri(uri, core::ubuntu::media::Player::HeadersType{}, do_pipeline_reset);
-    return true;
-}
-
-bool gstreamer::Engine::open_resource_for_uri(const media::Track::UriType& uri,
-                                              const core::ubuntu::media::Player::HeadersType& headers)
-{
-    d->playbin.set_uri(uri, headers);
+    d->playbin.set_uri(uri, headers, do_pipeline_reset);
     return true;
 }
 

=== 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 @@
 
     const core::Property<State>& state() const;
 
-    bool open_resource_for_uri(const core::ubuntu::media::Track::UriType& uri, bool do_pipeline_reset);
-    bool open_resource_for_uri(const core::ubuntu::media::Track::UriType& uri, const core::ubuntu::media::Player::HeadersType& headers);
+    bool open_resource_for_uri(const core::ubuntu::media::Track::UriType& uri, const core::ubuntu::media::Player::HeadersType& headers, bool do_pipeline_reset);
     void create_video_sink(uint32_t texture_id);
 
     // use_main_thread will set the pipeline's new state in the main thread context

=== 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 @@
         engine->reset();
     }
 
+    bool open_uri(const Track::UriType& uri, const Player::HeadersType& headers = Player::HeadersType{})
+    {
+        track_list->reset();
+
+        // If empty uri, give the same meaning as QMediaPlayer::setMedia("")
+        if (uri.empty())
+        {
+            MH_DEBUG("Resetting current media");
+            return true;
+        }
+
+        static const bool do_pipeline_reset = false;
+        const bool ret = engine->open_resource_for_uri(uri, headers, do_pipeline_reset);
+        // Don't set new track as the current track to play since we're calling open_resource_for_uri above
+        static const bool make_current = false;
+        track_list->add_track_with_uri_at(headers.empty() ? uri : "", media::TrackList::after_empty_track(), make_current);
+
+        return ret;
+    }
+
     void open_first_track_from_tracklist(const media::Track::Id& id)
     {
         const Track::UriType uri = track_list->query_uri_for_track(id);
@@ -312,7 +332,7 @@
                       uri);
             MH_INFO("\twith a Track::Id: %s", id);
             static const bool do_pipeline_reset = false;
-            engine->open_resource_for_uri(uri, do_pipeline_reset);
+            engine->open_resource_for_uri(uri, Player::HeadersType{}, do_pipeline_reset);
         }
     }
 
@@ -620,7 +640,7 @@
         {
             MH_INFO("Advancing to next track on playbin: %s", uri);
             static const bool do_pipeline_reset = false;
-            d->engine->open_resource_for_uri(uri, do_pipeline_reset);
+            d->engine->open_resource_for_uri(uri, Player::HeadersType{}, do_pipeline_reset);
         }
 
         d->doing_go_to_track.unlock();
@@ -700,7 +720,7 @@
             MH_INFO("Setting next track on playbin (on_go_to_track signal): %s", uri);
             MH_INFO("\twith a Track::Id: %s", id);
             static const bool do_pipeline_reset = true;
-            d->engine->open_resource_for_uri(uri, do_pipeline_reset);
+            d->engine->open_resource_for_uri(uri, Player::HeadersType{}, do_pipeline_reset);
         }
 
         if (auto_play)
@@ -854,28 +874,13 @@
 template<typename Parent>
 bool media::PlayerImplementation<Parent>::open_uri(const Track::UriType& uri)
 {
-    d->track_list->reset();
-
-    // If empty uri, give the same meaning as QMediaPlayer::setMedia("")
-    if (uri.empty())
-    {
-        MH_DEBUG("Resetting current media");
-        return true;
-    }
-
-    static const bool do_pipeline_reset = false;
-    const bool ret = d->engine->open_resource_for_uri(uri, do_pipeline_reset);
-    // Don't set new track as the current track to play since we're calling open_resource_for_uri above
-    static const bool make_current = false;
-    d->track_list->add_track_with_uri_at(uri, media::TrackList::after_empty_track(), make_current);
-
-    return ret;
+    return d->open_uri(uri);
 }
 
 template<typename Parent>
 bool media::PlayerImplementation<Parent>::open_uri(const Track::UriType& uri, const Player::HeadersType& headers)
 {
-    return d->engine->open_resource_for_uri(uri, headers);
+    return d->open_uri(uri, headers);
 }
 
 template<typename Parent>

=== 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 @@
                     std::placeholders::_1));
 
     static const bool do_pipeline_reset = false;
-    EXPECT_TRUE(engine.open_resource_for_uri(test_file_uri, do_pipeline_reset));
+    EXPECT_TRUE(engine.open_resource_for_uri(test_file_uri, core::ubuntu::media::Player::HeadersType{}, do_pipeline_reset));
     static const bool use_main_context = true;
     EXPECT_TRUE(engine.play(use_main_context));
     EXPECT_TRUE(wst.wait_for_state_for(
@@ -147,7 +147,7 @@
                     std::placeholders::_1));
 
     static const bool do_pipeline_reset = false;
-    EXPECT_TRUE(engine.open_resource_for_uri(test_file_uri, do_pipeline_reset));
+    EXPECT_TRUE(engine.open_resource_for_uri(test_file_uri, core::ubuntu::media::Player::HeadersType{}, do_pipeline_reset));
     static const bool use_main_context = true;
     EXPECT_TRUE(engine.play(use_main_context));
     EXPECT_TRUE(wst.wait_for_state_for(
@@ -208,7 +208,7 @@
                     std::ref(wst),
                     std::placeholders::_1));
 
-    EXPECT_TRUE(engine.open_resource_for_uri(test_audio_uri, headers));
+    EXPECT_TRUE(engine.open_resource_for_uri(test_audio_uri, headers, false));
     static const bool use_main_context = true;
     EXPECT_TRUE(engine.play(use_main_context));
     EXPECT_TRUE(wst.wait_for_state_for(
@@ -240,7 +240,7 @@
                     std::placeholders::_1));
 
     static const bool do_pipeline_reset = true;
-    EXPECT_TRUE(engine.open_resource_for_uri(test_file_uri, do_pipeline_reset));
+    EXPECT_TRUE(engine.open_resource_for_uri(test_file_uri, core::ubuntu::media::Player::HeadersType{}, do_pipeline_reset));
     EXPECT_TRUE(engine.play());
     EXPECT_TRUE(wst.wait_for_state_for(
                     core::ubuntu::media::Engine::State::playing,
@@ -289,7 +289,7 @@
                     std::placeholders::_1));
 
     static const bool do_pipeline_reset = true;
-    EXPECT_TRUE(engine.open_resource_for_uri(test_file_uri, do_pipeline_reset));
+    EXPECT_TRUE(engine.open_resource_for_uri(test_file_uri, core::ubuntu::media::Player::HeadersType{}, do_pipeline_reset));
     EXPECT_TRUE(engine.play());
     EXPECT_TRUE(wst.wait_for_state_for(
                     core::ubuntu::media::Engine::State::playing,
@@ -336,7 +336,7 @@
                     std::placeholders::_1));
 
     static const bool do_pipeline_reset = true;
-    EXPECT_TRUE(engine.open_resource_for_uri(test_file_uri, do_pipeline_reset));
+    EXPECT_TRUE(engine.open_resource_for_uri(test_file_uri, core::ubuntu::media::Player::HeadersType{}, do_pipeline_reset));
     EXPECT_TRUE(engine.play());
     EXPECT_TRUE(wst.wait_for_state_for(
                     core::ubuntu::media::Engine::State::playing,
@@ -372,7 +372,7 @@
                     std::ref(wst),
                     std::placeholders::_1));
     static const bool do_pipeline_reset = true;
-    EXPECT_TRUE(engine.open_resource_for_uri(test_file_uri, do_pipeline_reset));
+    EXPECT_TRUE(engine.open_resource_for_uri(test_file_uri, core::ubuntu::media::Player::HeadersType{}, do_pipeline_reset));
     EXPECT_TRUE(engine.play());
     EXPECT_TRUE(wst.wait_for_state_for(
                     core::ubuntu::media::Engine::State::playing,