Merge lp:~thomas-voss/media-hub/decouple-service-skeleton-and-implementation into lp:media-hub

[Thomas Voß]

Decouple the ServiceSkeleton from the ServiceImplementation by introducing a common interface media::KeyedPlayerStore for storing running Player sessions indexed by the Player::Key.
Provide a default implementation HashedKeyedPlayerStore relying on a hash map for keeping track of player instances.
Adjust implementation to account for ServiceImplementation no longer inheriting from ServiceSkeleton.

Decouple the ServiceSkeleton from the ServiceImplementation by introducing a common interface media::KeyedPlayerStore for storing running Player sessions indexed by the Player::Key.
Provide a default implementation HashedKeyedPlayerStore relying on a hash map for keeping track of player instances.
Adjust implementation to account for ServiceImplementation no longer inheriting from ServiceSkeleton.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:112
http://jenkins.qa.ubuntu.com/job/media-hub-ci/195/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-amd64-ci/35/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-armhf-ci/35/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-i386-ci/35/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/media-hub-ci/195/rebuild

review: Needs Fixing (continuous-integration)

[Jim Hodapp (jhodapp)]

One simple change listed below.

review: Needs Fixing (code)

[lp:~thomas-voss/media-hub/decouple-service-skeleton-and-implementation]

113. By Thomas Voß on 2014-12-15

