Merge lp:~phablet-team/qtubuntu-media/enable-mpris-controls into lp:qtubuntu-media/stable

[Jim Hodapp]

Make sure the that the current player instance is controlable by MPRIS controls if the type of player is appropriate for playlist control. Also, don't add a track to the tracklist when AalMediaPlayerService::setMedia() is called.

Make sure the that the current player instance is controlable by MPRIS controls if the type of player is appropriate for playlist control. Also, don't add a track to the tracklist when AalMediaPlayerService::setMedia() is called.

[Alfonso Sanchez-Beato (alfonsosanchezbeato)]

Looks good, some minor issues detected. I would also like to have a branch for trunk too.

review: Needs Fixing

[Jim Hodapp (jhodapp)]

Comments addresses.

[Alfonso Sanchez-Beato (alfonsosanchezbeato)]

review: Approve

[lp:~phablet-team/qtubuntu-media/enable-mpris-controls]

86. By Alfonso Sanchez-Beato on 2015-10-07

Make sure the that the current player instance is controlable by MPRIS controls
if the type of player is appropriate for playlist control. Also, don't add a
track to the tracklist when AalMediaPlayerService::setMedia() is called.

87. By Alfonso Sanchez-Beato on 2015-10-07

Do not detach from media-hub when we are inactive, as that is needed only
when the application terminates.

88. By Alfonso Sanchez-Beato on 2015-10-08

Fix and add integration tests

89. By Jim Hodapp on 2015-10-14

Make sure the current player is set when the client app regains focus.

=== added file 'README'
--- README	1970-01-01 00:00:00 +0000
+++ README	2015-10-14 20:56:01 +0000
@@ -0,0 +1,4 @@
+Coding Convention:
+------------------
+
+This project follows the Qt coding style as detailed here: https://wiki.qt.io/Qt_Coding_Style
 
=== modified file 'debian/control'
--- debian/control	2015-08-04 01:56:17 +0000
+++ debian/control	2015-10-14 20:56:01 +0000
@@ -9,7 +9,7 @@
                libgl1-mesa-dev | libgl-dev,
                libgles2-mesa-dev,
                libhybris-dev (>= 0.1.0+git20131207+e452e83-0ubuntu13),
-               libmedia-hub-dev (>= 3.1.0),
+               libmedia-hub-dev (>= 3.2.0),
                libpulse-dev,
                libqt5opengl5-dev,
                libqtubuntu-media-signals-dev (>= 0.3+15.04.20150618.1-0ubuntu1),
 
=== modified file 'src/aal/aalmediaplayerservice.cpp'
--- src/aal/aalmediaplayerservice.cpp	2015-08-18 15:49:35 +0000
+++ src/aal/aalmediaplayerservice.cpp	2015-10-14 20:56:01 +0000
@@ -35,7 +35,10 @@
 
 #include <qtubuntu_media_signals.h>
 
-// Uncomment to re-enable media-hub Player session detach/reattach functionality
+// Uncomment to re-enable media-hub Player session detach/reattach functionality.
+// It is not clear at all whether we should do this or not, as detaching
+// is probably something that should be done when the app finishes, not when it
+// simply moves to the background.
 //#define DO_PLAYER_ATTACH_DETACH
 
 // Defined in aalvideorenderercontrol.h
@@ -86,7 +89,7 @@
 
     // As core::Connection doesn't allow us to start with a disconnected connection
     // instance we have to connect it first with a dummy signal and then disconnect
-    // it again. If we don't do this connect_signals() will never be able to attach
+    // it again. If we don't do this connectSignals() will never be able to attach
     // to the relevant signals.
     m_endOfStreamConnection.disconnect();
 
