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

[Konrad Zapałowicz]

A rewrite of how the current player is set which is what the MPRIS control interface actively uses.

A rewrite of how the current player is set which is what the MPRIS control interface actively uses.

[Alfonso Sanchez-Beato (alfonsosanchezbeato)]

Looks good, but maybe we need to call update_current_player() also when adding complete track lists? Which is done with AddTrack(s) DBus calls.

review: Needs Information

[Jim Hodapp (jhodapp)]

> Looks good, but maybe we need to call update_current_player() also when adding
> complete track lists? Which is done with AddTrack(s) DBus calls.

Can you explain more why you think we need to do this? I don't see any gap that this would fix.

[Jim Hodapp (jhodapp)]

Addressed review comments.

[Alfonso Sanchez-Beato (alfonsosanchezbeato)]

LGTM

review: Approve

[Alfonso Sanchez-Beato (alfonsosanchezbeato)]

So I have just seen that Jim modified service.h too to add reset_current_player(), for some reason I did not notice last time I reviewed. TBH I do not like adding reset_current_player() and get_current_player() to service.h. That file is, in the end, defining a DBus interface so we should add only functions that make sense to a media-hub client, and these functions are not even implemented in the service stub (client library).

I think that changing PlayerSkeleton so player_service is a pointer to ServiceSkeleton, and implementing these functions only in ServiceSkeleton should work. We will make sure the DBus interface is not contaminated and we remove the need for dummy implementations in ServiceStub and ServiceImplementation.

@Konrad maybe we con wait for Jim if you do not feel comfortable with this change.

Finally, I appreciate the long description in the last commit message, but I think most of it should move to a comment inside the code so it is more easily found.

The change looks good beside this issue with service.h.

review: Needs Fixing

[Konrad Zapałowicz (kzapalowicz)]

Thanks for the review and I will try to make the changes that you suggest. Either I'm successful or not and the Jim will take over when he is back however before that happens I will get my hands dirty :)

[Alfonso Sanchez-Beato (alfonsosanchezbeato)]

LGTM

review: Approve

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

186. By Jim Hodapp on 2016-04-20

Pass a Service* instead of ServiceSkeleton* to avoid weird inheritance segfault

Unmerged revisions

186. By Jim Hodapp on 2016-04-20

Pass a Service* instead of ServiceSkeleton* to avoid weird inheritance segfault

=== modified file 'debian/libmedia-hub-doc.install'
--- debian/libmedia-hub-doc.install	2014-03-06 22:51:51 +0000
+++ debian/libmedia-hub-doc.install	2016-04-20 15:59:04 +0000
@@ -1,1 +1,1 @@
-usr/share/doc/music-hub/html
+usr/share/doc/media-hub/html

=== modified file 'doc/CMakeLists.txt'
--- doc/CMakeLists.txt	2013-08-14 18:05:23 +0000
+++ doc/CMakeLists.txt	2016-04-20 15:59:04 +0000
@@ -26,5 +26,5 @@
     COMMENT "Generating API documentation with Doxygen" VERBATIM)
   install(
     DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/html
-    DESTINATION share/doc/music-hub)
-endif(DOXYGEN_FOUND)
\ No newline at end of file
+    DESTINATION share/doc/media-hub)
+endif(DOXYGEN_FOUND)

=== modified file 'include/core/media/service.h'
--- include/core/media/service.h	2015-09-08 20:03:56 +0000
+++ include/core/media/service.h	2016-04-20 15:59:04 +0000
@@ -60,9 +60,6 @@
     /** @brief Resumes a fixed-name session directly by player key. */
     virtual std::shared_ptr<Player> resume_session(Player::PlayerKey) = 0;
 
-    /** @brief Sets the current player that the MPRIS interface will control */
-    virtual void set_current_player(Player::PlayerKey) = 0;
-
     /** @brief Pauses sessions other than the supplied one. */
     virtual void pause_other_sessions(Player::PlayerKey) = 0;
 

=== modified file 'src/core/media/player_configuration.h'
--- src/core/media/player_configuration.h	2015-04-17 16:44:30 +0000
+++ src/core/media/player_configuration.h	2016-04-20 15:59:04 +0000
@@ -19,6 +19,7 @@
 #define CORE_UBUNTU_MEDIA_PLAYER_CLIENT_CONFIGURATION_H_
 
 #include <core/media/player.h>
+#include <core/media/service.h>
 
 #include <core/dbus/bus.h>
 #include <core/dbus/object.h>
@@ -35,6 +36,8 @@
     std::shared_ptr<core::dbus::Service> service;
     // The actual session object representing a player instance.
     std::shared_ptr<core::dbus::Object> session;
+    // The Service instance that manages Player instances
+    core::ubuntu::media::Service* player_service;
 };
 
 #endif // CORE_UBUNTU_MEDIA_PLAYER_CLIENT_CONFIGURATION_H_

