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

[Jim Hodapp]

Enable exporting of metadata via MPRIS over dbus. This does not export everything yet and focused solely on track title, track album name and artist name.

Enable exporting of metadata via MPRIS over dbus. This does not export everything yet and focused solely on track title, track album name and artist name.

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

186. By Jim Hodapp on 2016-06-06

Add metadata.cpp

187. By Jim Hodapp on 2016-06-08

Make sure we don't lose the album_missing.svg image in indicator-sound when toggling play/pause

188. By Jim Hodapp on 2016-06-08

Undo last set of extra changes that snuck in to a commit

189. By Jim Hodapp on 2016-06-08

Make sure that the MPRIS client sees a the same data after switching between MPRIS clients

[Alfonso Sanchez-Beato (alfonsosanchezbeato)]

Some minor nits around.

review: Needs Fixing

[Konrad Zapałowicz (kzapalowicz)]

LGTM except on the sizeof thing.

review: Needs Fixing (code)

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

190. By Jim Hodapp on 2016-06-09

Address review comments

[Konrad Zapałowicz (kzapalowicz)]

LGTM

review: Approve (code)

[Alfonso Sanchez-Beato (alfonsosanchezbeato)]

LGTM

review: Approve

=== modified file 'include/core/media/track.h'
--- include/core/media/track.h	2015-07-02 12:30:09 +0000
+++ include/core/media/track.h	2016-06-09 16:54:40 +0000
@@ -39,10 +39,15 @@
 public:
     typedef std::string UriType;
     typedef std::string Id;
+    typedef std::map<std::string, std::string> MetaDataType;
 
     class MetaData
     {
     public:
+        static constexpr const char* TrackArtlUrlKey = "mpris:artUrl";
+        static constexpr const char* TrackLengthKey = "mpris:length";
+        static constexpr const char* TrackIdKey = "mpris:trackid";
+
         bool operator==(const MetaData& rhs) const
         {
             return map == rhs.map;
@@ -90,13 +95,36 @@
             return map.at(key);
         }
 
+        bool is_set(const std::string& key) const
+        {
+            try {
+                return map.at(key).empty();
+            } catch (const std::out_of_range& e) {
+                return false;
+            }
+        }
+
         const std::map<std::string, std::string>& operator*() const
         {
             return map;
         }
 
+        const std::string& album() const;
+        const std::string& artist() const;
+        const std::string& title() const;
+        const std::string& track_id() const;
+        const std::string& art_url() const;
+        const std::string& last_used() const;
+
+        void set_album(const std::string& album);
+        void set_artist(const std::string& artist);
+        void set_title(const std::string& title);
+        void set_track_id(const std::string& id);
+        void set_art_url(const std::string& url);
+        void set_last_used(const std::string& datetime);
+
     private:
-        std::map<std::string, std::string> map;
+        MetaDataType map;
     };
 
     Track(const Id& id);
@@ -108,51 +136,7 @@
 
     virtual const Id& id() const;
     virtual const UriType& uri() const;
-    /*
-    class MetaData
-    {
-    public:
-        MetaData() = default;
-        MetaData(const MetaData&) = default;
-        ~MetaData() = default;
-
-        MetaData& operator=(const MetaData&) = default;
-
-        bool operator==(const MetaData&) const
-        {
-            return true;
-        }
-
-        bool operator!=(const MetaData&) const
-        {
-            return false;
-        }
-
-        struct NotImplementedFields
-        {
-            NotImplementedFields() = default;
-
-            virtual const UriType& uri() const = 0;
-            virtual const std::chrono::microseconds length() const = 0;
-            virtual const UriType& art_uri() const = 0;
-            virtual const std::string& album() const = 0;
-            virtual const std::vector<std::string>& album_artist() const = 0;
-            virtual const std::vector<std::string>& artist() const = 0;
-            virtual const std::string& as_text() const = 0;
-            virtual unsigned int audio_bpm() const = 0;
-            virtual float auto_rating() const = 0;
-            virtual const std::vector<std::string>& comment() const = 0;
-            virtual const std::vector<std::string>& composer() const = 0;
-            virtual unsigned int disc_number() const = 0;
-            virtual const std::vector<std::string>& genre() const = 0;
-            virtual const std::vector<std::string>& lyricist() const = 0;
-            virtual const std::string title() const = 0;
-            virtual unsigned int track_number() const = 0;
-            virtual unsigned int use_count() const = 0;
-            virtual float user_rating() const = 0;
-        };
-    };
-*/
+
 private:
     struct Private;
     std::unique_ptr<Private> d;
 