@@ -271,7 +274,7 @@
 
 void AalMediaPlayerService::setMedia(const QUrl &url)
 {
-    if (m_hubPlayerSession == NULL)
+    if (m_hubPlayerSession == nullptr)
     {
         qWarning() << "Cannot open uri without a valid media-hub player session";
         return;
@@ -292,16 +295,19 @@
 
     qDebug() << "Setting media to: " << url;
     const media::Track::UriType uri(url.url().toStdString());
-    try {
-        if (m_mediaPlaylistProvider != NULL)
-            m_mediaPlaylistProvider->addMedia(QMediaContent(url));
-        else
+    if (m_mediaPlaylistProvider == nullptr)
+    {
+        try {
             m_hubPlayerSession->open_uri(uri);
-    }
-    catch (const std::runtime_error &e) {
-        qWarning() << "Failed to open media " << url << ": " << e.what();
-        return;
-    }
+        }
+        catch (const std::runtime_error &e) {
+            qWarning() << "Failed to set media " << url << ": " << e.what();
+            return;
+        }
+    }
+
+    // Make sure this player can be controlled by MPRIS if appropriate
+    updateCurrentPlayer();
 
     if (isVideoSource())
         m_videoOutput->setupSurface();
@@ -327,13 +333,18 @@
 
     qDebug() << "Setting media to: " << AalUtility::unescape(media);
     try {
+        // TODO: Change this to use the QUrl from QMediaContent and then call
+        // open_uri(uri) like the other version of setMedia does above
         m_mediaPlaylistProvider->addMedia(media);
     }
     catch (const std::runtime_error &e) {
-        qWarning() << "Failed to open media " << AalUtility::unescape(media) << ": " << e.what();
+        qWarning() << "Failed to set media " << AalUtility::unescape(media) << ": " << e.what();
         return;
     }
 
+    // Make sure this player can be controlled by MPRIS if appropriate
+    updateCurrentPlayer();
+
     if (isVideoSource())
         m_videoOutput->setupSurface();
 }
@@ -546,7 +557,7 @@
         return;
 
     m_mediaPlayerControl = new AalMediaPlayerControl(this);
-    connect_signals();
+    connectSignals();
 }
 
 void AalMediaPlayerService::createVideoRendererControl()
@@ -704,7 +715,7 @@
             case Qt::ApplicationInactive:
                 qDebug() << "** Application is now inactive";
 #ifdef DO_PLAYER_ATTACH_DETACH
-                disconnect_signals();
+                disconnectSignals();
                 m_hubService->detach_session(m_sessionUuid, media::Player::Client::default_configuration());
                 m_doReattachSession = true;
 #endif
@@ -716,13 +727,16 @@
                 // will break video playback
                 if (m_doReattachSession)
                 {
-                    m_hubPlayerSession = m_hubService->reattach_session(m_sessionUuid, media::Player::Client::default_configuration());
-                    // Make sure the client's status (position, duraiton, state, etc) are all correct when reattaching
-                    // to the media-hub Player session
+                    m_hubPlayerSession = m_hubService->
+                        reattach_session(m_sessionUuid,
+                                         media::Player::Client::default_configuration());
+                    // Make sure the client's status (position, duraiton, state, etc) are all
+                    // correct when reattaching to the media-hub Player session
                     updateClientSignals();
-                    connect_signals();
+                    connectSignals();
                 }
 #endif
+                updateCurrentPlayer();
                 break;
             default:
                 qDebug() << "Unknown ApplicationState";
@@ -758,7 +772,7 @@
     }
 }
 
-void AalMediaPlayerService::connect_signals()
+void AalMediaPlayerService::connectSignals()
 {
     if (!m_endOfStreamConnection.is_connected())
     {
@@ -770,12 +784,29 @@
     }
 }
 
-void AalMediaPlayerService::disconnect_signals()
+void AalMediaPlayerService::disconnectSignals()
 {
     if (m_endOfStreamConnection.is_connected())
         m_endOfStreamConnection.disconnect();
 }
 