=== modified file 'src/core/media/player_implementation.cpp'
--- src/core/media/player_implementation.cpp	2016-02-19 16:14:42 +0000
+++ src/core/media/player_implementation.cpp	2016-04-20 15:59:04 +0000
@@ -17,7 +17,10 @@
  *              Jim Hodapp <[e-mail]>
  */
 
+#include <core/media/service.h>
+
 #include "player_implementation.h"
+#include "service_skeleton.h"
 #include "util/timeout.h"
 
 #include <unistd.h>
@@ -302,14 +305,14 @@
         }
     }
 
-    void update_mpris_properties(void)
+    void update_mpris_properties()
     {
-        bool has_previous =    track_list->has_previous()
+        const bool has_previous = track_list->has_previous()
                             or parent->Parent::loop_status() != Player::LoopStatus::none;
-        bool has_next =    track_list->has_next()
+        const bool has_next = track_list->has_next()
                         or parent->Parent::loop_status() != Player::LoopStatus::none;
-        auto n_tracks = track_list->tracks()->size();
-        bool has_tracks = (n_tracks > 0) ? true : false;
+        const auto n_tracks = track_list->tracks()->size();
+        const bool has_tracks = (n_tracks > 0) ? true : false;
 
         std::cout << "Updating MPRIS TrackList properties"
                   << "; Tracks: " << n_tracks
@@ -323,6 +326,40 @@
         parent->can_go_next().set(has_next);
     }
 
+    ServiceSkeleton* get_player_service_skeleton() const
+    {
+        return reinterpret_cast<ServiceSkeleton*>(config.parent.player_service);
+    }
+
+    bool update_current_player(const media::Player::PlayerKey& key)
+    {
+        if (not config.parent.player_service)
+            return false;
+
+        get_player_service_skeleton()->set_current_player(key);
+        return true;
+    }
+
+    // Utility to hide the complexity of getting the ptr to current player
+    bool is_current_player(const media::PlayerImplementation<Parent> *p) const
+    {
+        return p == get_player_service_skeleton()->get_current_player().get();
+    }
+
+    bool is_multimedia_role() const
+    {
+        return get_player_service_skeleton()->is_multimedia_role();
+    }
+
+    bool reset_current_player()
+    {
+        if (not get_player_service_skeleton())
+            return false;
+
+        get_player_service_skeleton()->reset_current_player();
+        return true;
+    }
+
     // Our link back to our parent.
     media::PlayerImplementation<Parent>* parent;
     // We just store the parameters passed on construction.
@@ -479,6 +516,20 @@
         // are cleared
         d->clear_wakelocks();
         d->track_list->reset();
+
+        // Here it is decided if it is good to reset the current player. The
+        // conditions to do so are very simple. It must be a multimedia role
+        // and the Player that disconnected must be the last player to play
+        // audio/video.
+        if (d->is_multimedia_role() && d->is_current_player(this))
+        {
+            // This is not a fatal error but merely
+            // a warning that should be logged
+            if (not d->reset_current_player())
+                std::cerr << "Failed to reset current player in "
+                    << __PRETTY_FUNCTION__ << std::endl;
+        }
+
         // And tell the outside world that the client has gone away
         d->on_client_disconnected();
     });
@@ -688,7 +739,8 @@
     d->track_list->reset();
 
     // If empty uri, give the same meaning as QMediaPlayer::setMedia("")
-    if (uri.empty()) {
+    if (uri.empty())
+    {
         cout << __PRETTY_FUNCTION__ << ": resetting current media" << endl;
         return true;
     }
@@ -698,6 +750,7 @@
     // 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;
 }
 
@@ -722,6 +775,15 @@
 template<typename Parent>
 void media::PlayerImplementation<Parent>::play()
 {
+    if (Parent::audio_stream_role() == media::Player::AudioStreamRole::multimedia)
+    {
+        std::cout << "==== Updating the current player from " << __PRETTY_FUNCTION__ << std::endl;
+        // This player will begin playing so make sure it's the current player. If
+        // this operation fails it is not a fatal condition but should be logged
+        if (not d->update_current_player(d->config.key))
+            std::cerr << "Failed to update current player in " << __PRETTY_FUNCTION__ << std::endl;
+    }
+
     d->engine->play();
 }
 

=== modified file 'src/core/media/player_skeleton.h'
--- src/core/media/player_skeleton.h	2015-04-17 15:13:56 +0000
+++ src/core/media/player_skeleton.h	2016-04-20 15:59:04 +0000
@@ -57,6 +57,8 @@
         std::shared_ptr<core::dbus::Service> service;
         // The session object that we want to expose the skeleton upon.
         std::shared_ptr<core::dbus::Object> session;
