[Merge] lp:~thomas-voss/media-hub/decouple-player-skeleton-and-implementation into lp:media-hub
Jim Hodapp
jim.hodapp at canonical.com
Mon Dec 8 17:12:18 UTC 2014
Review: Needs Fixing code
One comment inline below.
Diff comments:
> === modified file 'src/core/media/player_implementation.cpp'
> --- src/core/media/player_implementation.cpp 2014-12-01 16:15:22 +0000
> +++ src/core/media/player_implementation.cpp 2014-12-01 16:15:22 +0000
> @@ -39,7 +39,92 @@
>
> using namespace std;
>
> -struct media::PlayerImplementation::Private :
> +namespace
> +{
> +// A helper type to replace the playlist implementation below.
> +// Please note that this type is only a temporary manner. Ideally,
> +// the actual implementation should be injected as a dependency from the
> +// outside.
> +struct NullTrackList : public media::TrackList
Please add this to a separate header file.
> +{
> + NullTrackList() = default;
> +
> + bool has_next()
> + {
> + return false;
> + }
> +
> + media::Track::Id next()
> + {
> + return media::Track::Id{};
> + }
> +
> + media::Track::UriType query_uri_for_track(const media::Track::Id&)
> + {
> + return media::Track::UriType{};
> + }
> +
> + const core::Property<bool>& can_edit_tracks() const override
> + {
> + return props_and_sigs.can_edit_tracks;
> + }
> +
> + const core::Property<Container>& tracks() const override
> + {
> + return props_and_sigs.tracks;
> + }
> +
> + virtual media::Track::MetaData query_meta_data_for_track(const media::Track::Id&) override
> + {
> + return media::Track::MetaData{};
> + }
> +
> + void add_track_with_uri_at(const media::Track::UriType&, const media::Track::Id&, bool) override
> + {
> + }
> +
> + void remove_track(const media::Track::Id&) override
> + {
> + }
> +
> + void go_to(const media::Track::Id&) override
> + {
> + }
> +
> + const core::Signal<void>& on_track_list_replaced() const override
> + {
> + return props_and_sigs.on_track_list_replaced;
> + }
> +
> + const core::Signal<media::Track::Id>& on_track_added() const override
> + {
> + return props_and_sigs.on_track_added;
> + }
> +
> + const core::Signal<media::Track::Id>& on_track_removed() const override
> + {
> + return props_and_sigs.on_track_removed;
> + }
> +
> + const core::Signal<media::Track::Id>& on_track_changed() const override
> + {
> + return props_and_sigs.on_track_changed;
> + }
> +
> + struct
> + {
> + core::Property<bool> can_edit_tracks;
> + core::Property<TrackList::Container> tracks;
> + core::Signal<void> on_track_list_replaced;
> + core::Signal<media::Track::Id> on_track_added;
> + core::Signal<media::Track::Id> on_track_removed;
> + core::Signal<media::Track::Id> on_track_changed;
> + } props_and_sigs;
> +};
> +}
> +
> +template<typename Parent>
> +struct media::PlayerImplementation<Parent>::Private :
> public std::enable_shared_from_this<Private>
> {
> enum class wakelock_clear_t
> @@ -50,16 +135,13 @@
> WAKELOCK_CLEAR_INVALID
> };
>
> - Private(PlayerImplementation* parent, const media::PlayerImplementation::Configuration& config)
> + Private(PlayerImplementation* parent, const media::PlayerImplementation<Parent>::Configuration& config)
> : parent(parent),
> config(config),
> display_state_lock(config.power_state_controller->display_state_lock()),
> system_state_lock(config.power_state_controller->system_state_lock()),
> engine(std::make_shared<gstreamer::Engine>()),
> - track_list(
> - new media::TrackListImplementation(
> - config.session->path().as_string() + "/TrackList",
> - engine->meta_data_extractor())),
> + track_list(std::make_shared<NullTrackList>()),
> system_wakelock_count(0),
> display_wakelock_count(0),
> previous_state(Engine::State::stopped),
> @@ -224,7 +306,7 @@
> // the execution of the functor may surpass the lifetime of this Private
> // object instance. By keeping a weak_ptr to the private object instance
> // we can check if the object is dead before calling methods on it
> - std::weak_ptr<Private> weak_self{shared_from_this()};
> + std::weak_ptr<Private> weak_self{this->shared_from_this()};
> auto wakelock_type = current_wakelock_type();
> return [weak_self, wakelock_type] {
> if (auto self = weak_self.lock())
> @@ -238,14 +320,14 @@
> }
>
> // Our link back to our parent.
> - media::PlayerImplementation* parent;
> + media::PlayerImplementation<Parent>* parent;
> // We just store the parameters passed on construction.
> - media::PlayerImplementation::Configuration config;
> + media::PlayerImplementation<Parent>::Configuration config;
> media::power::StateController::Lock<media::power::DisplayState>::Ptr display_state_lock;
> media::power::StateController::Lock<media::power::SystemState>::Ptr system_state_lock;
>
> std::shared_ptr<Engine> engine;
> - std::shared_ptr<TrackListImplementation> track_list;
> + std::shared_ptr<NullTrackList> track_list;
> std::atomic<int> system_wakelock_count;
> std::atomic<int> display_wakelock_count;
> Engine::State previous_state;
> @@ -253,38 +335,34 @@
> core::Connection engine_state_change_connection;
> };
>
> -media::PlayerImplementation::PlayerImplementation(const media::PlayerImplementation::Configuration& config)
> - : media::PlayerSkeleton
> - {
> - media::PlayerSkeleton::Configuration
> - {
> - config.bus,
> - config.session,
> - config.request_context_resolver,
> - config.request_authenticator
> - }
> - },
> +template<typename Parent>
> +media::PlayerImplementation<Parent>::PlayerImplementation(const media::PlayerImplementation<Parent>::Configuration& config)
> + : Parent{config.parent},
> d{std::make_shared<Private>(this, config)}
> {
> // Initialize default values for Player interface properties
> - can_play().set(true);
> - can_pause().set(true);
> - can_seek().set(true);
> - can_go_previous().set(true);
> - can_go_next().set(true);
> - is_video_source().set(false);
> - is_audio_source().set(false);
> - is_shuffle().set(true);
> - playback_rate().set(1.f);
> - playback_status().set(Player::PlaybackStatus::null);
> - loop_status().set(Player::LoopStatus::none);
> - position().set(0);
> - duration().set(0);
> - audio_stream_role().set(Player::AudioStreamRole::multimedia);
> + Parent::can_play().set(true);
> + Parent::can_pause().set(true);
> + Parent::can_seek().set(true);
> + Parent::can_go_previous().set(true);
> + Parent::can_go_next().set(true);
> + Parent::is_video_source().set(false);
> + Parent::is_audio_source().set(false);
> + Parent::is_shuffle().set(true);
> + Parent::playback_rate().set(1.f);
> + Parent::playback_status().set(Player::PlaybackStatus::null);
> + Parent::loop_status().set(Player::LoopStatus::none);
> + Parent::position().set(0);
> + Parent::duration().set(0);
> + Parent::audio_stream_role().set(Player::AudioStreamRole::multimedia);
> d->engine->audio_stream_role().set(Player::AudioStreamRole::multimedia);
> +<<<<<<< TREE
> orientation().set(Player::Orientation::rotate0);
> lifetime().set(Player::Lifetime::normal);
> d->engine->lifetime().set(Player::Lifetime::normal);
> +=======
> + Parent::orientation().set(Player::Orientation::rotate0);
> +>>>>>>> MERGE-SOURCE
>
> // Make sure that the Position property gets updated from the Engine
> // every time the client requests position
> @@ -292,7 +370,7 @@
> {
> return d->engine->position().get();
> };
> - position().install(position_getter);
> + Parent::position().install(position_getter);
>
> // Make sure that the Duration property gets updated from the Engine
> // every time the client requests duration
> @@ -300,23 +378,23 @@
> {
> return d->engine->duration().get();
> };
> - duration().install(duration_getter);
> + Parent::duration().install(duration_getter);
>
> std::function<bool()> video_type_getter = [this]()
> {
> return d->engine->is_video_source().get();
> };
> - is_video_source().install(video_type_getter);
> + Parent::is_video_source().install(video_type_getter);
>
> std::function<bool()> audio_type_getter = [this]()
> {
> return d->engine->is_audio_source().get();
> };
> - is_audio_source().install(audio_type_getter);
> + Parent::is_audio_source().install(audio_type_getter);
>
> // Make sure that the audio_stream_role property gets updated on the Engine side
> // whenever the client side sets the role
> - audio_stream_role().changed().connect([this](media::Player::AudioStreamRole new_role)
> + Parent::audio_stream_role().changed().connect([this](media::Player::AudioStreamRole new_role)
> {
> d->engine->audio_stream_role().set(new_role);
> });
> @@ -325,7 +403,7 @@
> // update the Player's cached value
> d->engine->orientation().changed().connect([this](const Player::Orientation& o)
> {
> - orientation().set(o);
> + Parent::orientation().set(o);
> });
>
> lifetime().changed().connect([this](media::Player::Lifetime lifetime)
> @@ -354,26 +432,27 @@
>
> d->engine->seeked_to_signal().connect([this](uint64_t value)
> {
> - seeked_to()(value);
> + Parent::seeked_to()(value);
> });
>
> d->engine->end_of_stream_signal().connect([this]()
> {
> - end_of_stream()();
> + Parent::end_of_stream()();
> });
>
> d->engine->playback_status_changed_signal().connect([this](const Player::PlaybackStatus& status)
> {
> - playback_status_changed()(status);
> + Parent::playback_status_changed()(status);
> });
>
> d->engine->video_dimension_changed_signal().connect([this](const media::video::Dimensions& dimensions)
> {
> - video_dimension_changed()(dimensions);
> + Parent::video_dimension_changed()(dimensions);
> });
> }
>
> -media::PlayerImplementation::~PlayerImplementation()
> +template<typename Parent>
> +media::PlayerImplementation<Parent>::~PlayerImplementation()
> {
> // Install null getters as these properties may be destroyed
> // after the engine has been destroyed since they are owned by the
> @@ -382,45 +461,49 @@
> {
> return static_cast<uint64_t>(0);
> };
> - position().install(position_getter);
> + Parent::position().install(position_getter);
>
> std::function<uint64_t()> duration_getter = [this]()
> {
> return static_cast<uint64_t>(0);
> };
> - duration().install(duration_getter);
> + Parent::duration().install(duration_getter);
>
> std::function<bool()> video_type_getter = [this]()
> {
> return false;
> };
> - is_video_source().install(video_type_getter);
> + Parent::is_video_source().install(video_type_getter);
>
> std::function<bool()> audio_type_getter = [this]()
> {
> return false;
> };
> - is_audio_source().install(audio_type_getter);
> + Parent::is_audio_source().install(audio_type_getter);
> }
>
> -std::shared_ptr<media::TrackList> media::PlayerImplementation::track_list()
> +template<typename Parent>
> +std::shared_ptr<media::TrackList> media::PlayerImplementation<Parent>::track_list()
> {
> return d->track_list;
> }
>
> // TODO: Convert this to be a property instead of sync call
> -media::Player::PlayerKey media::PlayerImplementation::key() const
> +template<typename Parent>
> +media::Player::PlayerKey media::PlayerImplementation<Parent>::key() const
> {
> return d->config.key;
> }
>
> -media::video::Sink::Ptr media::PlayerImplementation::create_gl_texture_video_sink(std::uint32_t texture_id)
> +template<typename Parent>
> +media::video::Sink::Ptr media::PlayerImplementation<Parent>::create_gl_texture_video_sink(std::uint32_t texture_id)
> {
> d->engine->create_video_sink(texture_id);
> return media::video::Sink::Ptr{};
> }
>
> -bool media::PlayerImplementation::open_uri(const Track::UriType& uri)
> +template<typename Parent>
> +bool media::PlayerImplementation<Parent>::open_uri(const Track::UriType& uri)
> {
> return d->engine->open_resource_for_uri(uri);
> }
> @@ -442,8 +525,6 @@
> return NULL;
> }
>
> -=======
> ->>>>>>> MERGE-SOURCE
> void media::PlayerImplementation::next()
> {
> }
> @@ -453,27 +534,50 @@
> }
>
> void media::PlayerImplementation::play()
> +=======
> +template<typename Parent>
> +void media::PlayerImplementation<Parent>::next()
> +{
> +}
> +
> +template<typename Parent>
> +void media::PlayerImplementation<Parent>::previous()
> +{
> +}
> +
> +template<typename Parent>
> +void media::PlayerImplementation<Parent>::play()
> +>>>>>>> MERGE-SOURCE
> {
> d->engine->play();
> }
>
> -void media::PlayerImplementation::pause()
> +template<typename Parent>
> +void media::PlayerImplementation<Parent>::pause()
> {
> d->engine->pause();
> }
>
> -void media::PlayerImplementation::stop()
> +template<typename Parent>
> +void media::PlayerImplementation<Parent>::stop()
> {
> std::cout << __PRETTY_FUNCTION__ << std::endl;
> d->engine->stop();
> }
>
> -void media::PlayerImplementation::seek_to(const std::chrono::microseconds& ms)
> +template<typename Parent>
> +void media::PlayerImplementation<Parent>::seek_to(const std::chrono::microseconds& ms)
> {
> d->engine->seek_to(ms);
> }
>
> -const core::Signal<>& media::PlayerImplementation::on_client_disconnected() const
> +template<typename Parent>
> +const core::Signal<>& media::PlayerImplementation<Parent>::on_client_disconnected() const
> {
> return d->on_client_disconnected;
> }
> +
> +#include <core/media/player_skeleton.h>
> +
> +// For linking purposes, we have to make sure that we have all symbols included within the dso.
> +template class media::PlayerImplementation<media::PlayerSkeleton>;
>
> === modified file 'src/core/media/player_implementation.h'
> --- src/core/media/player_implementation.h 2014-12-01 16:15:22 +0000
> +++ src/core/media/player_implementation.h 2014-12-01 16:15:22 +0000
> @@ -19,7 +19,7 @@
> #ifndef CORE_UBUNTU_MEDIA_PLAYER_IMPLEMENTATION_H_
> #define CORE_UBUNTU_MEDIA_PLAYER_IMPLEMENTATION_H_
>
> -#include "player_skeleton.h"
> +#include <core/media/player.h>
>
> #include "apparmor/ubuntu.h"
> #include "client_death_observer.h"
> @@ -36,20 +36,18 @@
> class Engine;
> class Service;
>
> -class PlayerImplementation : public PlayerSkeleton
> +template<typename Parent>
> +class PlayerImplementation : public Parent
> {
> public:
> // All creation time arguments go here
> struct Configuration
> {
> - std::shared_ptr<core::dbus::Bus> bus;
> - std::shared_ptr<core::dbus::Object> session;
> - std::shared_ptr<Service> service;
> - PlayerKey key;
> -
> + // All creation time configuration options of the Parent class.
> + typename Parent::Configuration parent;
> + // The unique key identifying the player instance.
> + Player::PlayerKey key;
> // Functional dependencies
> - apparmor::ubuntu::RequestContextResolver::Ptr request_context_resolver;
> - apparmor::ubuntu::RequestAuthenticator::Ptr request_authenticator;
> ClientDeathObserver::Ptr client_death_observer;
> power::StateController::Ptr power_state_controller;
> };
> @@ -58,7 +56,7 @@
> ~PlayerImplementation();
>
> virtual std::shared_ptr<TrackList> track_list();
> - virtual PlayerKey key() const;
> + virtual Player::PlayerKey key() const;
>
> virtual video::Sink::Ptr create_gl_texture_video_sink(std::uint32_t texture_id);
>
>
> === modified file 'src/core/media/player_skeleton.h'
> --- src/core/media/player_skeleton.h 2014-12-01 16:15:22 +0000
> +++ src/core/media/player_skeleton.h 2014-12-01 16:15:22 +0000
> @@ -48,6 +48,19 @@
> class PlayerSkeleton : public core::ubuntu::media::Player
> {
> public:
> + // All creation time arguments go here.
> + struct Configuration
> + {
> + // The bus connection we are associated with.
> + std::shared_ptr<core::dbus::Bus> bus;
> + // The session object that we want to expose the skeleton upon.
> + std::shared_ptr<core::dbus::Object> session;
> + // Our functional dependencies.
> + apparmor::ubuntu::RequestContextResolver::Ptr request_context_resolver;
> + apparmor::ubuntu::RequestAuthenticator::Ptr request_authenticator;
> + };
> +
> + PlayerSkeleton(const Configuration& configuration);
> ~PlayerSkeleton();
>
> virtual const core::Property<bool>& can_play() const;
> @@ -83,21 +96,6 @@
> virtual core::Signal<PlaybackStatus>& playback_status_changed();
> virtual const core::Signal<video::Dimensions>& video_dimension_changed() const;
>
> -protected:
> - // All creation time arguments go here.
> - struct Configuration
> - {
> - // The bus connection we are associated with.
> - std::shared_ptr<core::dbus::Bus> bus;
> - // The session object that we want to expose the skeleton upon.
> - std::shared_ptr<core::dbus::Object> session;
> - // Our functional dependencies.
> - apparmor::ubuntu::RequestContextResolver::Ptr request_context_resolver;
> - apparmor::ubuntu::RequestAuthenticator::Ptr request_authenticator;
> - };
> -
> - PlayerSkeleton(const Configuration& configuration);
> -
> // These properties are not exposed to the client, but still need to be
> // able to be settable from within the Player:
> virtual core::Property<PlaybackStatus>& playback_status();
>
> === modified file 'src/core/media/service_implementation.cpp'
> --- src/core/media/service_implementation.cpp 2014-12-01 16:15:22 +0000
> +++ src/core/media/service_implementation.cpp 2014-12-01 16:15:22 +0000
> @@ -26,6 +26,7 @@
> #include "audio/output_observer.h"
> #include "client_death_observer.h"
> #include "player_configuration.h"
> +#include "player_skeleton.h"
> #include "player_implementation.h"
> #include "power/battery_observer.h"
> #include "power/state_controller.h"
> @@ -148,14 +149,16 @@
> std::shared_ptr<media::Player> media::ServiceImplementation::create_session(
> const media::Player::Configuration& conf)
> {
> - auto player = std::make_shared<media::PlayerImplementation>(media::PlayerImplementation::Configuration
> + auto player = std::make_shared<media::PlayerImplementation<media::PlayerSkeleton>>(media::PlayerImplementation<media::PlayerSkeleton>::Configuration
> {
> - conf.bus,
> - conf.session,
> - shared_from_this(),
> + media::PlayerSkeleton::Configuration
> + {
> + conf.bus,
> + conf.session,
> + d->request_context_resolver,
> + d->request_authenticator
> + },
> conf.key,
> - d->request_context_resolver,
> - d->request_authenticator,
> d->client_death_observer,
> d->power_state_controller
> });
>
--
https://code.launchpad.net/~thomas-voss/media-hub/decouple-player-skeleton-and-implementation/+merge/243303
Your team Ubuntu Phablet Team is subscribed to branch lp:media-hub.
More information about the Ubuntu-reviews
mailing list