+void AalMediaPlayerService::updateCurrentPlayer()
+{
+        // If this player is a multimedia audioRole, then it should possible to
+        // use it for MPRIS control
+    if (audioRole() == QMediaPlayer::MultimediaRole)
+    {
+        qDebug() << "Setting player as current player";
+        try {
+            const media::Player::PlayerKey key = m_hubPlayerSession->key();
+            m_hubService->set_current_player(key);
+        } catch (const std::runtime_error &e) {
+            qWarning() << "Failed to set player as current player: " << e.what();
+            return;
+        }
+    }
+}
+
 void AalMediaPlayerService::onError(const core::ubuntu::media::Player::Error &error)
 {
     qWarning() << "** Media playback error: " << error;
 
=== modified file 'src/aal/aalmediaplayerservice.h'
--- src/aal/aalmediaplayerservice.h	2015-08-18 15:48:31 +0000
+++ src/aal/aalmediaplayerservice.h	2015-10-14 20:56:01 +0000
@@ -114,8 +114,9 @@
 
 protected:
     void updateClientSignals();
-    void connect_signals();
-    void disconnect_signals();
+    void connectSignals();
+    void disconnectSignals();
+    void updateCurrentPlayer();
 #ifdef MEASURE_PERFORMANCE
     void measurePerformance();
 #endif
@@ -153,6 +154,7 @@
     const QMediaPlaylist* m_mediaPlaylist;
 
     core::ubuntu::media::Player::PlaybackStatus m_newStatus;
+
     std::string m_sessionUuid;
     bool m_doReattachSession;
 
 
=== modified file 'tests/integration/tst_mediaplaylist.cpp'
--- tests/integration/tst_mediaplaylist.cpp	2015-07-10 14:49:05 +0000
+++ tests/integration/tst_mediaplaylist.cpp	2015-10-14 20:56:01 +0000
@@ -215,23 +215,11 @@
 
     QCOMPARE(playlist->mediaCount(), 3);
 
-    QMediaContent current_media;
-    std::promise<QMediaContent> promise;
-    std::future<QMediaContent> future{promise.get_future()};
-    QMetaObject::Connection c = connect(playlist, &QMediaPlaylist::currentMediaChanged, [&](const QMediaContent& content)
-    {
-            qDebug() << "currentMediaChanged to: " << content.canonicalUrl().toString();
-            current_media = content;
-            promise.set_value(current_media);
-            // Make sure the promise is not fulfilled twice
-            QObject::disconnect(c);
-    });
-
     qDebug() << "Setting current index to be 1";
     playlist->setCurrentIndex(1);
 
     // Wait for the currentMediaChanged signal to be emited
-    wait_for_signal(future);
+    waitTrackChange(playlist);
 
     qDebug() << "Checking if current index is 1";
     QCOMPARE(playlist->currentIndex(), 1);
@@ -312,30 +300,16 @@
 
     QCOMPARE(playlist->mediaCount(), 2);
 
-    QMediaContent current_media;
-    std::promise<QMediaContent> promise;
-    std::future<QMediaContent> future{promise.get_future()};
-    QMetaObject::Connection c = connect(playlist, &QMediaPlaylist::currentMediaChanged, [&](const QMediaContent& content)
-    {
-            qDebug() << "currentMediaChanged to: " << content.canonicalUrl().toString();
-            current_media = content;
-            promise.set_value(current_media);
-            // Make sure the promise is not fulfilled twice
-            QObject::disconnect(c);
-    });
-
     playlist->setPlaybackMode(QMediaPlaylist::CurrentItemInLoop);
 
     qDebug() << "Call player->play()";
     player->play();
 
     // Wait for the currentMediaChanged signal to be emited
-    wait_for_signal(future);
+    waitTrackChange(playlist);
 
     QCOMPARE(playlist->currentIndex(), 0);
 
-    QObject::disconnect(c);
-
     delete playlist;
     delete player;
 }
@@ -346,19 +320,6 @@
     QMediaPlaylist *playlist = new QMediaPlaylist;
     player->setPlaylist(playlist);
 
-    QMediaContent current_media;
-    std::promise<QMediaContent> promise;
-    std::future<QMediaContent> future{promise.get_future()};
-
-    QMetaObject::Connection c = connect(playlist, &QMediaPlaylist::currentMediaChanged, [&](const QMediaContent& content)
-    {
-            qDebug() << "currentMediaChanged to: " << content.canonicalUrl().toString();
-            current_media = content;
-            promise.set_value(current_media);
-            // Make sure the promise is not fulfilled twice
-            QObject::disconnect(c);
-    });
-
     QList<QMediaContent> content;
     content.push_back(QUrl("file://" + QFINDTESTDATA("testdata/testfile.ogg")));
     content.push_back(QUrl("file://" + QFINDTESTDATA("testdata/testfile.ogg")));
@@ -368,14 +329,48 @@
 
     playlist->setPlaybackMode(QMediaPlaylist::Sequential);
 
