Merge lp:~phablet-team/media-hub/wily-sync into lp:media-hub

[Alfonso Sanchez-Beato]

[ Simon Fels ]
  * Prevent us from leaking file descriptors and memory due to not
    releasing gstreamer elements correctly.

  [ Alfonso Sanchez-Beato ]
  * Avoid getting additional references for audio_sink, which were not
    being freed. Also, remove unneeded unrefs that must be performed in
    gstreamer instead.
  * Emit CanPlay, CanPause, and CanGo[Next|Previous] in the mpris object
    path

  [ Jim Hodapp ]
  * Make sure the correct player is set as the current player controled by
    MPRIS.
  * Improved the can_go_next() and can_go_previous() logic to always
    return true if the loop_status is currently set to loop over the
    entire playlist.

[Jim Hodapp (jhodapp)]

LGTM.

review: Approve (code)

=== modified file 'CMakeLists.txt'
--- CMakeLists.txt	2015-09-01 08:30:08 +0000
+++ CMakeLists.txt	2015-10-16 08:36:42 +0000
@@ -3,7 +3,7 @@
 project(ubuntu-media-hub)
 
 set(UBUNTU_MEDIA_HUB_VERSION_MAJOR 4)
-set(UBUNTU_MEDIA_HUB_VERSION_MINOR 0)
+set(UBUNTU_MEDIA_HUB_VERSION_MINOR 1)
 set(UBUNTU_MEDIA_HUB_VERSION_PATCH 0)
 
 set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wall -Wextra -fPIC -pthread")

=== modified file 'README'
--- README	2014-08-08 14:36:29 +0000
+++ README	2015-10-16 08:36:42 +0000
@@ -1,3 +1,12 @@
+Coding Convention:
+------------------
+
+NOTE: the media-hub code did not start out with the following coding convention, but it is being introduced to try and
+converge on a standard from this point forward:
+
+https://google-styleguide.googlecode.com/svn/trunk/cppguide.html
+
+
 To Build:
 ---------
 

=== modified file 'debian/changelog'
--- debian/changelog	2015-09-07 10:26:04 +0000
+++ debian/changelog	2015-10-16 08:36:42 +0000
@@ -1,3 +1,25 @@
+media-hub (4.1.0+15.10.20151016-0ubuntu1) UNRELEASED; urgency=medium
+
+  [ Simon Fels ]
+  * Prevent us from leaking file descriptors and memory due to not releasing
+    gstreamer elements correctly.
+
+  [ Alfonso Sanchez-Beato ]
+  * Avoid getting additional references for audio_sink, which were not
+    being freed. Also, remove unneeded unrefs that must be performed in
+    gstreamer instead.
+  * Emit CanPlay, CanPause, and CanGo[Next|Previous] in the mpris object path
+
+  [ Jim Hodapp ]
+  * Make sure the correct player is set as the current player controled by
+    MPRIS.
+  * Improved the can_go_next() and can_go_previous() logic to always return
+    true if the loop_status is currently set to loop over the entire
+    playlist.
+
+
+ -- Alfonso Sanchez-Beato (email Canonical) <[e-mail]>  Fri, 16 Oct 2015 10:28:38 +0200
+
 media-hub (4.0.0+15.10.20150907-0ubuntu1) wily; urgency=medium
 
   * Gather information about missing codecs and selected streams from