+        // The Service instance that manages Player instances
+        core::ubuntu::media::Service* player_service;
         // Our functional dependencies.
         apparmor::ubuntu::RequestContextResolver::Ptr request_context_resolver;
         apparmor::ubuntu::RequestAuthenticator::Ptr request_authenticator;

=== modified file 'src/core/media/service_implementation.cpp'
--- src/core/media/service_implementation.cpp	2016-03-02 18:32:46 +0000
+++ src/core/media/service_implementation.cpp	2016-04-20 15:59:04 +0000
@@ -185,11 +185,13 @@
     auto player = std::make_shared<media::PlayerImplementation<media::PlayerSkeleton>>
                     (media::PlayerImplementation<media::PlayerSkeleton>::Configuration
     {
+        // Derive a PlayerSkeleton-specific Configuration based on Player::Configuration
         media::PlayerSkeleton::Configuration
         {
             conf.bus,
             conf.service,
             conf.session,
+            conf.player_service,
             d->request_context_resolver,
             d->request_authenticator
         },
@@ -259,13 +261,11 @@
 
 void media::ServiceImplementation::set_current_player(Player::PlayerKey)
 {
-  // no impl
+    // no impl
 }
 
 void media::ServiceImplementation::pause_other_sessions(media::Player::PlayerKey key)
 {
-    std::cout << __PRETTY_FUNCTION__ << std::endl;
-
     if (not d->configuration.player_store->has_player_for_key(key))
     {
         cerr << "Could not find Player by key: " << key << endl;

=== modified file 'src/core/media/service_skeleton.cpp'
--- src/core/media/service_skeleton.cpp	2016-03-02 18:32:46 +0000
+++ src/core/media/service_skeleton.cpp	2016-04-20 15:59:04 +0000
@@ -131,7 +131,8 @@
             key,
             impl->access_bus(),
             impl->access_service(),
-            impl->access_service()->add_object_for_path(op)
+            impl->access_service()->add_object_for_path(op),
+            impl
         };
 
         cout << "Session created by request of: " << msg->sender()
@@ -366,7 +367,8 @@
                     key,
                     impl->access_bus(),
                     impl->access_service(),
-                    impl->access_service()->add_object_for_path(op)
+                    impl->access_service()->add_object_for_path(op),
+                    impl
                 };
 
                 auto session = impl->create_session(config);
@@ -787,6 +789,22 @@
             std::cout << __PRETTY_FUNCTION__ << std::endl;
             // And announce that we can no longer be controlled.
             player.properties.can_control->set(false);
+            player.properties.can_play->set(false);
+            player.properties.can_pause->set(false);
+            player.properties.can_go_previous->set(false);
+            player.properties.can_go_next->set(false);
+
+            // Reset to null event connections
+            connections.seeked_to = the_empty_signal.connect([](){});
+            connections.duration_changed = the_empty_signal.connect([](){});
+            connections.position_changed = the_empty_signal.connect([](){});
+            connections.playback_status_changed = the_empty_signal.connect([](){});
+            connections.loop_status_changed = the_empty_signal.connect([](){});
+            connections.can_play_changed = the_empty_signal.connect([](){});
+            connections.can_pause_changed = the_empty_signal.connect([](){});
+            connections.can_go_previous_changed = the_empty_signal.connect([](){});
+            connections.can_go_next_changed = the_empty_signal.connect([](){});
+
             current_player.reset();
         }
 
@@ -901,6 +919,21 @@
         d->exported.set_current_player(player);
 }
 
+bool media::ServiceSkeleton::is_multimedia_role() const
+{
+    return d->exported.is_multimedia_role();
+}
+
+std::shared_ptr<media::Player> media::ServiceSkeleton::get_current_player() const
+{
+    return d->exported.current_player.lock();
+}
+
+void media::ServiceSkeleton::reset_current_player()
+{
+    d->exported.reset_current_player();
+}
+
 void media::ServiceSkeleton::pause_other_sessions(media::Player::PlayerKey key)
 {
     d->configuration.impl->pause_other_sessions(key);

=== modified file 'src/core/media/service_skeleton.h'
--- src/core/media/service_skeleton.h	2015-09-08 20:03:56 +0000
+++ src/core/media/service_skeleton.h	2016-04-20 15:59:04 +0000
@@ -60,6 +60,17 @@
     std::shared_ptr<Player> create_fixed_session(const std::string& name, const Player::Configuration&);
     std::shared_ptr<Player> resume_session(Player::PlayerKey);
     void set_current_player(Player::PlayerKey key);
+
+    /** @brief returns true for multimedia role */
+    bool is_multimedia_role() const;
+
+    /** @brief gets the current player controlled by MRIS */
+    std::shared_ptr<Player> get_current_player() const;
+
+    /** @detail Resets the current player so that the MPRIS interface will no
+     *  longer have a current player to control
+     */
+    void reset_current_player();
     void pause_other_sessions(Player::PlayerKey key);
 
     void run();