=== modified file 'src/core/media/CMakeLists.txt'
--- src/core/media/CMakeLists.txt	2016-04-05 15:40:11 +0000
+++ src/core/media/CMakeLists.txt	2016-06-09 16:54:40 +0000
@@ -49,6 +49,7 @@
   service.cpp
   track.cpp
   track_list.cpp
+  metadata.cpp
 
   player_stub.cpp
   service_stub.cpp
@@ -99,6 +100,7 @@
     hybris_client_death_observer.cpp
     cover_art_resolver.cpp
     engine.cpp
+    metadata.cpp
 
     apparmor/context.cpp
     apparmor/ubuntu.cpp
 
=== modified file 'src/core/media/codec.h'
--- src/core/media/codec.h	2015-04-14 19:11:15 +0000
+++ src/core/media/codec.h	2016-06-09 16:54:40 +0000
@@ -23,10 +23,18 @@
 #include <core/media/player.h>
 #include <core/media/track.h>
 
+#include <core/dbus/types/stl/map.h>
 #include <core/dbus/types/stl/string.h>
+#include <core/dbus/types/variant.h>
 #include <core/dbus/types/stl/vector.h>
 #include <core/dbus/codec.h>
 
+#include <boost/lexical_cast.hpp>
+
+#include "core/media/xesam.h"
+
+#include "core/media/logger/logger.h"
+
 namespace core
 {
 namespace dbus
@@ -38,7 +46,7 @@
 {
     constexpr static ArgumentType type_value()
     {
-        return ArgumentType::floating_point;
+        return ArgumentType::array;
     }
     constexpr static bool is_basic_type()
     {
@@ -51,7 +59,7 @@
 
     static std::string signature()
     {
-        static const std::string s = TypeMapper<double>::signature();
+        static const std::string s = TypeMapper<std::map<std::string, dbus::types::Variant>>::signature();
         return s;
     }
 };
@@ -60,16 +68,83 @@
 template<>
 struct Codec<core::ubuntu::media::Track::MetaData>
 {
-    static void encode_argument(core::dbus::Message::Writer& out, const core::ubuntu::media::Track::MetaData& in)
+    static void encode_argument(core::dbus::Message::Writer& writer, const core::ubuntu::media::Track::MetaData& md)
     {
-        (void) out;
-        (void) in;
+        typedef std::pair<std::string, dbus::types::Variant> Pair;
+        auto dict = writer.open_array(dbus::types::Signature
+                {dbus::helper::TypeMapper<Pair>::signature()});
+
+        for (const auto& pair : *md)
+        {
+            auto de = dict.open_dict_entry();
+            {
+                if (pair.first == core::ubuntu::media::Track::MetaData::TrackLengthKey
+                        and not pair.second.empty())
+                {
+                    Codec<Pair>::encode_argument(de, std::make_pair(pair.first,
+                                dbus::types::Variant::encode(
+                                    boost::lexical_cast<std::int64_t>(pair.second))));
+                }
+                else if (pair.first == xesam::Album::name and not pair.second.empty())
+                {
+                    Codec<Pair>::encode_argument(de, std::make_pair(pair.first,
+                                dbus::types::Variant::encode(pair.second)));
+                }
+                else if (pair.first == xesam::AlbumArtist::name and not pair.second.empty())
+                {
+#if 0
+                    // TODO: This code doesn't work but will be needed for full MPRIS compliance.
+                    // Technically there can be more than one album artist stuffed into an array.
+                    // How to satisfy stuffing this data into dbus-cpp is what needs fixing.
+                    auto array = de.open_array(dbus::types::Signature
+                            {dbus::helper::TypeMapper<Pair>::signature()});
+                    {
+                        // TODO: this should really iterate over all artists, but seems
+                        // we only extract one artist from playbin
+                        Codec<Pair>::encode_argument(array, std::make_pair(pair.first,
+                                    dbus::types::Variant::encode(pair.second)));
+                    }
+                    de.close_array(std::move(array));
+#else
+                    Codec<Pair>::encode_argument(de, std::make_pair(pair.first,
+                                dbus::types::Variant::encode(pair.second)));
+#endif
+                }
+                else
+                {
+                    Codec<Pair>::encode_argument(de, std::make_pair(pair.first,
+                                dbus::types::Variant::encode(pair.second)));
+                }
+            }
+            dict.close_dict_entry(std::move(de));
+        }
+        writer.close_array(std::move(dict));
     }
 
-    static void decode_argument(core::dbus::Message::Reader& out, core::ubuntu::media::Track::MetaData& in)
+    static void decode_argument(core::dbus::Message::Reader& reader, core::ubuntu::media::Track::MetaData& md)
     {
-        (void) out;
-        (void) in;
+        auto array = reader.pop_array();
+
+        while (array.type() != dbus::ArgumentType::invalid)
+        {
+            auto entry = array.pop_dict_entry();
+            {
+                std::string key {entry.pop_string()};
+                auto variant = entry.pop_variant();
+                {
+                    if (key == xesam::Album::name)
+                    {
+                        const std::string album = variant.pop_string();
+                        MH_DEBUG("Getting key \"%s\" and value \"%s\"", key, album);
+                        md.set_album(album);
+                    }
+                    else
+                    {
+                        MH_WARNING("Unknown metadata key \"%s\" while decoding dbus message", key);
+                    }
+                }
+            }
+        }
     }
 };
 
 
=== modified file 'src/core/media/cover_art_resolver.cpp'
--- src/core/media/cover_art_resolver.cpp	2014-09-09 14:02:52 +0000
+++ src/core/media/cover_art_resolver.cpp	2016-06-09 16:54:40 +0000
@@ -22,6 +22,6 @@
 {
     return [](const std::string&, const std::string&, const std::string&)
     {
-        return "file:///usr/share/unity/icons/album_missing.png";
+        return "file:///usr/lib/arm-linux-gnueabihf/unity-scopes/mediascanner-music/album_missing.svg";
     };
 }
 
=== modified file 'src/core/media/gstreamer/meta_data_extractor.h'
--- src/core/media/gstreamer/meta_data_extractor.h	2015-09-23 11:17:26 +0000
+++ src/core/media/gstreamer/meta_data_extractor.h	2016-06-09 16:54:40 +0000
@@ -24,6 +24,8 @@
 
 #include "bus.h"
 
+#include "core/media/logger/logger.h"
+
 #include <gst/gst.h>
 
 #include <exception>
@@ -72,7 +74,7 @@
             auto md = static_cast<media::Track::MetaData*>(user_data);
             std::stringstream ss;
 
-            switch(gst_tag_get_type(tag))
+            switch (gst_tag_get_type(tag))
             {
             case G_TYPE_BOOLEAN:
             {
@@ -137,8 +139,7 @@
                 break;
             }
 
-            (*md).set(
-                        (gstreamer_to_mpris_tag_lut().count(tag) > 0 ? gstreamer_to_mpris_tag_lut().at(tag) : tag),
+            (*md).set( (gstreamer_to_mpris_tag_lut().count(tag) > 0 ? gstreamer_to_mpris_tag_lut().at(tag) : tag),
                         ss.str());
         },
         &md);
 
=== modified file 'src/core/media/gstreamer/playbin.cpp'
--- src/core/media/gstreamer/playbin.cpp	2016-04-06 15:28:29 +0000
+++ src/core/media/gstreamer/playbin.cpp	2016-06-09 16:54:40 +0000
@@ -30,8 +30,6 @@
 
 #include <utility>
 
-//#define VERBOSE_DEBUG
-
 namespace
 {
 void setup_video_sink_for_buffer_streaming(GstElement* pipeline)
@@ -256,7 +254,7 @@
 
 void gstreamer::Playbin::on_new_message_async(const Bus::Message& message)
 {
-    switch(message.type)
+    switch (message.type)
     {
     case GST_MESSAGE_ERROR:
         signals.on_error(message.detail.error_warning_info);
 
=== added file 'src/core/media/metadata.cpp'
--- src/core/media/metadata.cpp	1970-01-01 00:00:00 +0000
+++ src/core/media/metadata.cpp	2016-06-09 16:54:40 +0000
@@ -0,0 +1,83 @@
+/*
+ * Copyright © 2016 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser General Public License version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Jim Hodapp <[e-mail]>
+ */
+
+#include "xesam.h"
+
+#include <core/media/track.h>
+
+namespace media = core::ubuntu::media;
+
+const std::string& media::Track::MetaData::album() const
+{
+    return map.at(xesam::Album::name);
+}
+
+const std::string& media::Track::MetaData::artist() const
+{
+    return map.at(xesam::Artist::name);
+}
+
+const std::string& media::Track::MetaData::title() const
+{
+    return map.at(xesam::Title::name);
+}
+
+const std::string& media::Track::MetaData::track_id() const
+{
+    return map.at(media::Track::MetaData::TrackIdKey);
+}
+
+const std::string& media::Track::MetaData::art_url() const
+{
+    return map.at(media::Track::MetaData::TrackArtlUrlKey);
+}
+
+const std::string& media::Track::MetaData::last_used() const
+{
+    return map.at(xesam::LastUsed::name);
+}
+
+void media::Track::MetaData::set_album(const std::string& album)
+{
+    map[xesam::Album::name] = album;
+}
+
+void media::Track::MetaData::set_artist(const std::string& artist)
+{
+    map[xesam::Artist::name] = artist;
+}
+
+void media::Track::MetaData::set_title(const std::string& title)
+{
+    map[xesam::Title::name] = title;
+}
+
+void media::Track::MetaData::set_track_id(const std::string& id)
+{
+    map[media::Track::MetaData::TrackIdKey] = id;
+}
+
+void media::Track::MetaData::set_art_url(const std::string& url)
+{
+    map[media::Track::MetaData::TrackArtlUrlKey] = url;
+}
+
+void media::Track::MetaData::set_last_used(const std::string& datetime)
+{
+    map[xesam::LastUsed::name] = datetime;
+}
 
=== modified file 'src/core/media/mpris/player.h'
--- src/core/media/mpris/player.h	2016-02-19 16:14:42 +0000
+++ src/core/media/mpris/player.h	2016-06-09 16:54:40 +0000
@@ -44,6 +44,8 @@
 
 #include <cstdint>
 
+#include "core/media/logger/logger.h"
+
 namespace dbus = core::dbus;
 
 namespace mpris
@@ -172,8 +174,7 @@
         DBUS_CPP_WRITABLE_PROPERTY_DEF(PlaybackRate, Player, double)
         DBUS_CPP_WRITABLE_PROPERTY_DEF(Rate, Player, double)
         DBUS_CPP_WRITABLE_PROPERTY_DEF(Shuffle, Player, bool)
-        DBUS_CPP_READABLE_PROPERTY_DEF(Metadata, Player, Dictionary)
-        DBUS_CPP_READABLE_PROPERTY_DEF(TypedMetaData, Player, core::ubuntu::media::Track::MetaData)
+        DBUS_CPP_READABLE_PROPERTY_DEF(Metadata, Player, core::ubuntu::media::Track::MetaData)
         DBUS_CPP_WRITABLE_PROPERTY_DEF(Volume, Player, double)
         DBUS_CPP_READABLE_PROPERTY_DEF(Position, Player, std::int64_t)
         DBUS_CPP_READABLE_PROPERTY_DEF(Duration, Player, std::int64_t)
@@ -222,7 +223,7 @@
                 Properties::TypedLoopStatus::ValueType typed_loop_status{core::ubuntu::media::Player::LoopStatus::none};
                 Properties::PlaybackRate::ValueType playback_rate{1.f};
                 Properties::Shuffle::ValueType shuffle{false};
-                Properties::TypedMetaData::ValueType typed_meta_data{};
+                Properties::Metadata::ValueType meta_data{};
                 Properties::Volume::ValueType volume{0.f};
                 Properties::Position::ValueType position{0};
                 Properties::Duration::ValueType duration{0};
@@ -253,7 +254,7 @@
                   configuration.object->template get_property<Properties::Lifetime>(),
                   configuration.object->template get_property<Properties::PlaybackRate>(),
                   configuration.object->template get_property<Properties::Shuffle>(),
-                  configuration.object->template get_property<Properties::TypedMetaData>(),
+                  configuration.object->template get_property<Properties::Metadata>(),
                   configuration.object->template get_property<Properties::Volume>(),
                   configuration.object->template get_property<Properties::Position>(),
                   configuration.object->template get_property<Properties::Duration>(),
@@ -288,6 +289,7 @@
             properties.lifetime->set(core::ubuntu::media::Player::Lifetime::normal);
             properties.playback_rate->set(configuration.defaults.playback_rate);
             properties.shuffle->set(configuration.defaults.shuffle);
+            properties.meta_data_for_current_track->set(configuration.defaults.meta_data);
             properties.position->set(configuration.defaults.position);
             properties.duration->set(configuration.defaults.duration);
             properties.minimum_playback_rate->set(configuration.defaults.minimum_rate);
@@ -351,9 +353,9 @@
             Dictionary dict; dict[Property::name()] = dbus::types::Variant::encode(value);
 
             signals.properties_changed->emit(std::make_tuple(
-                            dbus::traits::Service<Player>::interface_name(),
-                            dict,
-                            the_empty_list_of_invalidated_properties()));
+                        dbus::traits::Service<Player>::interface_name(),
+                        dict,
+                        the_empty_list_of_invalidated_properties()));
         }
 
         Dictionary get_all_properties()
@@ -374,6 +376,7 @@
             dict[Properties::Lifetime::name()] = dbus::types::Variant::encode(properties.lifetime->get());
             dict[Properties::PlaybackRate::name()] = dbus::types::Variant::encode(properties.playback_rate->get());
             dict[Properties::Shuffle::name()] = dbus::types::Variant::encode(properties.shuffle->get());
+            dict[Properties::Metadata::name()] = dbus::types::Variant::encode(properties.meta_data_for_current_track->get());
             dict[Properties::Duration::name()] = dbus::types::Variant::encode(properties.duration->get());
             dict[Properties::Position::name()] = dbus::types::Variant::encode(properties.position->get());
             dict[Properties::MinimumRate::name()] = dbus::types::Variant::encode(properties.minimum_playback_rate->get());
@@ -405,7 +408,7 @@
             std::shared_ptr<core::dbus::Property<Properties::Lifetime>> lifetime;
             std::shared_ptr<core::dbus::Property<Properties::PlaybackRate>> playback_rate;
             std::shared_ptr<core::dbus::Property<Properties::Shuffle>> shuffle;
-            std::shared_ptr<core::dbus::Property<Properties::TypedMetaData>> typed_meta_data_for_current_track;
+            std::shared_ptr<core::dbus::Property<Properties::Metadata>> meta_data_for_current_track;
             std::shared_ptr<core::dbus::Property<Properties::Volume>> volume;
             std::shared_ptr<core::dbus::Property<Properties::Position>> position;
             std::shared_ptr<core::dbus::Property<Properties::Duration>> duration;
 
=== modified file 'src/core/media/player_implementation.cpp'
--- src/core/media/player_implementation.cpp	2016-05-03 19:08:00 +0000
+++ src/core/media/player_implementation.cpp	2016-06-09 16:54:40 +0000
@@ -24,6 +24,7 @@
 #include "util/timeout.h"
 
 #include <unistd.h>
+#include <ctime>
 
 #include "client_death_observer.h"
 #include "engine.h"
@@ -138,9 +139,19 @@
             }
             case Engine::State::playing:
             {
-                // We update the track meta data prior to updating the playback status.
+                // We update the track metadata prior to updating the playback status.
                 // Some MPRIS clients expect this order of events.
-                parent->meta_data_for_current_track().set(std::get<1>(engine->track_meta_data().get()));
+                time_t now;
+                time(&now);
+                char buf[sizeof("2011-10-08T07:07:09Z")];
+                strftime(buf, sizeof(buf), "%FT%TZ", gmtime(&now));
+                media::Track::MetaData metadata{std::get<1>(engine->track_meta_data().get())};
+                // Setting this with second resolution makes sure that the track_meta_data property changes
+                // and thus the track_meta_data().changed() signal gets sent out over dbus. Otherwise the
+                // Property caching mechanism would prevent this.
+                metadata.set_last_used(std::string{buf});
+                update_mpris_metadata(metadata);
+
                 // And update our playback status.
                 parent->playback_status().set(media::Player::playing);
                 MH_INFO("Requesting power state");
@@ -325,6 +336,27 @@
         parent->can_go_next().set(has_next);
     }
 
+    // Makes sure all relevant metadata fields are set to current data and
+    // will trigger the track_meta_data().changed() signal to go out over dbus
+    void update_mpris_metadata(const media::Track::MetaData& md)
+    {
+        media::Track::MetaData metadata{md};
+        if (not metadata.is_set(media::Track::MetaData::TrackIdKey))
+        {
+            const std::size_t last_slash = track_list->current().find_last_of("/");
+            const std::string track_id = track_list->current().substr(last_slash+1);
+            if (not track_id.empty())
+                metadata.set_track_id("/org/mpris/MediaPlayer2/Track/" + track_id);
+            else
+                MH_WARNING("Failed to set MPRIS track id since the id value is NULL");
+        }
+        // TODO: This needs to be extracted from GStreamer and dynamically set
+        if (not metadata.is_set(media::Track::MetaData::TrackArtlUrlKey))
+            metadata.set_art_url("file:///usr/lib/arm-linux-gnueabihf/unity-scopes/mediascanner-music/album_missing.svg");
+
+        parent->meta_data_for_current_track().set(metadata);
+    }
+
     bool pause_other_players(media::Player::PlayerKey key)
     {
         if (not config.parent.player_service)
@@ -501,6 +533,12 @@
         d->engine->lifetime().set(lifetime);
     });
 