=== modified file 'include/core/media/service.h'
--- include/core/media/service.h	2015-04-14 02:43:56 +0000
+++ include/core/media/service.h	2015-10-16 08:36:42 +0000
@@ -60,6 +60,9 @@
     /** @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/gstreamer/meta_data_extractor.h'
--- src/core/media/gstreamer/meta_data_extractor.h	2015-04-27 23:01:48 +0000
+++ src/core/media/gstreamer/meta_data_extractor.h	2015-10-16 08:36:42 +0000
@@ -147,7 +147,7 @@
     MetaDataExtractor()
         : pipe(gst_pipeline_new("meta_data_extractor_pipeline")),
           decoder(gst_element_factory_make ("uridecodebin", NULL)),
-          bus(GST_ELEMENT_BUS(pipe))
+          bus(gst_element_get_bus(pipe))
     {
         gst_bin_add(GST_BIN(pipe), decoder);
 
@@ -159,8 +159,40 @@
 
     ~MetaDataExtractor()
     {
-        gst_element_set_state(pipe, GST_STATE_NULL);
-        // gst_object_unref(pipe);
+        set_state_and_wait(GST_STATE_NULL);
+        gst_object_unref(pipe);
+    }
+
+    bool set_state_and_wait(GstState new_state)
+    {
+        static const std::chrono::nanoseconds state_change_timeout
+        {
+            // We choose a quite high value here as tests are run under valgrind
+            // and gstreamer pipeline setup/state changes take longer in that scenario.
+            // The value does not negatively impact runtime performance.
+            std::chrono::milliseconds{5000}
+        };
+
+        auto ret = gst_element_set_state(pipe, new_state);
+
+        bool result = false; GstState current, pending;
+        switch(ret)
+        {
+            case GST_STATE_CHANGE_FAILURE:
+                result = false; break;
+            case GST_STATE_CHANGE_NO_PREROLL:
+            case GST_STATE_CHANGE_SUCCESS:
+                result = true; break;
+            case GST_STATE_CHANGE_ASYNC:
+                result = GST_STATE_CHANGE_SUCCESS == gst_element_get_state(
+                    pipe,
+                    &current,
+                    &pending,
+                    state_change_timeout.count());
+                break;
+        }
+
+        return result;
     }
 
     core::ubuntu::media::Track::MetaData meta_data_for_track_with_uri(const core::ubuntu::media::Track::UriType& uri)
@@ -193,11 +225,11 @@
 
         if (std::future_status::ready != future.wait_for(std::chrono::seconds(4)))
         {
-            gst_element_set_state(pipe, GST_STATE_NULL);
+            set_state_and_wait(GST_STATE_NULL);
             throw std::runtime_error("Problem extracting meta data for track");
         } else
         {
-            gst_element_set_state(pipe, GST_STATE_NULL);
+            set_state_and_wait(GST_STATE_NULL);
         }
 
         return future.get();

=== modified file 'src/core/media/gstreamer/playbin.cpp'
--- src/core/media/gstreamer/playbin.cpp	2015-09-07 10:15:51 +0000
+++ src/core/media/gstreamer/playbin.cpp	2015-10-16 08:36:42 +0000
@@ -101,6 +101,7 @@
       bus{gst_element_get_bus(pipeline)},
       file_type(MEDIA_FILE_TYPE_NONE),
       video_sink(nullptr),
+      audio_sink(nullptr),
       on_new_message_connection_async(
           bus.on_new_message_async.connect(
               std::bind(
@@ -142,13 +143,39 @@
         );
 }
 
+// Note that we might be accessing freed memory here, so activate DEBUG_REFS
+// only for debugging
+//#define DEBUG_REFS
+#ifdef DEBUG_REFS
+static void print_refs(const gstreamer::Playbin &pb, const char *func)
+{
+    using namespace std;
+
+    cout << func << endl;
+    if (pb.pipeline)
+        cout << "pipeline:   " << GST_OBJECT_REFCOUNT(pb.pipeline) << endl;
+    if (pb.video_sink)
+        cout << "video_sink: " << GST_OBJECT_REFCOUNT(pb.video_sink) << endl;
+    if (pb.audio_sink)
+        cout << "audio_sink: " << GST_OBJECT_REFCOUNT(pb.audio_sink) << endl;
+}
+#endif
+
 gstreamer::Playbin::~Playbin()
 {
+#ifdef DEBUG_REFS
+    print_refs(*this, "gstreamer::Playbin::~Playbin pipeline");
+#endif
+
     g_signal_handler_disconnect(pipeline, about_to_finish_handler_id);
     g_signal_handler_disconnect(pipeline, source_setup_handler_id);
 
     if (pipeline)
         gst_object_unref(pipeline);
+
+#ifdef DEBUG_REFS
+    print_refs(*this, "gstreamer::Playbin::~Playbin pipeline");
+#endif
 }
 
 void gstreamer::Playbin::reset()
@@ -290,7 +317,7 @@
 
     if (::getenv("CORE_UBUNTU_MEDIA_SERVICE_AUDIO_SINK_NAME") != nullptr)
     {
-        auto audio_sink = gst_element_factory_make (
+        audio_sink = gst_element_factory_make (
                     ::getenv("CORE_UBUNTU_MEDIA_SERVICE_AUDIO_SINK_NAME"),
                     "audio-sink");
 
@@ -374,17 +401,13 @@
 /** Sets the new audio stream role on the pulsesink in playbin */
 void gstreamer::Playbin::set_audio_stream_role(media::Player::AudioStreamRole new_audio_role)
 {
-    GstElement *audio_sink = NULL;
-    g_object_get (pipeline, "audio-sink", &audio_sink, NULL);
-
     std::string role_str("props,media.role=" + get_audio_role_str(new_audio_role));
     std::cout << "Audio stream role: " << role_str << std::endl;
 
     GstStructure *props = gst_structure_from_string (role_str.c_str(), NULL);
-    if (audio_sink != nullptr && props != nullptr)
+    if (audio_sink != nullptr && props != nullptr) {
         g_object_set (audio_sink, "stream-properties", props, NULL);
-    else
-    {
+    } else {
         std::cerr <<
             "Warning: couldn't set audio stream role - couldn't get audio_sink from pipeline" <<
             std::endl;

=== modified file 'src/core/media/gstreamer/playbin.h'
--- src/core/media/gstreamer/playbin.h	2015-08-10 13:07:04 +0000
+++ src/core/media/gstreamer/playbin.h	2015-10-16 08:36:42 +0000
@@ -117,6 +117,7 @@
     gstreamer::Bus bus;
     MediaFileType file_type;
     GstElement* video_sink;
+    GstElement* audio_sink;
     core::Connection on_new_message_connection_async;
     bool is_seeking;
     mutable uint64_t previous_position;

=== modified file 'src/core/media/mpris/player.h'
--- src/core/media/mpris/player.h	2015-04-06 20:59:14 +0000
+++ src/core/media/mpris/player.h	2015-10-16 08:36:42 +0000
@@ -192,12 +192,12 @@
             // Default values for properties
             struct Defaults
             {
-                Properties::CanPlay::ValueType can_play{true};
-                Properties::CanPause::ValueType can_pause{true};
+                Properties::CanPlay::ValueType can_play{false};
+                Properties::CanPause::ValueType can_pause{false};
                 Properties::CanSeek::ValueType can_seek{true};
                 Properties::CanControl::ValueType can_control{true};
-                Properties::CanGoNext::ValueType can_go_next{true};
-                Properties::CanGoPrevious::ValueType can_go_previous{true};
+                Properties::CanGoNext::ValueType can_go_next{false};
+                Properties::CanGoPrevious::ValueType can_go_previous{false};
                 Properties::IsVideoSource::ValueType is_video_source{false};
                 Properties::IsAudioSource::ValueType is_audio_source{true};
                 Properties::PlaybackStatus::ValueType playback_status{PlaybackStatus::stopped};
@@ -307,6 +307,26 @@
             {
                 on_property_value_changed<Properties::Shuffle>(shuffle);
             });
+
+            properties.can_play->changed().connect([this](bool can_play)
+            {
+                on_property_value_changed<Properties::CanPlay>(can_play);
+            });
+
+            properties.can_pause->changed().connect([this](bool can_pause)
+            {
+                on_property_value_changed<Properties::CanPause>(can_pause);
+            });
+
+            properties.can_go_next->changed().connect([this](bool can_go_next)
+            {
+                on_property_value_changed<Properties::CanGoNext>(can_go_next);
+            });
+
+            properties.can_go_previous->changed().connect([this](bool can_go_previous)
+            {
+                on_property_value_changed<Properties::CanGoPrevious>(can_go_previous);
+            });
         }
 
         template<typename Property>

=== modified file 'src/core/media/mpris/service.h'
--- src/core/media/mpris/service.h	2015-04-10 16:13:55 +0000
+++ src/core/media/mpris/service.h	2015-10-16 08:36:42 +0000
@@ -115,6 +115,7 @@
     DBUS_CPP_METHOD_WITH_TIMEOUT_DEF(DestroySession, Service, 1000)
     DBUS_CPP_METHOD_WITH_TIMEOUT_DEF(CreateFixedSession, Service, 1000)
     DBUS_CPP_METHOD_WITH_TIMEOUT_DEF(ResumeSession, Service, 1000)
+    DBUS_CPP_METHOD_WITH_TIMEOUT_DEF(SetCurrentPlayer, Service, 1000)
     DBUS_CPP_METHOD_WITH_TIMEOUT_DEF(PauseOtherSessions, Service, 1000)
 };
 }

