[Merge] lp:~justinmcp/media-hub/proxy-player into lp:media-hub
Jim Hodapp
jim.hodapp at canonical.com
Tue May 10 19:42:02 UTC 2016
Review: Needs Fixing code
Several comments inline below after doing a more thorough code review. Also, this MR needs syncing with the latest media-hub branch which includes the use of a new logging facility instead of using cout/cerr.
Diff comments:
>
> === added file 'src/core/media/proxy_player_implementation.cpp'
> --- src/core/media/proxy_player_implementation.cpp 1970-01-01 00:00:00 +0000
> +++ src/core/media/proxy_player_implementation.cpp 2016-04-06 01:02:31 +0000
> @@ -0,0 +1,396 @@
> +/*
> + * Copyright © 2013-2015 Canonical Ltd. *
Update the copyright since this is a new file.
> + * 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>
Feel free to take credit. :)
> + * Jim Hodapp <jim.hodapp at canonical.com>
> + */
> +
> +#include "proxy_player_implementation.h"
> +#include "util/timeout.h"
> +
> +#include <unistd.h>
> +
> +#include "client_death_observer.h"
> +#include "track_list_implementation.h"
> +
> +#include <memory>
> +#include <exception>
> +#include <iostream>
> +#include <mutex>
> +
> +namespace media = core::ubuntu::media;
> +namespace dbus = core::dbus;
> +
> +using namespace std;
> +
> +template<typename Parent>
> +struct media::ProxyPlayerImplementation<Parent>::Private :
> + public std::enable_shared_from_this<Private>
> +{
> + enum class wakelock_clear_t
> + {
> + WAKELOCK_CLEAR_INACTIVE,
> + WAKELOCK_CLEAR_DISPLAY,
> + WAKELOCK_CLEAR_SYSTEM,
> + WAKELOCK_CLEAR_INVALID
> + };
> +
> + Private(ProxyPlayerImplementation* parent, const media::ProxyPlayerImplementation<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()),
> + system_wakelock_count(0),
> + display_wakelock_count(0),
> + previous_state(media::Player::stopped),
> + state(media::Player::null)
> + {
> + // Poor man's logging of release/acquire events.
> + display_state_lock->acquired().connect([](media::power::DisplayState state)
> + {
> + std::cout << "Acquired new display state: " << state << std::endl;
> + });
> +
> + display_state_lock->released().connect([](media::power::DisplayState state)
> + {
> + std::cout << "Released display state: " << state << std::endl;
> + });
> +
> + system_state_lock->acquired().connect([](media::power::SystemState state)
> + {
> + std::cout << "Acquired new system state: " << state << std::endl;
> + });
> +
> + system_state_lock->released().connect([](media::power::SystemState state)
> + {
> + std::cout << "Released system state: " << state << std::endl;
> + });
> + }
> +
> + ~Private()
> + {
> + // Make sure that we don't hold on to the wakelocks if media-hub-server
> + // ever gets restarted manually or automatically
> + clear_wakelocks();
> + }
> +
> + void request_power_state()
> + {
> + std::cout << __PRETTY_FUNCTION__ << std::endl;
> + try
> + {
> + if (parent->is_video_source())
Since this isn't using the Engine class and thus not GStreamer, is this still going to work for this new Player class?
> + {
> + if (++display_wakelock_count == 1)
> + {
> + std::cout << "Requesting new display wakelock." << std::endl;
> + display_state_lock->request_acquire(media::power::DisplayState::on);
> + std::cout << "Requested new display wakelock." << std::endl;
> + }
> + }
> + else
> + {
> + if (++system_wakelock_count == 1)
> + {
> + std::cout << "Requesting new system wakelock." << std::endl;
> + system_state_lock->request_acquire(media::power::SystemState::active);
> + std::cout << "Requested new system wakelock." << std::endl;
> + }
> + }
> + }
> + catch(const std::exception& e)
> + {
> + std::cerr << "Warning: failed to request power state: ";
> + std::cerr << e.what() << std::endl;
> + }
> + }
> +
> + void clear_wakelock(const wakelock_clear_t &wakelock)
> + {
> + cout << __PRETTY_FUNCTION__ << endl;
> + try
> + {
> + switch (wakelock)
> + {
> + case wakelock_clear_t::WAKELOCK_CLEAR_INACTIVE:
> + break;
> + case wakelock_clear_t::WAKELOCK_CLEAR_SYSTEM:
> + // Only actually clear the system wakelock once the count reaches zero
> + if (--system_wakelock_count == 0)
> + {
> + std::cout << "Clearing system wakelock." << std::endl;
> + system_state_lock->request_release(media::power::SystemState::active);
> + }
> + break;
> + case wakelock_clear_t::WAKELOCK_CLEAR_DISPLAY:
> + // Only actually clear the display wakelock once the count reaches zero
> + if (--display_wakelock_count == 0)
> + {
> + std::cout << "Clearing display wakelock." << std::endl;
> + display_state_lock->request_release(media::power::DisplayState::on);
> + }
> + break;
> + case wakelock_clear_t::WAKELOCK_CLEAR_INVALID:
> + default:
> + cerr << "Can't clear invalid wakelock type" << endl;
> + }
> + }
> + catch(const std::exception& e)
> + {
> + std::cerr << "Warning: failed to clear power state: ";
> + std::cerr << e.what() << std::endl;
> + }
> + }
> +
> + wakelock_clear_t current_wakelock_type() const
> + {
> + return (parent->is_video_source()) ?
> + wakelock_clear_t::WAKELOCK_CLEAR_DISPLAY : wakelock_clear_t::WAKELOCK_CLEAR_SYSTEM;
> + }
> +
> + void clear_wakelocks()
> + {
> + // Clear both types of wakelocks (display and system)
> + if (system_wakelock_count.load() > 0)
> + {
> + system_wakelock_count = 1;
> + clear_wakelock(wakelock_clear_t::WAKELOCK_CLEAR_SYSTEM);
> + }
> + if (display_wakelock_count.load() > 0)
> + {
> + display_wakelock_count = 1;
> + clear_wakelock(wakelock_clear_t::WAKELOCK_CLEAR_DISPLAY);
> + }
> + }
> +
> + std::function<void()> make_clear_wakelock_functor()
> + {
> + // Since this functor will be executed on a separate detached thread
> + // 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{this->shared_from_this()};
> + auto wakelock_type = current_wakelock_type();
> + return [weak_self, wakelock_type] {
> + if (auto self = weak_self.lock())
> + self->clear_wakelock(wakelock_type);
> + };
> + }
> +
> + void on_client_died()
> + {
> + }
> +
> + void change_state(media::Player::PlaybackStatus state)
> + {
> + switch(state)
> + {
> + case media::Player::playing:
> + {
> + // And update our playback status.
> + parent->playback_status().set(media::Player::playing);
> +
> + request_power_state();
> + break;
> + }
> + case media::Player::stopped:
> + {
> + parent->playback_status().set(media::Player::stopped);
> + if (previous_state == media::Player::playing)
> + {
> + timeout(4000, true, make_clear_wakelock_functor());
> + }
> + break;
> + }
> + case media::Player::paused:
> + {
> + parent->playback_status().set(media::Player::paused);
> + if (previous_state == media::Player::playing)
> + {
> + timeout(4000, true, make_clear_wakelock_functor());
> + }
> + break;
> + }
> + default:
> + break;
> + };
> +
> + parent->emit_playback_status_changed(state);
> +
> + // Keep track of the previous Engine playback state:
> + previous_state = state;
> + }
> +
> + // Our link back to our parent.
> + media::ProxyPlayerImplementation<Parent>* parent;
> + // We just store the parameters passed on construction.
> + media::ProxyPlayerImplementation<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<media::TrackListImplementation> track_list;
> + std::atomic<int> system_wakelock_count;
> + std::atomic<int> display_wakelock_count;
> + media::Player::PlaybackStatus previous_state;
> + media::Player::PlaybackStatus state;
> + core::Signal<> on_client_disconnected;
> +};
> +
> +template<typename Parent>
> +media::ProxyPlayerImplementation<Parent>::ProxyPlayerImplementation(const media::ProxyPlayerImplementation<Parent>::Configuration& config)
> + : Parent{config.parent},
> + d{std::make_shared<Private>(this, config)}
> +{
> + // Initialize default values for Player interface properties
> + Parent::can_play().set(true);
> + Parent::can_pause().set(true);
> + Parent::can_seek().set(true);
> + Parent::can_go_previous().set(false);
> + Parent::can_go_next().set(false);
> + Parent::is_video_source().set(true);
> + Parent::is_audio_source().set(false);
> + Parent::shuffle().set(false);
> + 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);
> + Parent::orientation().set(Player::Orientation::rotate0);
> + Parent::lifetime().set(Player::Lifetime::normal);
> +
> + // Everything is setup, we now subscribe to death notifications.
> + std::weak_ptr<Private> wp{d};
> +
> + d->config.client_death_observer->register_for_death_notifications_with_key(config.key);
Will this still do anything without Binder being used in this case? The Binder connection between client and server are how we know that a normal player has quit or crashed.
> + d->config.client_death_observer->on_client_with_key_died().connect([wp](const media::Player::PlayerKey& died)
> + {
> + if (auto sp = wp.lock())
> + {
> + if (died != sp->config.key)
> + return;
> +
> + static const std::chrono::milliseconds timeout{1000};
> + media::timeout(timeout.count(), true, [wp]()
> + {
> + if (auto sp = wp.lock())
> + sp->on_client_died();
> + });
> + }
> + });
> +}
> +
> +template<typename Parent>
> +media::ProxyPlayerImplementation<Parent>::~ProxyPlayerImplementation()
> +{
> +}
> +
> +template<typename Parent>
> +std::string media::ProxyPlayerImplementation<Parent>::uuid() const
> +{
> + return std::string{};
> +}
> +
> +template<typename Parent>
> +void media::ProxyPlayerImplementation<Parent>::reconnect()
> +{
> + d->config.client_death_observer->register_for_death_notifications_with_key(d->config.key);
> +}
> +
> +template<typename Parent>
> +void media::ProxyPlayerImplementation<Parent>::abandon()
> +{
> + // Signal client disconnection due to abandonment of player
> + d->on_client_died();
> +}
> +
> +template<typename Parent>
> +std::shared_ptr<media::TrackList> media::ProxyPlayerImplementation<Parent>::track_list()
> +{
> + return std::shared_ptr<media::TrackList>();
> +}
> +
> +template<typename Parent>
> +media::Player::PlayerKey media::ProxyPlayerImplementation<Parent>::key() const
> +{
> + return d->config.key;
> +}
> +
> +template<typename Parent>
> +media::video::Sink::Ptr media::ProxyPlayerImplementation<Parent>::create_gl_texture_video_sink(std::uint32_t /*texture_id*/)
> +{
> + return media::video::Sink::Ptr{};
> +}
> +
> +template<typename Parent>
> +bool media::ProxyPlayerImplementation<Parent>::open_uri(const Track::UriType& /*uri*/)
> +{
> + return false;
> +}
> +
> +template<typename Parent>
> +bool media::ProxyPlayerImplementation<Parent>::open_uri(const Track::UriType& /*uri*/, const Player::HeadersType& /*headers*/)
> +{
> + return false;
> +}
> +
> +template<typename Parent>
> +void media::ProxyPlayerImplementation<Parent>::next()
> +{
> +}
> +
> +template<typename Parent>
> +void media::ProxyPlayerImplementation<Parent>::previous()
> +{
> +}
> +
> +template<typename Parent>
> +void media::ProxyPlayerImplementation<Parent>::play()
> +{
> + d->change_state(media::Player::playing);
> +}
> +
> +template<typename Parent>
> +void media::ProxyPlayerImplementation<Parent>::pause()
> +{
> + d->change_state(media::Player::paused);
> +}
> +
> +template<typename Parent>
> +void media::ProxyPlayerImplementation<Parent>::stop()
> +{
> + d->change_state(media::Player::stopped);
> +}
> +
> +template<typename Parent>
> +void media::ProxyPlayerImplementation<Parent>::seek_to(const std::chrono::microseconds& /*ms*/)
> +{
> +}
> +
> +template<typename Parent>
> +const core::Signal<>& media::ProxyPlayerImplementation<Parent>::on_client_disconnected() const
> +{
> + return d->on_client_disconnected;
> +}
> +
> +template<typename Parent>
> +void media::ProxyPlayerImplementation<Parent>::emit_playback_status_changed(const media::Player::PlaybackStatus &status)
> +{
> + Parent::playback_status_changed()(status);
> +}
> +
> +#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::ProxyPlayerImplementation<media::PlayerSkeleton>;
>
> === modified file 'src/core/media/service_implementation.cpp'
> --- src/core/media/service_implementation.cpp 2016-03-02 18:32:46 +0000
> +++ src/core/media/service_implementation.cpp 2016-04-06 01:02:31 +0000
> @@ -297,6 +298,45 @@
> });
> }
>
> +std::shared_ptr<media::Player> media::ServiceImplementation::create_proxy_session(
What I'm thinking here is something like this from the client side:
auto hubService = media::Service::Client::instance();
auto hubPlayerSession = hubService->create_session(media::Player::Client::proxy_configuration());
------------
As you can see from the existing code, we already have a method called media::Player::Client::default_configuration() so I'm proposing to add the new proxy_configuration() and reuse the same create_session() method. The method would detect the type of configuration passed and construct the correct Player session type. The core::ubuntu::media::Player::Configuration struct would need to be updated to contain a new enum type, something like enum class PlayerType { DefaultPlayer, ProxyPlayer }; This makes all of the following copy and paste code unnecessary which will make future code maintenance much easier.
> + const media::Player::Configuration& conf)
> +{
> + auto player = std::make_shared<media::ProxyPlayerImplementation<media::PlayerSkeleton>>(media::ProxyPlayerImplementation<media::PlayerSkeleton>::Configuration
const
> + {
> + media::PlayerSkeleton::Configuration
> + {
> + conf.bus,
> + conf.service,
> + conf.session,
> + d->request_context_resolver,
> + d->request_authenticator
> + },
> + conf.key,
> + d->client_death_observer,
> + d->power_state_controller
> + });
> +
> + auto key = conf.key;
const
> + player->on_client_disconnected().connect([this, key]()
> + {
> + // Call remove_player_for_key asynchronously otherwise deadlock can occur
> + // if called within this dispatcher context.
> + // remove_player_for_key can destroy the player instance which in turn
> + // destroys the "on_client_disconnected" signal whose destructor will wait
> + // until all dispatches are done
> + d->configuration.external_services.io_service.post([this, key]()
> + {
> + if (!d->configuration.player_store->has_player_for_key(key))
> + return;
> +
> + if (d->configuration.player_store->player_for_key(key)->lifetime() == Player::Lifetime::normal)
> + d->configuration.player_store->remove_player_for_key(key);
> + });
> + });
> +
> + return player;
> +}
> +
> void media::ServiceImplementation::pause_all_multimedia_sessions(bool resume_play_after_phonecall)
> {
> d->configuration.player_store->enumerate_players([this, resume_play_after_phonecall](const media::Player::PlayerKey& key, const std::shared_ptr<media::Player>& player)
--
https://code.launchpad.net/~justinmcp/media-hub/proxy-player/+merge/268862
Your team Ubuntu Phablet Team is subscribed to branch lp:media-hub.
More information about the Ubuntu-reviews
mailing list