+    d->engine->track_meta_data().changed().connect([this, config](
+           const std::tuple<media::Track::UriType, media::Track::MetaData>& md)
+    {
+        d->update_mpris_metadata(std::get<1>(md));
+    });
+
     d->engine->about_to_finish_signal().connect([this]()
     {
         if (d->doing_abandon)
 
=== modified file 'src/core/media/player_skeleton.cpp'
--- src/core/media/player_skeleton.cpp	2016-04-06 15:28:29 +0000
+++ src/core/media/player_skeleton.cpp	2016-06-09 16:54:40 +0000
@@ -478,7 +478,7 @@
 
 const core::Property<media::Track::MetaData>& media::PlayerSkeleton::meta_data_for_current_track() const
 {
-    return *d->skeleton.properties.typed_meta_data_for_current_track;
+    return *d->skeleton.properties.meta_data_for_current_track;
 }
 
 const core::Property<media::Player::Volume>& media::PlayerSkeleton::volume() const
@@ -608,7 +608,7 @@
 
 core::Property<media::Track::MetaData>& media::PlayerSkeleton::meta_data_for_current_track()
 {
-    return *d->skeleton.properties.typed_meta_data_for_current_track;
+    return *d->skeleton.properties.meta_data_for_current_track;
 }
 
 core::Property<media::Player::PlaybackRate>& media::PlayerSkeleton::minimum_playback_rate()
 
=== modified file 'src/core/media/player_stub.cpp'
--- src/core/media/player_stub.cpp	2016-04-06 15:28:29 +0000
+++ src/core/media/player_stub.cpp	2016-06-09 16:54:40 +0000
@@ -70,7 +70,7 @@
                     object->get_property<mpris::Player::Properties::TypedLoopStatus>(),
                     object->get_property<mpris::Player::Properties::PlaybackRate>(),
                     object->get_property<mpris::Player::Properties::Shuffle>(),
-                    object->get_property<mpris::Player::Properties::TypedMetaData>(),
+                    object->get_property<mpris::Player::Properties::Metadata>(),
                     object->get_property<mpris::Player::Properties::Volume>(),
                     object->get_property<mpris::Player::Properties::Position>(),
                     object->get_property<mpris::Player::Properties::Duration>(),
@@ -118,7 +118,7 @@
         std::shared_ptr<core::dbus::Property<mpris::Player::Properties::TypedLoopStatus>> loop_status;
         std::shared_ptr<core::dbus::Property<mpris::Player::Properties::PlaybackRate>> playback_rate;
         std::shared_ptr<core::dbus::Property<mpris::Player::Properties::Shuffle>> shuffle;
-        std::shared_ptr<core::dbus::Property<mpris::Player::Properties::TypedMetaData>> meta_data_for_current_track;
+        std::shared_ptr<core::dbus::Property<mpris::Player::Properties::Metadata>> meta_data_for_current_track;
         std::shared_ptr<core::dbus::Property<mpris::Player::Properties::Volume>> volume;
         std::shared_ptr<core::dbus::Property<mpris::Player::Properties::Position>> position;
         std::shared_ptr<core::dbus::Property<mpris::Player::Properties::Duration>> duration;
 
=== modified file 'src/core/media/service_skeleton.cpp'
--- src/core/media/service_skeleton.cpp	2016-05-17 17:51:16 +0000
+++ src/core/media/service_skeleton.cpp	2016-06-09 16:54:40 +0000
@@ -732,6 +732,21 @@
                 player.properties.can_go_next->set(can_go_next);
             });
 
+            // NOTE: the metadata first gets updated in the PlayerImplementation constructor which connects
+            // and reacts to the Engine track_meta_data changed signal. Setting the
+            // meta_data_for_current_track here makes sure that the signal uses the MPRIS object path
+            connections.meta_data_changed = player_sp->meta_data_for_current_track().changed().connect(
+                    [this](const media::Track::MetaData& metadata)
+            {
+                player.properties.meta_data_for_current_track->set(metadata);
+                mpris::Player::Dictionary dict; dict[mpris::Player::Properties::Metadata::name()]
+                        = dbus::types::Variant::encode(metadata);
+                player.signals.properties_changed->emit(std::make_tuple(
+                                dbus::traits::Service<mpris::Player>::interface_name(),
+                                dict,
+                                mpris::Player::Skeleton::the_empty_list_of_invalidated_properties()));
+            });
+
             // 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
@@ -748,43 +763,6 @@
             player.properties.can_pause->set(player_sp->can_pause().get());
             player.properties.can_go_previous->set(player_sp->can_go_previous().get());
             player.properties.can_go_next->set(player_sp->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 reset_current_player()
@@ -807,6 +785,7 @@
             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([](){});
+            connections.meta_data_changed = the_empty_signal.connect([](){});
         }
 
         bool is_current_player(media::Player::PlayerKey key)