[Merge] lp:~thomas-voss/media-hub/decouple-service-skeleton-and-implementation into lp:media-hub
Jim Hodapp
jim.hodapp at canonical.com
Mon Dec 8 16:40:20 UTC 2014
Review: Needs Fixing code
One simple change listed below.
Diff comments:
> === modified file 'src/core/media/CMakeLists.txt'
> --- src/core/media/CMakeLists.txt 2014-12-01 13:20:31 +0000
> +++ src/core/media/CMakeLists.txt 2014-12-01 13:20:31 +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 2014-12-01 13:20:31 +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ß <thomas.voss at canonical.com>
> + */
> +
> +#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 2014-12-01 13:20:31 +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ß <thomas.voss at canonical.com>
> + */
> +
> +#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
Isn't the current player also the one that has been set as the current player by either being the foreground playing player, or the background playing player. I'd add a comment about this too.
> + // that has been created most recently and 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 2014-12-01 13:20:31 +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ß <thomas.voss at canonical.com>
> + */
> +
> +#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 2014-12-01 13:20:31 +0000
> +++ src/core/media/server/server.cpp 2014-12-01 13:20:31 +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>
> @@ -96,23 +97,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)
> {
> @@ -134,7 +149,7 @@
>
> // And stop execution of helper and actual service.
> external_services.stop();
> - service->stop();
> + skeleton->stop();
>
> if (external_services_worker.joinable())
> external_services_worker.join();
>
> === modified file 'src/core/media/service_implementation.cpp'
> --- src/core/media/service_implementation.cpp 2014-12-01 13:20:31 +0000
> +++ src/core/media/service_implementation.cpp 2014-12-01 13:20:31 +0000
> @@ -74,7 +74,7 @@
> display_state_lock->request_release(media::power::DisplayState::off);
> }
>
> - media::ServiceImplementation::Configuration configuration;
> + media::ServiceImplementation::Configuration configuration;
> // This holds the key of the multimedia role Player instance that was paused
> // when the battery level reached 10% or 5%
> media::Player::PlayerKey resume_key;
> @@ -86,7 +86,7 @@
> media::audio::OutputObserver::Ptr audio_output_observer;
> media::apparmor::ubuntu::RequestContextResolver::Ptr request_context_resolver;
> media::apparmor::ubuntu::RequestAuthenticator::Ptr request_authenticator;
> - media::telephony::CallMonitor::Ptr call_monitor;
> + media::telephony::CallMonitor::Ptr call_monitor;
> std::list<media::Player::PlayerKey> paused_sessions;
> };
>
> @@ -170,11 +170,15 @@
> // until all dispatches are done
> d->configuration.external_services.io_service.post([this, key]()
> {
> +<<<<<<< TREE
> if (!has_player_for_key(key))
> return;
>
> if (player_for_key(key)->lifetime() == Player::Lifetime::normal)
> remove_player_for_key(key);
> +=======
> + d->configuration.player_store->remove_player_for_key(key);
> +>>>>>>> MERGE-SOURCE
> });
> });
>
> @@ -195,19 +199,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
> @@ -227,7 +231,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)
> @@ -240,19 +244,20 @@
>
> void media::ServiceImplementation::resume_paused_multimedia_sessions()
> {
> - std::for_each(d->paused_sessions.begin(), d->paused_sessions.end(), [this](const media::Player::PlayerKey& key) {
> - player_for_key(key)->play();
> - });
> + std::for_each(d->paused_sessions.begin(), d->paused_sessions.end(), [this](const media::Player::PlayerKey& key)
> + {
> + d->configuration.player_store->player_for_key(key)->play();
> + });
>
> d->paused_sessions.clear();
> }
>
> 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 2014-12-01 13:20:31 +0000
> +++ src/core/media/service_implementation.h 2014-12-01 13:20:31 +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 2014-12-01 13:20:31 +0000
> +++ src/core/media/service_skeleton.cpp 2014-12-01 13:20:31 +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(
> @@ -252,15 +253,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;
>
> @@ -290,9 +283,16 @@
> media::ServiceSkeleton* impl;
> dbus::Object::Ptr object;
>
> +<<<<<<< TREE
> + // We query the apparmor profile to obtain an identity for players.
> + org::freedesktop::dbus::DBus::Stub dbus_stub;
> // 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;
> +>>>>>>> MERGE-SOURCE
> // We expose the entire service as an MPRIS player.
> struct Exported
> {
> @@ -526,7 +526,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;
> @@ -561,9 +561,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))
> {
> }
>
> @@ -571,6 +571,7 @@
> {
> }
>
> +<<<<<<< TREE
> bool media::ServiceSkeleton::has_player_for_key(const media::Player::PlayerKey& key) const
> {
> return d->session_store.count(key) > 0;
> @@ -611,6 +612,16 @@
> break;
> }
> }
> +=======
> +std::shared_ptr<media::Player> media::ServiceSkeleton::create_session(const media::Player::Configuration& config)
> +{
> + return d->configuration.impl->create_session(config);
> +}
> +
> +void media::ServiceSkeleton::pause_other_sessions(media::Player::PlayerKey key)
> +{
> + d->configuration.impl->pause_other_sessions(key);
> +>>>>>>> MERGE-SOURCE
> }
>
> 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 2014-12-01 13:20:31 +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,20 @@
> 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&);
> + void pause_other_sessions(Player::PlayerKey key);
>
> void run();
> void stop();
>
--
https://code.launchpad.net/~thomas-voss/media-hub/decouple-service-skeleton-and-implementation/+merge/243283
Your team Ubuntu Phablet Team is subscribed to branch lp:media-hub.
More information about the Ubuntu-reviews
mailing list