=== modified file 'src/core/media/player_implementation.cpp'
--- src/core/media/player_implementation.cpp	2015-07-27 22:15:33 +0000
+++ src/core/media/player_implementation.cpp	2015-10-16 08:36:42 +0000
@@ -301,6 +301,27 @@
         }
     }
 
+    void update_mpris_properties(void)
+    {
+        bool has_previous =    track_list->has_previous()
+                            or parent->Parent::loop_status() != Player::LoopStatus::none;
+        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;
+
+        std::cout << "Updating MPRIS TrackList properties"
+                  << "; Tracks: " << n_tracks
+                  << ", has_previous: " << has_previous
+                  << ", has_next: " << has_next << std::endl;
+
+        // Update properties
+        parent->can_play().set(has_tracks);
+        parent->can_pause().set(has_tracks);
+        parent->can_go_previous().set(has_previous);
+        parent->can_go_next().set(has_next);
+    }
+
     // Our link back to our parent.
     media::PlayerImplementation<Parent>* parent;
     // We just store the parameters passed on construction.
@@ -327,8 +348,8 @@
       d{std::make_shared<Private>(this, config)}
 {
     // Initialize default values for Player interface properties
-    Parent::can_play().set(true);
-    Parent::can_pause().set(true);
+    Parent::can_play().set(false);
+    Parent::can_pause().set(false);
     Parent::can_seek().set(true);
     Parent::can_go_previous().set(false);
     Parent::can_go_next().set(false);
@@ -376,13 +397,15 @@
 
     std::function<bool()> can_go_next_getter = [this]()
     {
-        return d->track_list->has_next();
+        // If LoopStatus == playlist, then there is always a next track
+        return d->track_list->has_next() or Parent::loop_status() != Player::LoopStatus::none;
     };
     Parent::can_go_next().install(can_go_next_getter);
 
     std::function<bool()> can_go_previous_getter = [this]()
     {
-        return d->track_list->has_previous();
+        // If LoopStatus == playlist, then there is always a next previous
+        return d->track_list->has_previous() or Parent::loop_status() != Player::LoopStatus::none;
     };
     Parent::can_go_previous().install(can_go_previous_getter);
 
@@ -520,7 +543,26 @@
         std::cout << "** Track was added, handling in PlayerImplementation" << std::endl;
         if (d->track_list->tracks()->size() == 1)
             d->open_first_track_from_tracklist(id);
-    });
+
+        d->update_mpris_properties();
+    });
+
+    d->track_list->on_track_removed().connect([this](const media::Track::Id&)
+    {
+        d->update_mpris_properties();
+    });
+
+    d->track_list->on_track_changed().connect([this](const media::Track::Id&)
+    {
+        d->update_mpris_properties();
+    });
+
+    d->track_list->on_track_list_replaced().connect(
+        [this](const media::TrackList::ContainerTrackIdTuple&)
+        {
+            d->update_mpris_properties();
+        }
+    );
 
     // Everything is setup, we now subscribe to death notifications.
     std::weak_ptr<Private> wp{d};

=== modified file 'src/core/media/service_implementation.cpp'
--- src/core/media/service_implementation.cpp	2015-04-17 16:44:30 +0000
+++ src/core/media/service_implementation.cpp	2015-10-16 08:36:42 +0000
@@ -246,8 +246,15 @@
   return std::shared_ptr<media::Player>();
 }
 