[ Jim Hodapp ]
* Resubmitting with prerequisite branch (LP: #1331041)
[ Justin McPherson ]
* Resubmitting with prerequisite branch (LP: #1331041)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:113
http://jenkins.qa.ubuntu.com/job/media-hub-ci/214/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-amd64-ci/54
    SUCCESS: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-armhf-ci/54
        deb: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-armhf-ci/54/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-i386-ci/54

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/media-hub-ci/214/rebuild

review: Approve (continuous-integration)

[lp:~thomas-voss/media-hub/decouple-service-skeleton-and-implementation]

114. By Thomas Voß on 2015-01-26

[ Jim Hodapp ]
* Error reporting all the way up to the app level from the playbin
  pipeline.
[ Ubuntu daily release ]
* New rebuild forced
[ Jim Hodapp ]
* Don't auto-resume playback of videos after a phone call ends. (LP:
  #1411273)
[ Ubuntu daily release ]
* New rebuild forced
[ Ricardo Salveti de Araujo ]
* service_implementation: adding debug for call started/ended signals.
  Make sure account and connection are available when setting up
  account manager (patch from Gustavo Boiko). call_monitor: don't
  check caps when hooking up on/off signals, until bug 1409125 is
  fixed. Enable parallel building . (LP: #1409125)
[ Jim Hodapp ]
* Pause playback when recording begins. (LP: #1398047)
[ Ricardo Salveti de Araujo ]
* call_monitor.cpp: waiting for bridge to be up, and also protecting
  the on_change call (LP: #1408137)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:114
http://jenkins.qa.ubuntu.com/job/media-hub-ci/241/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-amd64-ci/81/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-armhf-ci/81
        deb: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-armhf-ci/81/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-i386-ci/81/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/media-hub-ci/241/rebuild

review: Needs Fixing (continuous-integration)

[Francis Ginther (fginther)]

The jenkins node for the amd64 build failed, I've restarted a new ci run.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:114
http://jenkins.qa.ubuntu.com/job/media-hub-ci/246/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-amd64-ci/86/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-armhf-ci/86
        deb: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-armhf-ci/86/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-i386-ci/86/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/media-hub-ci/246/rebuild

review: Needs Fixing (continuous-integration)

[lp:~thomas-voss/media-hub/decouple-service-skeleton-and-implementation]

115. By Thomas Voß on 2015-01-29

Merge prereq branch.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:115
http://jenkins.qa.ubuntu.com/job/media-hub-ci/251/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-amd64-ci/91
    SUCCESS: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-armhf-ci/91
        deb: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-armhf-ci/91/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-i386-ci/91

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/media-hub-ci/251/rebuild

review: Approve (continuous-integration)

[lp:~thomas-voss/media-hub/decouple-service-skeleton-and-implementation]

116. By Thomas Voß on 2015-02-24

Merge prereq branch.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:116
http://jenkins.qa.ubuntu.com/job/media-hub-ci/279/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-amd64-ci/119
    SUCCESS: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-armhf-ci/119
        deb: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-armhf-ci/119/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-i386-ci/119

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/media-hub-ci/279/rebuild

review: Approve (continuous-integration)

[lp:~thomas-voss/media-hub/decouple-service-skeleton-and-implementation]

117. By Thomas Voß on 2015-03-04

Merge prereq branch.

118. By Thomas Voß on 2015-03-12

* debian/control:
  - Removing pre-depends that are not required
  - Bumping standards-version to 3.9.6
[ Ricardo Salveti de Araujo ]
* Migrating tests to use ogg instead of mp3/avi removed:
  tests/h264.avi tests/test.mp3 added: tests/test-audio-1.ogg
  tests/test-video.ogg tests/test.mp3 renamed: tests/test.ogg =>
  tests/test-audio.ogg

Unmerged revisions

=== modified file 'src/core/media/CMakeLists.txt'
--- src/core/media/CMakeLists.txt	2015-03-04 16:37:33 +0000
+++ src/core/media/CMakeLists.txt	2015-03-04 16:37:33 +0000
@@ -87,7 +87,8 @@
     ${MPRIS_HEADERS}
 
     client_death_observer.cpp
-    hybris_client_death_observer.cpp
+    hashed_keyed_player_store.cpp
+    hybris_client_death_observer.cpp    
     cover_art_resolver.cpp
     engine.cpp
 

=== added file 'src/core/media/hashed_keyed_player_store.cpp'
--- src/core/media/hashed_keyed_player_store.cpp	1970-01-01 00:00:00 +0000
+++ src/core/media/hashed_keyed_player_store.cpp	2015-03-04 16:37:33 +0000
@@ -0,0 +1,80 @@
+/*
+ * Copyright © 2014 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: Thomas Voß <[e-mail]>
+ */
+
+#include <core/media/hashed_keyed_player_store.h>
+
+namespace media = core::ubuntu::media;
+
+media::HashedKeyedPlayerStore::HashedKeyedPlayerStore()
+{
+}
+
+const core::Property<std::shared_ptr<media::Player>>& media::HashedKeyedPlayerStore::current_player() const
+{
+    return prop_current_player;
+}
+
+bool media::HashedKeyedPlayerStore::has_player_for_key(const media::Player::PlayerKey& key) const
+{
+    std::lock_guard<std::mutex> lg{guard};
+    return map.count(key) > 0;
+}
+
+std::shared_ptr<media::Player> media::HashedKeyedPlayerStore::player_for_key(const media::Player::PlayerKey& key) const
+{
+    std::lock_guard<std::mutex> lg{guard};
+    auto it = map.find(key);
+
+    if (it == map.end()) throw std::out_of_range
+    {
+        "HashedKeyedPlayerStore::player_for_key: No player known for " + std::to_string(key)
+    };
+
+    return it->second;
+}
+
+void media::HashedKeyedPlayerStore::enumerate_players(const media::KeyedPlayerStore::PlayerEnumerator& enumerator) const
+{
+    std::lock_guard<std::mutex> lg{guard};
+    for (const auto& pair : map)
+        enumerator(pair.first, pair.second);
+}
+
+void media::HashedKeyedPlayerStore::add_player_for_key(const media::Player::PlayerKey& key, const std::shared_ptr<media::Player>& player)
+{
+    std::lock_guard<std::mutex> lg{guard};
+    map[key] = player;
+}
+
+void media::HashedKeyedPlayerStore::remove_player_for_key(const media::Player::PlayerKey& key)
+{
+    std::lock_guard<std::mutex> lg{guard};
+    auto it = map.find(key);
+    if (it != map.end())
+    {
+        if (prop_current_player == it->second)
+            prop_current_player = nullptr;
+
+        map.erase(it);
+    }
+}
+
+void media::HashedKeyedPlayerStore::set_current_player_for_key(const media::Player::PlayerKey& key)
+{
+    prop_current_player = player_for_key(key);
+}

=== added file 'src/core/media/hashed_keyed_player_store.h'
--- src/core/media/hashed_keyed_player_store.h	1970-01-01 00:00:00 +0000
+++ src/core/media/hashed_keyed_player_store.h	2015-03-04 16:37:33 +0000
@@ -0,0 +1,76 @@
+/*
+ * Copyright © 2014 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: Thomas Voß <[e-mail]>
+ */
+
+#ifndef CORE_UBUNTU_MEDIA_HASHED_KEYED_PLAYER_STORE_H_
+#define CORE_UBUNTU_MEDIA_HASHED_KEYED_PLAYER_STORE_H_
+
+#include <core/media/keyed_player_store.h>
+
+#include <mutex>
+#include <unordered_map>
+
+namespace core
+{
+namespace ubuntu
+{
+namespace media
+{
+// Implements KeyedPlayerStore using a std::unordered_map.
+class HashedKeyedPlayerStore : public KeyedPlayerStore
+{
+public:
+    HashedKeyedPlayerStore();
+    // We keep track of the "current" player, that is, the one
+    // that has been created most recently, or has been explicitly foregrounded, or has been enabled for
+    // background playback. We provide a getable/observable access to that designated instance.
+    const core::Property<std::shared_ptr<media::Player>>& current_player() const override;
+
+    // We keep track of all known player sessions here and render them accessible via
+    // the key. All of these functions are thread-safe but not reentrant.
+    // Returns true iff a player is known for the given key.
+    bool has_player_for_key(const Player::PlayerKey& key) const override;
+
+    // Returns the player for the given key or throws std::out_of_range if no player is known
+    // for the given key.
+    // Throws std::out_of_range if no player is known for the key.
+    std::shared_ptr<Player> player_for_key(const Player::PlayerKey& key) const override;
+
+    // Enumerates all known players and invokes the given enumerator for each
+    // (key, player) pair.
+    void enumerate_players(const PlayerEnumerator& enumerator) const override;
+
+    // Adds the given player with the given key.
+    void add_player_for_key(const Player::PlayerKey& key, const std::shared_ptr<Player>& player) override;
+
+    // Removes the player for the given key, and unsets it if it is the current one.
+    void remove_player_for_key(const Player::PlayerKey& key) override;
+
+    // Makes the player known under the given key current.
+    // Throws std::out_of_range if no player is known for the key.
+    void set_current_player_for_key(const Player::PlayerKey& key) override;
+
+private:
+    core::Property<std::shared_ptr<Player>> prop_current_player;
+    mutable std::mutex guard;
+    std::unordered_map<Player::PlayerKey, std::shared_ptr<Player>> map;
+};
+}
+}
+}
+
+#endif // CORE_UBUNTU_MEDIA_KEYED_PLAYER_STORE_H_

=== added file 'src/core/media/keyed_player_store.cpp'
=== added file 'src/core/media/keyed_player_store.h'
--- src/core/media/keyed_player_store.h	1970-01-01 00:00:00 +0000
+++ src/core/media/keyed_player_store.h	2015-03-04 16:37:33 +0000
@@ -0,0 +1,83 @@
+/*
+ * Copyright © 2014 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: Thomas Voß <[e-mail]>
+ */
+
+#ifndef CORE_UBUNTU_MEDIA_KEYED_PLAYER_STORE_H_
+#define CORE_UBUNTU_MEDIA_KEYED_PLAYER_STORE_H_
+
+#include <core/media/player.h>
+
+#include <core/property.h>
+
+#include <functional>
+#include <memory>
+
+namespace core
+{
+namespace ubuntu
+{
+namespace media
+{
+// An interface abstracting keyed lookups of known Player instances.
+class KeyedPlayerStore
+{
+public:
+    // Save us some typing.
+    typedef std::shared_ptr<KeyedPlayerStore> Ptr;
+    // Functor for enumerating all known (key, player) pairs.
+    typedef std::function
+    <
+        void(
+            // The key of the player.
+            const Player::PlayerKey&,
+            // The actual player instance.
+            const std::shared_ptr<Player>&
+        )
+    > PlayerEnumerator;
+    // We keep track of the "current" player, that is, the one
+    // that has been created most recently and provide a getable/observable
+    // access to that designated instance.
+    virtual const core::Property<std::shared_ptr<Player>>& current_player() const = 0;
+
+    // We keep track of all known player sessions here and render them accessible via
+    // the key. All of these functions are thread-safe but not reentrant.
+    // Returns true iff a player is known for the given key.
+    virtual bool has_player_for_key(const Player::PlayerKey& key) const = 0;
+    // Returns the player for the given key or throws std::out_of_range if no player is known
+    // for the given key.
+    virtual std::shared_ptr<Player> player_for_key(const Player::PlayerKey& key) const = 0;
+    // Enumerates all known players and invokes the given enumerator for each
+    // (key, player) pair.
+    virtual void enumerate_players(const PlayerEnumerator& enumerator) const = 0;
+    // Adds the given player with the given key.
+    virtual void add_player_for_key(const Player::PlayerKey& key, const std::shared_ptr<Player>& player) = 0;
+    // Removes the player for the given key, and unsets it if it is the current one.
+    virtual void remove_player_for_key(const Player::PlayerKey& key) = 0;
+    // Makes the player known under the given key current.
+    virtual void set_current_player_for_key(const Player::PlayerKey& key) = 0;
+
+protected:
+    KeyedPlayerStore() = default;
+    KeyedPlayerStore(const KeyedPlayerStore&) = delete;
+    virtual ~KeyedPlayerStore() = default;
+    KeyedPlayerStore& operator=(const KeyedPlayerStore&) = delete;
+};
+}
+}
+}
+
+#endif // CORE_UBUNTU_MEDIA_KEYED_PLAYER_STORE_H_

=== modified file 'src/core/media/server/server.cpp'
--- src/core/media/server/server.cpp	2015-03-04 16:37:33 +0000
+++ src/core/media/server/server.cpp	2015-03-04 16:37:33 +0000
@@ -20,6 +20,7 @@
 #include <core/media/player.h>
 #include <core/media/track_list.h>
 
+#include "core/media/hashed_keyed_player_store.h"
 #include "core/media/service_implementation.h"
 
 #include <core/posix/signal.h>
@@ -103,23 +104,37 @@
         }
     };
 
+    // Our common player store instance for tracking player instances.
+    auto player_store = std::make_shared<media::HashedKeyedPlayerStore>();
     // We assemble the configuration for executing the service now.
     media::ServiceImplementation::Configuration service_config
     {
+        std::make_shared<media::HashedKeyedPlayerStore>(),
         external_services
     };
 
-    auto service = std::make_shared<media::ServiceImplementation>(service_config);
+    auto impl = std::make_shared<media::ServiceImplementation>(media::ServiceImplementation::Configuration
+    {
+        player_store,
+        external_services
+    });
+
+    auto skeleton = std::make_shared<media::ServiceSkeleton>(media::ServiceSkeleton::Configuration
+    {
+        impl,
+        player_store,
+
+    });
 
     std::thread service_worker
     {
-        [&shutdown_requested, service]()
+        [&shutdown_requested, skeleton]()
         {
             while (not shutdown_requested)
             {
                 try
                 {
-                    service->run();
+                    skeleton->run();
                 }
                 catch (const std::exception& e)
                 {
@@ -142,7 +157,7 @@
     shutdown_requested = true;
 
     // And stop execution of helper and actual service.
-    service->stop();
+    skeleton->stop();
 
     if (service_worker.joinable())
         service_worker.join();

=== modified file 'src/core/media/service_implementation.cpp'
--- src/core/media/service_implementation.cpp	2015-03-04 16:37:33 +0000
+++ src/core/media/service_implementation.cpp	2015-03-04 16:37:33 +0000
@@ -188,11 +188,11 @@
         // until all dispatches are done
         d->configuration.external_services.io_service.post([this, key]()
         {
-            if (!has_player_for_key(key))
+            if (!d->configuration.player_store->has_player_for_key(key))
                 return;
 
-            if (player_for_key(key)->lifetime() == Player::Lifetime::normal)
-                remove_player_for_key(key);
+            if (d->configuration.player_store->player_for_key(key)->lifetime() == Player::Lifetime::normal)
+                d->configuration.player_store->remove_player_for_key(key);
         });
     });
 
@@ -213,19 +213,19 @@
 
 void media::ServiceImplementation::pause_other_sessions(media::Player::PlayerKey key)
 {
-    if (not has_player_for_key(key))
+    if (not d->configuration.player_store->has_player_for_key(key))
     {
         cerr << "Could not find Player by key: " << key << endl;
         return;
     }
 
-    auto current_player = player_for_key(key);
+    auto current_player = d->configuration.player_store->player_for_key(key);
 
     // We immediately make the player known as new current player.
     if (current_player->audio_stream_role() == media::Player::multimedia)
-        set_current_player_for_key(key);
+        d->configuration.player_store->set_current_player_for_key(key);
 
-    enumerate_players([current_player, key](const media::Player::PlayerKey& other_key, const std::shared_ptr<media::Player>& other_player)
+    d->configuration.player_store->enumerate_players([current_player, key](const media::Player::PlayerKey& other_key, const std::shared_ptr<media::Player>& other_player)
     {
         // Only pause a Player if all of the following criteria are met:
         // 1) currently playing
@@ -245,7 +245,7 @@
 
 void media::ServiceImplementation::pause_all_multimedia_sessions()
 {
-    enumerate_players([this](const media::Player::PlayerKey& key, const std::shared_ptr<media::Player>& player)
+    d->configuration.player_store->enumerate_players([this](const media::Player::PlayerKey& key, const std::shared_ptr<media::Player>& player)
                       {
                           if (player->playback_status() == Player::playing
                               && player->audio_stream_role() == media::Player::multimedia)
@@ -260,7 +260,7 @@
 void media::ServiceImplementation::resume_paused_multimedia_sessions(bool resume_video_sessions)
 {
     std::for_each(d->paused_sessions.begin(), d->paused_sessions.end(), [this, resume_video_sessions](const media::Player::PlayerKey& key) {
-            auto player = player_for_key(key);
+            auto player = d->configuration.player_store->player_for_key(key);
             // Only resume video playback if explicitly desired
             if (resume_video_sessions || player->is_audio_source())
                 player->play();
@@ -273,10 +273,10 @@
 
 void media::ServiceImplementation::resume_multimedia_session()
 {
-    if (not has_player_for_key(d->resume_key))
+    if (not d->configuration.player_store->has_player_for_key(d->resume_key))
         return;
 
-    auto player = player_for_key(d->resume_key);
+    auto player = d->configuration.player_store->player_for_key(d->resume_key);
 
     if (player->playback_status() == Player::paused)
     {

=== modified file 'src/core/media/service_implementation.h'
--- src/core/media/service_implementation.h	2015-03-04 16:37:33 +0000
+++ src/core/media/service_implementation.h	2015-03-04 16:37:33 +0000
@@ -30,12 +30,13 @@
 {
 class Player;
 
-class ServiceImplementation : public ServiceSkeleton
+class ServiceImplementation : public Service
 {
 public:
     // All creation time arguments go here.
     struct Configuration
     {
+        KeyedPlayerStore::Ptr player_store;
         helper::ExternalServices& external_services;
     };
 

=== modified file 'src/core/media/service_skeleton.cpp'
--- src/core/media/service_skeleton.cpp	2015-03-04 16:37:33 +0000
+++ src/core/media/service_skeleton.cpp	2015-03-04 16:37:33 +0000
@@ -49,11 +49,12 @@
 
 struct media::ServiceSkeleton::Private
 {
-    Private(media::ServiceSkeleton* impl, const media::CoverArtResolver& resolver)
+    Private(media::ServiceSkeleton* impl, const ServiceSkeleton::Configuration& config)
         : impl(impl),
           object(impl->access_service()->add_object_for_path(
-                     dbus::traits::Service<media::Service>::object_path())),
-          exported(impl->access_bus(), resolver)
+                     dbus::traits::Service<media::Service>::object_path())),          
+          exported(impl->access_bus(), config.cover_art_resolver),
+          configuration(config)
     {
         object->install_method_handler<mpris::Service::CreateSession>(
                     std::bind(
@@ -105,15 +106,7 @@
 
         try
         {
-            auto session = impl->create_session(config);
-
-            bool inserted = false;
-            std::tie(std::ignore, inserted)
-                    = session_store.insert(std::make_pair(key, session));
-
-            if (!inserted)
-                throw std::runtime_error("Problem persisting session in session store.");
-
+            configuration.player_store->add_player_for_key(key, impl->create_session(config));
             auto reply = dbus::Message::make_method_return(msg);
             reply->writer() << op;
 
@@ -135,7 +128,7 @@
             std::string name;
             msg->reader() >> name;
 
-            if (fixed_session_store.count(name) == 0) {
+            if (named_player_map.count(name) == 0) {
                 // Create new session
                 auto  session_info = create_session_info();
 
@@ -152,14 +145,10 @@
                 auto session = impl->create_session(config);
                 session->lifetime().set(media::Player::Lifetime::resumable);
 
-                bool inserted = false;
-                std::tie(std::ignore, inserted)
-                        = session_store.insert(std::make_pair(key, session));
-
-                if (!inserted)
-                    throw std::runtime_error("Problem persisting session in session store.");
-
-                fixed_session_store.insert(std::make_pair(name, key));
+                configuration.player_store->add_player_for_key(key, session);
+
+
+                named_player_map.insert(std::make_pair(name, key));
 
                 auto reply = dbus::Message::make_method_return(msg);
                 reply->writer() << op;
@@ -168,8 +157,8 @@
             }
             else {
                 // Resume previous session
-                auto key = fixed_session_store[name];
-                if (session_store.count(key) == 0) {
+                auto key = named_player_map.at(name);
+                if (not configuration.player_store->has_player_for_key(key)) {
                     auto reply = dbus::Message::make_error(
                                 msg,
                                 mpris::Service::Errors::CreatingFixedSession::name(),
@@ -204,7 +193,7 @@
             Player::PlayerKey key;
             msg->reader() >> key;
 
-            if (session_store.count(key) == 0) {
+            if (not configuration.player_store->has_player_for_key(key)) {
                 auto reply = dbus::Message::make_error(
                             msg,
                             mpris::Service::Errors::ResumingSession::name(),
@@ -245,9 +234,10 @@
     media::ServiceSkeleton* impl;
     dbus::Object::Ptr object;
 
-    // We track all running player instances.
-    std::map<media::Player::PlayerKey, std::shared_ptr<media::Player>> session_store;
-    std::map<std::string, media::Player::PlayerKey> fixed_session_store;
+    // We remember all our creation time arguments.
+    ServiceSkeleton::Configuration configuration;
+    // We map named/fixed player instances to their respective keys.
+    std::map<std::string, media::Player::PlayerKey> named_player_map;
     // We expose the entire service as an MPRIS player.
     struct Exported
     {
@@ -481,7 +471,7 @@
         mpris::Player::Skeleton player;
         mpris::Playlists::Skeleton playlists;
 
-        // Helper to resolve (title, artist, album) tuples to cover art.
+        // The CoverArtResolver used by the exported player.
         media::CoverArtResolver cover_art_resolver;
         // The actual player instance.
         std::weak_ptr<media::Player> current_player;
@@ -516,9 +506,9 @@
     } exported;
 };
 
-media::ServiceSkeleton::ServiceSkeleton(const media::CoverArtResolver& resolver)
+media::ServiceSkeleton::ServiceSkeleton(const Configuration& configuration)
     : dbus::Skeleton<media::Service>(the_session_bus()),
-      d(new Private(this, resolver))
+      d(new Private(this, configuration))
 {
 }
 
@@ -526,46 +516,24 @@
 {
 }
 
-bool media::ServiceSkeleton::has_player_for_key(const media::Player::PlayerKey& key) const
-{
-    return d->session_store.count(key) > 0;
-}
-
-std::shared_ptr<media::Player> media::ServiceSkeleton::player_for_key(const media::Player::PlayerKey& key) const
-{
-    return d->session_store.at(key);
-}
-
-void media::ServiceSkeleton::enumerate_players(const media::ServiceSkeleton::PlayerEnumerator& enumerator) const
-{
-    for (const auto& pair : d->session_store)
-        enumerator(pair.first, pair.second);
-}
-
-void media::ServiceSkeleton::set_current_player_for_key(const media::Player::PlayerKey& key)
-{
-    if (not has_player_for_key(key))
-        return;
-
-    d->exported.set_current_player(player_for_key(key));
-}
-
-void media::ServiceSkeleton::remove_player_for_key(const media::Player::PlayerKey& key)
-{
-    if (not has_player_for_key(key))
-        return;
-
-    auto player = player_for_key(key);
-
-    d->session_store.erase(key);
-    d->exported.unset_if_current(player);
-    // All non-durable fixed sessions are also removed
-    for (auto it: d->fixed_session_store) {
-        if (it.second == key) {
-            d->fixed_session_store.erase(it.first);
-            break;
-        }
-    }
+std::shared_ptr<media::Player> media::ServiceSkeleton::create_session(const media::Player::Configuration& config)
+{
+    return d->configuration.impl->create_session(config);
+}
+
+std::shared_ptr<media::Player> media::ServiceSkeleton::create_fixed_session(const std::string& name, const media::Player::Configuration&config)
+{
+    return d->configuration.impl->create_fixed_session(name, config);
+}
+
+std::shared_ptr<media::Player> media::ServiceSkeleton::resume_session(media::Player::PlayerKey key)
+{
+    return d->configuration.impl->resume_session(key);
+}
+
+void media::ServiceSkeleton::pause_other_sessions(media::Player::PlayerKey key)
+{
+    d->configuration.impl->pause_other_sessions(key);
 }
 
 void media::ServiceSkeleton::run()

=== modified file 'src/core/media/service_skeleton.h'
--- src/core/media/service_skeleton.h	2014-09-09 21:27:29 +0000
+++ src/core/media/service_skeleton.h	2015-03-04 16:37:33 +0000
@@ -22,6 +22,7 @@
 #include <core/media/service.h>
 
 #include "cover_art_resolver.h"
+#include "keyed_player_store.h"
 #include "service_traits.h"
 
 #include <core/dbus/skeleton.h>
@@ -37,34 +38,22 @@
 class ServiceSkeleton : public core::dbus::Skeleton<core::ubuntu::media::Service>
 {
 public:
-    // Functor for enumerating all known (key, player) pairs.
-    typedef std::function
-    <
-        void(
-            // The key of the player.
-            const core::ubuntu::media::Player::PlayerKey&,
-            // The actual player instance.
-            const std::shared_ptr<core::ubuntu::media::Player>&
-        )
-    > PlayerEnumerator;
+    // Creation time arguments go here.
+    struct Configuration
+    {
+        std::shared_ptr<Service> impl;
+        KeyedPlayerStore::Ptr player_store;
+        CoverArtResolver cover_art_resolver;
+    };
 
-    ServiceSkeleton(const CoverArtResolver& cover_art_resolver = always_missing_cover_art_resolver());
+    ServiceSkeleton(const Configuration& configuration);
     ~ServiceSkeleton();
 
-    // We keep track of all known player sessions here and render them accessible via
-    // the key. All of these functions are thread-safe but not reentrant.
-    // Returns true iff a player is known for the given key.
-    bool has_player_for_key(const Player::PlayerKey& key) const;
-    // Returns the player for the given key or throws std::out_of_range if no player is known
-    // for the given key.
-    std::shared_ptr<Player> player_for_key(const Player::PlayerKey& key) const;
-    // Enumerates all known players and invokes the given enumerator for each
-    // (key, player) pair.
-    void enumerate_players(const PlayerEnumerator& enumerator) const;
-    // Removes the player for the given key, and unsets it if it is the current one.
-    void remove_player_for_key(const Player::PlayerKey& key);
-    // Makes the player known under the given key current.
-    void set_current_player_for_key(const Player::PlayerKey& key);
+    // From media::Service
+    std::shared_ptr<Player> create_session(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);
+    void pause_other_sessions(Player::PlayerKey key);
 
     void run();
     void stop();