+    // Wait until the first track is set as the current one
+    waitTrackChange(playlist);
+
     player->play();
 
-    // Wait for the currentMediaChanged signal to be emited
-    wait_for_signal(future);
+    // Wait until the second track is selected
+    waitTrackChange(playlist);
 
     QCOMPARE(playlist->currentIndex(), 1);
 
-    QObject::disconnect(c);
+    delete playlist;
+    delete player;
+}
+
+void tst_MediaPlaylist::playReusePlayTrackList()
+{
+    QMediaPlayer *player = new QMediaPlayer;
+    QMediaPlaylist *playlist = new QMediaPlaylist;
+    player->setPlaylist(playlist);
+
+    const QUrl audio(QUrl("file://" + QFINDTESTDATA("testdata/testfile.ogg")));
+    const QUrl video(QUrl("file://" + QFINDTESTDATA("testdata/testfile.mp4")));
+
+    for (int i = 0; i < 5; ++i) {
+        playlist->addMedia(audio);
+        playlist->addMedia(video);
+        playlist->addMedia(audio);
+        QCOMPARE(playlist->mediaCount(), 3);
+
+        player->play();
+        QCoreApplication::processEvents();
+
+        const QUrl audioToVerify(playlist->currentMedia().canonicalUrl());
+        QCOMPARE(audioToVerify, audio);
+
+        player->stop();
+        QCoreApplication::processEvents();
+
+        playlist->clear();
+
+        QCOMPARE(playlist->mediaCount(), 0);
+    }
 
     delete playlist;
     delete player;
@@ -393,4 +388,23 @@
     }
 }
 
+void tst_MediaPlaylist::waitTrackChange(QMediaPlaylist *playlist)
+{
+    QMediaContent current_media;
+    std::promise<QMediaContent> promise;
+    std::future<QMediaContent> future{promise.get_future()};
+
+    QMetaObject::Connection c = connect(playlist, &QMediaPlaylist::currentMediaChanged,
+        [&](const QMediaContent& content)
+        {
+            qDebug() << "currentMediaChanged to: " << content.canonicalUrl().toString();
+            current_media = content;
+            promise.set_value(current_media);
+            // Make sure the promise is not fulfilled twice
+            QObject::disconnect(c);
+        });
+
+    wait_for_signal(future);
+}
+
 QTEST_GUILESS_MAIN(tst_MediaPlaylist)
 
=== modified file 'tests/integration/tst_mediaplaylist.h'
--- tests/integration/tst_mediaplaylist.h	2015-07-10 14:49:05 +0000
+++ tests/integration/tst_mediaplaylist.h	2015-10-14 20:56:01 +0000
@@ -63,6 +63,8 @@
     void verifyPlaybackModeCurrentItemInLoop();
     void verifyPlaybackModeSequential();
 
+    void playReusePlayTrackList();
+
 private:
     template<typename R>
         bool is_ready(std::future<R> const& f)
@@ -70,6 +72,8 @@
 
     template<typename R>
         void wait_for_signal(std::future<R> const& f);
+
+    void waitTrackChange(QMediaPlaylist *playlist);
 };
 
 #endif // TST_MEDIAPLAYLIST_H
 
=== modified file 'tests/unit/service.cpp'
--- tests/unit/service.cpp	2015-06-08 19:15:43 +0000
+++ tests/unit/service.cpp	2015-10-14 20:56:01 +0000
@@ -54,6 +54,10 @@
     return 0;
 }
 
+void TestService::set_current_player(Player::PlayerKey)
+{
+}
+
 void TestService::pause_other_sessions(Player::PlayerKey)
 {
 }
 
=== modified file 'tests/unit/service.h'
--- tests/unit/service.h	2015-06-08 19:15:43 +0000
+++ tests/unit/service.h	2015-10-14 20:56:01 +0000
@@ -50,6 +50,7 @@
     virtual void destroy_session(const std::string& uuid, const Player::Configuration&);
     virtual std::shared_ptr<Player> create_fixed_session(const std::string& name, const Player::Configuration&);
     virtual std::shared_ptr<Player> resume_session(Player::PlayerKey);
+    virtual void set_current_player(Player::PlayerKey key);
     virtual void pause_other_sessions(Player::PlayerKey);
 };
 }