+void media::ServiceImplementation::set_current_player(Player::PlayerKey key)
+{
+  // 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_implementation.h'
--- src/core/media/service_implementation.h	2015-04-16 15:19:35 +0000
+++ src/core/media/service_implementation.h	2015-10-16 08:36:42 +0000
@@ -49,6 +49,7 @@
     void destroy_session(const std::string&, const Player::Configuration&);
     std::shared_ptr<Player> create_fixed_session(const std::string& name, const Player::Configuration&);
     std::shared_ptr<Player> resume_session(Player::PlayerKey key);
+    void set_current_player(Player::PlayerKey key);
     void pause_other_sessions(Player::PlayerKey key);
 
 private:

=== modified file 'src/core/media/service_skeleton.cpp'
--- src/core/media/service_skeleton.cpp	2015-08-11 16:10:14 +0000
+++ src/core/media/service_skeleton.cpp	2015-10-16 08:36:42 +0000
@@ -59,7 +59,7 @@
           object(impl->access_service()->add_object_for_path(
                      dbus::traits::Service<media::Service>::object_path())),
           configuration(config),
-          exported(impl->access_bus(), config.cover_art_resolver)
+          exported(impl->access_bus(), config.cover_art_resolver, impl)
     {
         object->install_method_handler<mpris::Service::CreateSession>(
                     std::bind(
@@ -91,6 +91,11 @@
                         &Private::handle_resume_session,
                         this,
                         std::placeholders::_1));
+        object->install_method_handler<mpris::Service::SetCurrentPlayer>(
+                    std::bind(
+                        &Private::handle_set_current_player,
+                        this,
+                        std::placeholders::_1));
         object->install_method_handler<mpris::Service::PauseOtherSessions>(
                     std::bind(
                         &Private::handle_pause_other_sessions,
@@ -102,7 +107,7 @@
     {
         static unsigned int session_counter = 0;
 
-        unsigned int current_session = session_counter++;
+        const unsigned int current_session = session_counter++;
         boost::uuids::uuid uuid = gen();
 
         std::stringstream ss;
@@ -134,7 +139,8 @@
             configuration.player_store->add_player_for_key(key, impl->create_session(config));
             uuid_player_map.insert(std::make_pair(uuid, key));
 
-            request_context_resolver->resolve_context_for_dbus_name_async(msg->sender(), [this, key, msg](const media::apparmor::ubuntu::Context& context)
+            request_context_resolver->resolve_context_for_dbus_name_async(msg->sender(),
+                    [this, key, msg](const media::apparmor::ubuntu::Context& context)
             {
                 fprintf(stderr, "%s():%d -- app_name='%s', attached\n", __func__, __LINE__, context.str().c_str());
                 player_owner_map.insert(std::make_pair(key, std::make_tuple(context.str(), true, msg->sender())));
@@ -213,7 +219,8 @@
                 ss << "/core/ubuntu/media/Service/sessions/" << key;
                 dbus::types::ObjectPath op{ss.str()};
 
-                request_context_resolver->resolve_context_for_dbus_name_async(msg->sender(), [this, msg, key, op](const media::apparmor::ubuntu::Context& context)
+                request_context_resolver->resolve_context_for_dbus_name_async(msg->sender(),
+                        [this, msg, key, op](const media::apparmor::ubuntu::Context& context)
                 {
                     auto info = player_owner_map.at(key);
                     fprintf(stderr, "%s():%d -- reattach app_name='%s', info='%s', '%s'\n", __func__, __LINE__, context.str().c_str(), std::get<0>(info).c_str(), std::get<2>(info).c_str());
@@ -224,6 +231,12 @@
                         // Signal player reconnection
                         auto player = configuration.player_store->player_for_key(key);
                         player->reconnect();
+                        // We only care to allow the MPRIS controls to apply to multimedia player (i.e. audio, video)
+                        if (player->audio_stream_role() == media::Player::AudioStreamRole::multimedia)
+                        {
+                            std::cout << "Setting current_player" << std::endl;
+                            exported.set_current_player(player);
+                        }
 
                         auto reply = dbus::Message::make_method_return(msg);
                         reply->writer() << op;
@@ -280,7 +293,8 @@
                 // the session is no longer usable.
                 uuid_player_map.erase(uuid);
 
-                request_context_resolver->resolve_context_for_dbus_name_async(msg->sender(), [this, msg, key](const media::apparmor::ubuntu::Context& context)
+                request_context_resolver->resolve_context_for_dbus_name_async(msg->sender(),
+                        [this, msg, key](const media::apparmor::ubuntu::Context& context)
                 {
                     auto info = player_owner_map.at(key);
                     fprintf(stderr, "%s():%d -- Destroying app_name='%s', info='%s', '%s'\n", __func__, __LINE__, context.str().c_str(), std::get<0>(info).c_str(), std::get<2>(info).c_str());
@@ -424,9 +438,18 @@
         }
     }
 
+    void handle_set_current_player(const core::dbus::Message::Ptr& msg)
+    {
+        Player::PlayerKey key;
+        msg->reader() >> key;
+        impl->set_current_player(key);
+
+        auto reply = dbus::Message::make_method_return(msg);
+        impl->access_bus()->send(reply);
+    }
+
     void handle_pause_other_sessions(const core::dbus::Message::Ptr& msg)
     {
-        std::cout << __PRETTY_FUNCTION__ << std::endl;
         Player::PlayerKey key;
         msg->reader() >> key;
         impl->pause_other_sessions(key);
@@ -469,45 +492,33 @@
         {
             mpris::Player::Skeleton::Configuration::Defaults defaults;
 
-            // Disabled as track list is not fully implemented yet.
-            defaults.can_go_next = false;
-            // Disabled as track list is not fully implemented yet.
-            defaults.can_go_previous = false;
-
             return defaults;
         }
 
-        static std::string service_name()
-        {
-            static const bool export_to_indicator_sound_via_mpris
-            {
-                core::posix::this_process::env::get("UBUNTU_MEDIA_HUB_EXPORT_TO_INDICATOR_VIA_MPRIS", "0") == "1"
-            };
-
-            return export_to_indicator_sound_via_mpris ? "org.mpris.MediaPlayer2.MediaHub" :
-                                                         "hidden.org.mpris.MediaPlayer2.MediaHub";
-        }
-
-        explicit Exported(const dbus::Bus::Ptr& bus, const media::CoverArtResolver& cover_art_resolver)
+        explicit Exported(const dbus::Bus::Ptr& bus, const media::CoverArtResolver& cover_art_resolver,
+                media::ServiceSkeleton* impl)
             : bus{bus},
-              service{dbus::Service::add_service(bus, service_name())},
+              /* Export MediaHub service interface on dbus */
+              service{dbus::Service::add_service(bus, "org.mpris.MediaPlayer2.MediaHub")},
               object{service->add_object_for_path(dbus::types::ObjectPath{"/org/mpris/MediaPlayer2"})},
               media_player{mpris::MediaPlayer2::Skeleton::Configuration{bus, object, media_player_defaults()}},
               player{mpris::Player::Skeleton::Configuration{bus, object, player_defaults()}},
               playlists{mpris::Playlists::Skeleton::Configuration{bus, object, mpris::Playlists::Skeleton::Configuration::Defaults{}}},
-              cover_art_resolver{cover_art_resolver}
+              cover_art_resolver{cover_art_resolver},
+              impl{impl}
         {
             object->install_method_handler<core::dbus::interfaces::Properties::GetAll>([this](const core::dbus::Message::Ptr& msg)
             {
                 // Extract the interface
-                std::string itf; msg->reader() >> itf;
+                std::string interface;
+                msg->reader() >> interface;
                 core::dbus::Message::Ptr reply = core::dbus::Message::make_method_return(msg);
 
-                if (itf == mpris::Player::name())
+                if (interface == mpris::Player::name())
                     reply->writer() << player.get_all_properties();
-                else if (itf == mpris::MediaPlayer2::name())
+                else if (interface == mpris::MediaPlayer2::name())
                     reply->writer() << media_player.get_all_properties();
-                else if (itf == mpris::Playlists::name())
+                else if (interface == mpris::Playlists::name())
                     reply->writer() << playlists.get_all_properties();
 
                 Exported::bus->send(reply);
@@ -516,9 +527,9 @@
             // Setup method handlers for mpris::Player methods.
             auto next = [this](const core::dbus::Message::Ptr& msg)
             {
-                auto sp = current_player.lock();
+                const auto sp = current_player.lock();
 
-                if (sp)
+                if (is_multimedia_role())
                     sp->next();
 
                 Exported::bus->send(core::dbus::Message::make_method_return(msg));
@@ -527,9 +538,9 @@
 
             auto previous = [this](const core::dbus::Message::Ptr& msg)
             {
-                auto sp = current_player.lock();
+                const auto sp = current_player.lock();
 
-                if (sp)
+                if (is_multimedia_role())
                     sp->previous();
 
                 Exported::bus->send(core::dbus::Message::make_method_return(msg));
@@ -538,9 +549,9 @@
 
             auto pause = [this](const core::dbus::Message::Ptr& msg)
             {
-                auto sp = current_player.lock();
+                const auto sp = current_player.lock();
 
-                if (sp)
+                if (is_multimedia_role() and sp->can_pause())
                     sp->pause();
 
                 Exported::bus->send(core::dbus::Message::make_method_return(msg));
@@ -549,36 +560,52 @@
 
             auto stop = [this](const core::dbus::Message::Ptr& msg)
             {
-                auto sp = current_player.lock();
+                const auto sp = current_player.lock();
 
-                if (sp)
+                if (is_multimedia_role())
                     sp->stop();
 
                 Exported::bus->send(core::dbus::Message::make_method_return(msg));
             };
             object->install_method_handler<mpris::Player::Stop>(stop);
 
-            auto play = [this](const core::dbus::Message::Ptr& msg)
+            auto play = [this, impl](const core::dbus::Message::Ptr& msg)
             {
-                auto sp = current_player.lock();
-
-                if (sp)
+                const auto sp = current_player.lock();
+
+                if (is_multimedia_role() and sp->can_play())
+                {
+                    // Make sure other player sessions that are already playing
+                    // are paused before triggering new player (sp) to play
+                    if (impl)
+                        impl->pause_other_sessions(sp->key());
+
                     sp->play();
+                }
 
                 Exported::bus->send(core::dbus::Message::make_method_return(msg));
             };
             object->install_method_handler<mpris::Player::Play>(play);
 
-            auto play_pause = [this](const core::dbus::Message::Ptr& msg)
+            auto play_pause = [this, impl](const core::dbus::Message::Ptr& msg)
             {
-                auto sp = current_player.lock();
+                const auto sp = current_player.lock();
 
-                if (sp)
+                if (is_multimedia_role())
                 {
-                    if (sp->playback_status() == media::Player::PlaybackStatus::playing)
+                    if (sp->playback_status() == media::Player::PlaybackStatus::playing
+                            and sp->can_pause())
                         sp->pause();
-                    else if (sp->playback_status() != media::Player::PlaybackStatus::null)
+                    else if (sp->playback_status() != media::Player::PlaybackStatus::null
+                                and sp->can_play())
+                    {
+                        // Make sure other player sessions that are already playing
+                        // are paused before triggering new player (sp) to play
+                        if (impl)
+                            impl->pause_other_sessions(sp->key());
+
                         sp->play();
+                    }
                 }
 
                 Exported::bus->send(core::dbus::Message::make_method_return(msg));
@@ -586,15 +613,21 @@
             object->install_method_handler<mpris::Player::PlayPause>(play_pause);
         }
 
+        inline bool is_multimedia_role()
+        {
+            const auto sp = current_player.lock();
+
+            return (sp ? sp->audio_stream_role() == media::Player::AudioStreamRole::multimedia : false);
+        }
+
         void set_current_player(const std::shared_ptr<media::Player>& cp)
         {
-            unset_current_player();
-
+            std::cout << "*** " << __PRETTY_FUNCTION__ << std::endl;
             // We will not keep the object alive.
             current_player = cp;
 
             // And announce that we can be controlled again.
-            player.properties.can_control->set(false);
+            player.properties.can_control->set(true);
 
             // We wire up player state changes
             connections.seeked_to = cp->seeked_to().connect([this](std::uint64_t position)
@@ -612,68 +645,103 @@
                 player.properties.position->set(position);
             });
 
-            connections.playback_status_changed = cp->playback_status().changed().connect([this](core::ubuntu::media::Player::PlaybackStatus status)
+            connections.playback_status_changed = cp->playback_status().changed().connect(
+                [this](core::ubuntu::media::Player::PlaybackStatus status)
             {
                 player.properties.playback_status->set(mpris::Player::PlaybackStatus::from(status));
             });
 
-            connections.loop_status_changed = cp->loop_status().changed().connect([this](core::ubuntu::media::Player::LoopStatus status)
+            connections.loop_status_changed = cp->loop_status().changed().connect(
+                [this](core::ubuntu::media::Player::LoopStatus status)
             {
                 player.properties.loop_status->set(mpris::Player::LoopStatus::from(status));
             });
 
-            connections.meta_data_changed = cp->meta_data_for_current_track().changed().connect([this](const core::ubuntu::media::Track::MetaData& md)
-            {
-                mpris::Player::Dictionary dict;
-
-                bool has_title = md.count(xesam::Title::name) > 0;
-                bool has_album_name = md.count(xesam::Album::name) > 0;
-                bool has_artist_name = md.count(xesam::Artist::name) > 0;
-
-                if (has_title)
-                    dict[xesam::Title::name] = dbus::types::Variant::encode(md.get(xesam::Title::name));
-                if (has_album_name)
-                    dict[xesam::Album::name] = dbus::types::Variant::encode(md.get(xesam::Album::name));
-                if (has_artist_name)
-                    dict[xesam::Artist::name] = dbus::types::Variant::encode(md.get(xesam::Artist::name));
-
-                dict[mpris::metadata::ArtUrl::name] = dbus::types::Variant::encode(
-                            cover_art_resolver(
-                                has_title ? md.get(xesam::Title::name) : "",
-                                has_album_name ? md.get(xesam::Album::name) : "",
-                                has_artist_name ? md.get(xesam::Artist::name) : ""));
-
-                mpris::Player::Dictionary wrap;
-                wrap[mpris::Player::Properties::Metadata::name()] = dbus::types::Variant::encode(dict);
-
-                player.signals.properties_changed->emit(
-                            std::make_tuple(
-                                dbus::traits::Service<mpris::Player::Properties::Metadata::Interface>::interface_name(),
-                                wrap,
-                                std::vector<std::string>()));
-            });
+            connections.can_play_changed = cp->can_play().changed().connect(
+                [this](bool can_play)
+            {
+                player.properties.can_play->set(can_play);
+            });
+
+            connections.can_pause_changed = cp->can_pause().changed().connect(
+                [this](bool can_pause)
+            {
+                player.properties.can_pause->set(can_pause);
+            });
+
+            connections.can_go_previous_changed = cp->can_go_previous().changed().connect(
+                [this](bool can_go_previous)
+            {
+                player.properties.can_go_previous->set(can_go_previous);
+            });
+
+            connections.can_go_next_changed = cp->can_go_next().changed().connect(
+                [this](bool can_go_next)
+            {
+                player.properties.can_go_next->set(can_go_next);
+            });
+
+            // Sync property values between session and player mpris::Player instances
+            // TODO Getters from media::Player actually return values from a
+            // mpris::Player::Skeleton instance different from "player". Each of them use
+            // different DBus object paths, /core/ubuntu/media/Service/sessions/<n>
+            // and /org/mpris/MediaPlayer2 (this is the one enforced by the MPRIS spec).
+            // Discuss why this is needed with tvoss.
+            player.properties.duration->set(cp->duration().get());
+            player.properties.position->set(cp->position().get());
+            player.properties.playback_status->set(mpris::Player::PlaybackStatus::from(
+                                                       cp->playback_status().get()));
+            player.properties.loop_status->set(mpris::Player::LoopStatus::from(
+                                                   cp->loop_status().get()));
+            player.properties.can_play->set(cp->can_play().get());
+            player.properties.can_pause->set(cp->can_pause().get());
+            player.properties.can_go_previous->set(cp->can_go_previous().get());
+            player.properties.can_go_next->set(cp->can_go_next().get());
+
+#if 0
+            // TODO cover_art_resolver() is not implemented yet
+            connections.meta_data_changed = cp->meta_data_for_current_track().changed().connect(
+                [this](const core::ubuntu::media::Track::MetaData& md)
+                {
+                    mpris::Player::Dictionary dict;
+
+                    bool has_title = md.count(xesam::Title::name) > 0;
+                    bool has_album_name = md.count(xesam::Album::name) > 0;
+                    bool has_artist_name = md.count(xesam::Artist::name) > 0;
+
+                    if (has_title)
+                        dict[xesam::Title::name] = dbus::types::Variant::encode(md.get(xesam::Title::name));
+                    if (has_album_name)
+                        dict[xesam::Album::name] = dbus::types::Variant::encode(md.get(xesam::Album::name));
+                    if (has_artist_name)
+                        dict[xesam::Artist::name] = dbus::types::Variant::encode(md.get(xesam::Artist::name));
+
+                    dict[mpris::metadata::ArtUrl::name] = dbus::types::Variant::encode(
+                        cover_art_resolver(
+                            has_title ? md.get(xesam::Title::name) : "",
+                            has_album_name ? md.get(xesam::Album::name) : "",
+                            has_artist_name ? md.get(xesam::Artist::name) : ""));
+
+                    mpris::Player::Dictionary wrap;
+                    wrap[mpris::Player::Properties::Metadata::name()] = dbus::types::Variant::encode(dict);
+
+                    player.signals.properties_changed->emit(
+                        std::make_tuple(
+                            dbus::traits::Service<
+                            mpris::Player::Properties::Metadata::Interface>
+                            ::interface_name(),
+                            wrap,
+                            std::vector<std::string>()));
+                });
+#endif
         }
 
-        void unset_current_player()
+        void reset_current_player()
         {
+            std::cout << __PRETTY_FUNCTION__ << std::endl;
+            // And announce that we can no longer be controlled.
+            player.properties.can_control->set(false);
             current_player.reset();
-
-            // We disconnect all previous event connections.
-            connections.seeked_to.disconnect();
-            connections.duration_changed.disconnect();
-            connections.position_changed.disconnect();
-            connections.playback_status_changed.disconnect();
-            connections.loop_status_changed.disconnect();
-            connections.meta_data_changed.disconnect();
-
-            // And announce that we cannot be controlled anymore.
-            player.properties.can_control->set(false);
-        }
-
-        void unset_if_current(const std::shared_ptr<media::Player>& cp)
-        {
-            if (cp == current_player.lock())
-                unset_current_player();
         }
 
         dbus::Bus::Ptr bus;
@@ -688,6 +756,9 @@
         media::CoverArtResolver cover_art_resolver;
         // The actual player instance.
         std::weak_ptr<media::Player> current_player;
+
+        media::ServiceSkeleton* impl;
+
         // We track event connections.
         struct
         {
@@ -711,6 +782,22 @@
             {
                 the_empty_signal.connect([](){})
             };
+            core::Connection can_play_changed
+            {
+                the_empty_signal.connect([](){})
+            };
+            core::Connection can_pause_changed
+            {
+                the_empty_signal.connect([](){})
+            };
+            core::Connection can_go_previous_changed
+            {
+                the_empty_signal.connect([](){})
+            };
+            core::Connection can_go_next_changed
+            {
+                the_empty_signal.connect([](){})
+            };
             core::Connection meta_data_changed
             {
                 the_empty_signal.connect([](){})
@@ -759,6 +846,14 @@
     return d->configuration.impl->resume_session(key);
 }
 
+void media::ServiceSkeleton::set_current_player(media::Player::PlayerKey key)
+{
+    const auto player = d->configuration.player_store->player_for_key(key);
+    // We only care to allow the MPRIS controls to apply to multimedia player (i.e. audio, video)
+    if (player->audio_stream_role() == media::Player::AudioStreamRole::multimedia)
+        d->exported.set_current_player(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-04-16 15:19:35 +0000
+++ src/core/media/service_skeleton.h	2015-10-16 08:36:42 +0000
@@ -59,6 +59,7 @@
     void destroy_session(const std::string&, const media::Player::Configuration&);
     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);
     void pause_other_sessions(Player::PlayerKey key);
 
     void run();

=== modified file 'src/core/media/service_stub.cpp'
--- src/core/media/service_stub.cpp	2015-04-17 16:44:30 +0000
+++ src/core/media/service_stub.cpp	2015-10-16 08:36:42 +0000
@@ -139,9 +139,17 @@
     });
 }
 
+void media::ServiceStub::set_current_player(Player::PlayerKey key)
+{
+    auto op = d->object->invoke_method_synchronously<mpris::Service::SetCurrentPlayer,
+         void>(key);
+
+    if (op.is_error())
+        throw std::runtime_error("Problem setting current player: " + op.error());
+}
+
 void media::ServiceStub::pause_other_sessions(media::Player::PlayerKey key)
 {
-    std::cout << __PRETTY_FUNCTION__ << std::endl;
     auto op = d->object->invoke_method_synchronously<mpris::Service::PauseOtherSessions,
          void>(key);
 

=== modified file 'src/core/media/service_stub.h'
--- src/core/media/service_stub.h	2015-04-10 16:13:55 +0000
+++ src/core/media/service_stub.h	2015-10-16 08:36:42 +0000
@@ -45,6 +45,7 @@
     void destroy_session(const std::string& uuid, const Player::Configuration&);
     std::shared_ptr<Player> create_fixed_session(const std::string& name, const Player::Configuration&);
     std::shared_ptr<Player> resume_session(Player::PlayerKey key);
+    void set_current_player(Player::PlayerKey key);
     void pause_other_sessions(Player::PlayerKey key);
 
   private:

=== modified file 'src/core/media/track_list_implementation.cpp'
--- src/core/media/track_list_implementation.cpp	2015-08-11 15:25:50 +0000
+++ src/core/media/track_list_implementation.cpp	2015-10-16 08:36:42 +0000
@@ -215,20 +215,26 @@
 
 void media::TrackListImplementation::reset()
 {
-    std::cout << __PRETTY_FUNCTION__ << std::endl;
-
     // Make sure playback stops
     on_end_of_tracklist()();
+    // And make sure there is no "current" track
+    media::TrackListSkeleton::reset();
 
     auto result = tracks().update([this](TrackList::Container& container)
     {
-        container.clear();
+        TrackList::Container ids = container;
+
+        for (auto it = container.begin(); it != container.end(); ) {
+            auto id = *it;
+            it = container.erase(it);
+            on_track_removed()(id);
+        }
+
         container.resize(0);
         d->track_counter = 0;
+
         return true;
     });
 
-    media::TrackListSkeleton::reset();
-
     (void) result;
 }

=== modified file 'src/core/media/track_list_skeleton.cpp'
--- src/core/media/track_list_skeleton.cpp	2015-08-11 15:25:50 +0000
+++ src/core/media/track_list_skeleton.cpp	2015-10-16 08:36:42 +0000
@@ -241,16 +241,31 @@
 
 bool media::TrackListSkeleton::has_next()
 {
-    if (tracks().get().empty())
+    const auto n_tracks = tracks().get().size();
+
+    if (n_tracks == 0)
         return false;
 
+    // TODO Using current_iterator() makes media-hub crash later. Logic for
+    // handling the iterators must be reviewed. As a minimum updates to the
+    // track list should update current_track instead of the list being sneakly
+    // changed in player_implementation.cpp.
+    // To avoid the crash we consider that current_track will be eventually
+    // initialized to the first track when current_iterator() gets called.
+    if (d->current_track == d->empty_iterator) {
+        if (n_tracks < 2)
+            return false;
+        else
+            return true;
+    }
+
     const auto next_track = std::next(current_iterator());
     return !is_last_track(next_track);
 }
 
 bool media::TrackListSkeleton::has_previous()
 {
-    if (tracks().get().empty())
+    if (tracks().get().empty() || d->current_track == d->empty_iterator)
         return false;
 
     // If we are looping over the entire list, then there is always a previous track
@@ -273,8 +288,11 @@
 media::Track::Id media::TrackListSkeleton::next()
 {
     std::cout << __PRETTY_FUNCTION__ << std::endl;
-    if (tracks().get().empty())
-        return *(d->empty_iterator);
+    if (tracks().get().empty()) {
+        // TODO Change ServiceSkeleton to return with error from DBus call
+        std::cerr << "ERROR: no tracks, cannot go to next" << std::endl;
+        return media::Track::Id{};
+    }
 
     const auto next_track = std::next(current_iterator());
     bool do_go_to_next_track = false;
@@ -328,8 +346,11 @@
 media::Track::Id media::TrackListSkeleton::previous()
 {
     std::cout << __PRETTY_FUNCTION__ << std::endl;
-    if (tracks().get().empty())
-        return *(d->empty_iterator);
+    if (tracks().get().empty()) {
+        // TODO Change ServiceSkeleton to return with error from DBus call
+        std::cerr << "ERROR: no tracks, cannot go to previous" << std::endl;
+        return media::Track::Id{};
+    }
 
     bool do_go_to_previous_track = false;
 
@@ -521,7 +542,6 @@
 
 void media::TrackListSkeleton::reset()
 {
-    std::cout << __PRETTY_FUNCTION__ << std::endl;
     d->current_track = d->empty_iterator;
 }
 

=== modified file 'src/core/media/track_list_skeleton.h'
--- src/core/media/track_list_skeleton.h	2015-07-27 22:15:33 +0000
+++ src/core/media/track_list_skeleton.h	2015-10-16 08:36:42 +0000
@@ -51,10 +51,12 @@
     const core::Property<Container>& tracks() const;
 
     const core::Signal<ContainerTrackIdTuple>& on_track_list_replaced() const;
+    core::Signal<ContainerTrackIdTuple>& on_track_list_replaced();
     const core::Signal<Track::Id>& on_track_added() const;
     core::Signal<Track::Id>& on_track_added();
     const core::Signal<Track::Id>& on_track_removed() const;
     const core::Signal<Track::Id>& on_track_changed() const;
+    core::Signal<Track::Id>& on_track_changed();
     const core::Signal<std::pair<Track::Id, bool>>& on_go_to_track() const;
     core::Signal<std::pair<Track::Id, bool>>& on_go_to_track();
     const core::Signal<void>& on_end_of_tracklist() const;
@@ -77,9 +79,6 @@
 
     core::Property<bool>& can_edit_tracks();
 
-    core::Signal<ContainerTrackIdTuple>& on_track_list_replaced();
-    core::Signal<Track::Id>& on_track_changed();
-
     void reset();
 
 private: