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 :)

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

184. By Konrad Zapałowicz on 2016-04-15

Revert change which was reviewed as bad

185. By Konrad Zapałowicz on 2016-04-15

Fix resetting the current player on disconnection and interface pollution

This commit changes:

* The condition under which the current player is reset. Originally
  this happened only for multimedia role however this turned out to be
  faulty and trigger an unwanted behavior [1]. This change allows the
  reset to happen only when it is a multimedia role [already there] and
  the player is the current player i.e. the Player instance that
  disconnected was the last player to play audio/video [added].

  In order to achieve this the ServiceSkeleton class has been extended
  with a new function to return the current player.

* Reduces the pollution of the Service interface, which has been
  introduced in rev 152 & 153, by moving the added functions to the
  ServiceSkeleton class. This is because the Service interface should
  be used only for DBus exported stuff.

1.

The issue as described by jibel was: I see 1 issue with the silo,
play a video, open the music app (video will pause), enqueue several
tracks and start playing (verify that MPRIS switched to music app),
reveal the spread and close the mediaplayer the controls switch to the
mediaplayer but the music continues playing then if you switch back to
the music app, the controls are still for the mediaplayer even if music
app is in the foreground and music is playign. I've to pause/play the
track in the music app for the music app to take control of mpris.

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-15 18:49:23 +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-15 18:49:23 +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-15 18:49:23 +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-15 18:49:23 +0000
@@ -19,10 +19,23 @@
 #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>
 
+
+namespace core
+{
+namespace ubuntu
+{
+namespace media
+{
+class ServiceSkeleton;
+}
+}
+}
+
 // Our internal structure for handing down parameters from the skeleton
 // to the implementation in a way that is opaque to the client.
 struct core::ubuntu::media::Player::Configuration
@@ -35,6 +48,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::ServiceSkeleton* 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-15 18:49:23 +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,35 @@
         parent->can_go_next().set(has_next);
     }
 
+    bool update_current_player(const media::Player::PlayerKey& key)
+    {
+        if (not config.parent.player_service)
+            return false;
+
+        config.parent.player_service->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 == config.parent.player_service->get_current_player().get();
+    }
+
+    bool is_multimedia_role() const
+    {
+        return config.parent.player_service->is_multimedia_role();
+    }
+
+    bool reset_current_player()
+    {
+        if (not config.parent.player_service)
+            return false;
+
+        config.parent.player_service->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 +511,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 +734,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 +745,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 +770,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-15 18:49:23 +0000
@@ -43,7 +43,7 @@
 struct ExternalServices;
 }
 
-class Service;
+class ServiceSkeleton;
 
 class PlayerSkeleton : public core::ubuntu::media::Player
 {
@@ -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 ServiceSkeleton instance that manages Player instances
+        core::ubuntu::media::ServiceSkeleton* 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-15 18:49:23 +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-15 18:49:23 +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-15 18:49:23 +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();