[Merge] lp:~thomas-voss/media-hub/introduce-power-controller-interface into lp:media-hub
Jim Hodapp
jim.hodapp at canonical.com
Thu Dec 4 20:40:58 UTC 2014
Review: Needs Fixing code
There's a lot to this one and we'll want to test the wakelock parts extremely well since there have been issues in the past with race conditions. Looks good, several comments inline below:
Diff comments:
> === modified file 'src/core/media/CMakeLists.txt'
> --- src/core/media/CMakeLists.txt 2014-11-26 12:42:47 +0000
> +++ src/core/media/CMakeLists.txt 2014-11-26 12:42:47 +0000
> @@ -90,8 +90,12 @@
> hybris_client_death_observer.cpp
> cover_art_resolver.cpp
> engine.cpp
> +
> + power/state_controller.cpp
> +
> recorder_observer.cpp
> hybris_recorder_observer.cpp
> +
> gstreamer/engine.cpp
> gstreamer/playbin.cpp
>
>
> === added file 'src/core/media/external_services.h'
> --- src/core/media/external_services.h 1970-01-01 00:00:00 +0000
> +++ src/core/media/external_services.h 2014-11-26 12:42:47 +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_EXTERNAL_SERVICES_H_
> +#define CORE_UBUNTU_MEDIA_EXTERNAL_SERVICES_H_
> +
> +#include <core/dbus/bus.h>
> +
> +#include <core/dbus/asio/executor.h>
> +
> +#include <boost/asio.hpp>
> +
> +namespace core
> +{
> +namespace ubuntu
> +{
> +namespace media
> +{
> +namespace helper
> +{
> +// A helper struct that bundles:
> +// * a dispatcher, i.e., the io_service
> +// * access to the system and session bus
> +//
> +// In addtion, it allows us to mock out services and
> +// for acceptance testing purposes.
> +struct ExternalServices
> +{
> + ExternalServices(const core::dbus::Bus::Ptr& session, const core::dbus::Bus::Ptr& system)
> + : keep_alive{io_service},
> + session{session},
> + system{system}
> + {
> + }
> +
> + ExternalServices()
> + : ExternalServices
> + {
> + core::dbus::Bus::Ptr{new core::dbus::Bus{core::dbus::WellKnownBus::session}},
> + core::dbus::Bus::Ptr{new core::dbus::Bus{core::dbus::WellKnownBus::system}}
> + }
> + {
> + session->install_executor(core::dbus::asio::make_executor(session, io_service));
> + system->install_executor(core::dbus::asio::make_executor(session, io_service));
> + }
> +
> +
> + void run()
> + {
> + io_service.run();
> + }
> +
> + void stop()
> + {
> + io_service.stop();
> + }
> +
> + boost::asio::io_service io_service;
> + boost::asio::io_service::work keep_alive;
> +
> + core::dbus::Bus::Ptr session;
> + core::dbus::Bus::Ptr system;
> +};
> +}
> +}
> +}
> +}
> +
> +#endif // CORE_UBUNTU_MEDIA_EXTERNAL_SERVICES_H_
>
> === modified file 'src/core/media/player_implementation.cpp'
> --- src/core/media/player_implementation.cpp 2014-11-26 12:42:47 +0000
> +++ src/core/media/player_implementation.cpp 2014-11-26 12:42:47 +0000
> @@ -25,8 +25,6 @@
> #include "engine.h"
> #include "track_list_implementation.h"
>
> -#include "powerd_service.h"
> -#include "unity_screen_service.h"
> #include "gstreamer/engine.h"
>
> #include <memory>
> @@ -52,41 +50,25 @@
> WAKELOCK_CLEAR_INVALID
> };
>
> - Private(PlayerImplementation* parent,
> - const dbus::types::ObjectPath& session_path,
> - const std::shared_ptr<media::Service>& service,
> - PlayerImplementation::PlayerKey key)
> + Private(PlayerImplementation* parent, const media::PlayerImplementation::Configuration& config)
> : parent(parent),
> - service(service),
> + 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>()),
> - session_path(session_path),
> track_list(
> new media::TrackListImplementation(
> - session_path.as_string() + "/TrackList",
> + config.session->path().as_string() + "/TrackList",
> engine->meta_data_extractor())),
> - sys_lock_name("media-hub-music-playback"),
> - disp_cookie(-1),
> system_wakelock_count(0),
> display_wakelock_count(0),
> previous_state(Engine::State::stopped),
> - key(key),
> engine_state_change_connection(engine->state().changed().connect(make_state_change_handler()))
> {
> - auto bus = std::shared_ptr<dbus::Bus>(new dbus::Bus(core::dbus::WellKnownBus::system));
> - bus->install_executor(dbus::asio::make_executor(bus));
> -
> - auto stub_service = dbus::Service::use_service(bus, dbus::traits::Service<core::Powerd>::interface_name());
> - powerd_session = stub_service->object_for_path(dbus::types::ObjectPath("/com/canonical/powerd"));
> -
> - auto uscreen_stub_service = dbus::Service::use_service(bus, dbus::traits::Service<core::UScreen>::interface_name());
> - uscreen_session = uscreen_stub_service->object_for_path(dbus::types::ObjectPath("/com/canonical/Unity/Screen"));
> -
> - auto client_death_observer = media::platform_default_client_death_observer();
> -
> - client_death_observer->register_for_death_notifications_with_key(key);
> - client_death_observer->on_client_with_key_died().connect([this](const media::Player::PlayerKey& died)
> + config.client_death_observer->register_for_death_notifications_with_key(config.key);
> + config.client_death_observer->on_client_with_key_died().connect([this](const media::Player::PlayerKey& died)
> {
> - if (died != this->key)
> + if (died != this->config.key)
> return;
>
> on_client_died();
> @@ -169,24 +151,12 @@
> if (parent->is_video_source())
> {
> if (++display_wakelock_count == 1)
> - {
> - auto result = uscreen_session->invoke_method_synchronously<core::UScreen::keepDisplayOn, int>();
> - if (result.is_error())
> - throw std::runtime_error(result.error().print());
> - disp_cookie = result.value();
> - cout << "Requested new display wakelock" << endl;
> - }
> + display_state_lock->request_acquire(media::power::DisplayState::on);
I'd like to keep the cout statements that help in debugging the wakelocks when something doesn't quite work right: cout << "Requested new display wakelock" << endl;
> }
> else
> {
> if (++system_wakelock_count == 1)
> - {
> - auto result = powerd_session->invoke_method_synchronously<core::Powerd::requestSysState, std::string>(sys_lock_name, static_cast<int>(1));
> - if (result.is_error())
> - throw std::runtime_error(result.error().print());
> - sys_cookie = result.value();
> - cout << "Requested new system wakelock" << endl;
> - }
> + system_state_lock->request_acquire(media::power::SystemState::active);
I'd like to keep the cout statements that help in debugging the wakelocks when something doesn't quite work right: cout << "Requested new system wakelock" << endl;
> }
> }
> catch(const std::exception& e)
> @@ -208,20 +178,12 @@
> case wakelock_clear_t::WAKELOCK_CLEAR_SYSTEM:
> // Only actually clear the system wakelock once the count reaches zero
> if (--system_wakelock_count == 0)
> - {
> - cout << "Clearing system wakelock" << endl;
> - powerd_session->invoke_method_synchronously<core::Powerd::clearSysState, void>(sys_cookie);
> - sys_cookie.clear();
> - }
> + system_state_lock->request_release(media::power::SystemState::active);
I'd like to keep the cout statements that help in debugging the wakelocks when something doesn't quite work right: cout << "Clearing system wakelock" << endl;
> break;
> case wakelock_clear_t::WAKELOCK_CLEAR_DISPLAY:
> // Only actually clear the display wakelock once the count reaches zero
> if (--display_wakelock_count == 0)
> - {
> - cout << "Clearing display wakelock" << endl;
> - uscreen_session->invoke_method_synchronously<core::UScreen::removeDisplayOnRequest, void>(disp_cookie);
> - disp_cookie = -1;
> - }
> + display_state_lock->request_release(media::power::DisplayState::on);
I'd like to keep the cout statements that help in debugging the wakelocks when something doesn't quite work right: cout << "Clearing display wakelock" << endl;
> break;
> case wakelock_clear_t::WAKELOCK_CLEAR_INVALID:
> default:
> @@ -275,44 +237,33 @@
> engine->reset();
> }
>
> - PlayerImplementation* parent;
> - std::shared_ptr<Service> service;
> + // Our link back to our parent.
> + media::PlayerImplementation* parent;
> + // We just store the parameters passed on construction.
> + media::PlayerImplementation::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;
> - dbus::types::ObjectPath session_path;
> std::shared_ptr<TrackListImplementation> track_list;
> - std::shared_ptr<dbus::Object> powerd_session;
> - std::shared_ptr<dbus::Object> uscreen_session;
> - std::string sys_lock_name;
> - int disp_cookie;
> - std::string sys_cookie;
> std::atomic<int> system_wakelock_count;
> std::atomic<int> display_wakelock_count;
> Engine::State previous_state;
> - PlayerImplementation::PlayerKey key;
> core::Signal<> on_client_disconnected;
> core::Connection engine_state_change_connection;
> };
>
> -media::PlayerImplementation::PlayerImplementation(
> - const std::string& identity,
> - const std::shared_ptr<core::dbus::Bus>& bus,
> - const std::shared_ptr<core::dbus::Object>& session,
> - const std::shared_ptr<Service>& service,
> - PlayerKey key)
> +media::PlayerImplementation::PlayerImplementation(const media::PlayerImplementation::Configuration& config)
> : media::PlayerSkeleton
> {
> media::PlayerSkeleton::Configuration
> {
> - bus,
> - session,
> - identity
> + config.bus,
> + config.session,
> + config.identity
> }
> },
> - d(make_shared<Private>(
> - this,
> - session->path(),
> - service,
> - key))
> + d{std::make_shared<Private>(this, config)}
> {
> // Initialize default values for Player interface properties
> can_play().set(true);
> @@ -452,7 +403,7 @@
> // TODO: Convert this to be a property instead of sync call
> media::Player::PlayerKey media::PlayerImplementation::key() const
> {
> - return d->key;
> + return d->config.key;
> }
>
> media::video::Sink::Ptr media::PlayerImplementation::create_gl_texture_video_sink(std::uint32_t texture_id)
>
> === modified file 'src/core/media/player_implementation.h'
> --- src/core/media/player_implementation.h 2014-11-26 12:42:47 +0000
> +++ src/core/media/player_implementation.h 2014-11-26 12:42:47 +0000
> @@ -21,6 +21,9 @@
>
> #include "player_skeleton.h"
>
> +#include "client_death_observer.h"
> +#include "power/state_controller.h"
> +
> #include <memory>
>
> namespace core
> @@ -35,12 +38,21 @@
> class PlayerImplementation : public PlayerSkeleton
> {
> public:
> - PlayerImplementation(
> - const std::string& identity,
> - const std::shared_ptr<core::dbus::Bus>& bus,
> - const std::shared_ptr<core::dbus::Object>& session,
> - const std::shared_ptr<Service>& service,
> - PlayerKey key);
> + // All creation time arguments go here
> + struct Configuration
> + {
> + std::string identity;
> + std::shared_ptr<core::dbus::Bus> bus;
> + std::shared_ptr<core::dbus::Object> session;
> + std::shared_ptr<Service> service;
> + PlayerKey key;
> +
> + // Functional dependencies
> + ClientDeathObserver::Ptr client_death_observer;
> + power::StateController::Ptr power_state_controller;
> + };
> +
> + PlayerImplementation(const Configuration& configuration);
> ~PlayerImplementation();
>
> virtual std::shared_ptr<TrackList> track_list();
>
> === added directory 'src/core/media/power'
> === added file 'src/core/media/power/state_controller.cpp'
> --- src/core/media/power/state_controller.cpp 1970-01-01 00:00:00 +0000
> +++ src/core/media/power/state_controller.cpp 2014-11-26 12:42:47 +0000
> @@ -0,0 +1,302 @@
> +/*
> + * 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/power/state_controller.h>
> +
> +#include <core/dbus/macros.h>
> +#include <core/dbus/object.h>
> +
> +namespace media = core::ubuntu::media;
> +
> +namespace com { namespace canonical {
> +struct Unity
> +{
> + struct Screen
> + {
> + static const std::string& name()
> + {
> + static std::string s = "com.canonical.Unity.Screen";
> + return s;
> + }
> +
> + static const core::dbus::types::ObjectPath& path()
> + {
> + static core::dbus::types::ObjectPath p{"/com/canonical/Unity/Screen"};
> + return p;
> + }
> +
> + DBUS_CPP_METHOD_DEF(keepDisplayOn, Screen)
> + DBUS_CPP_METHOD_DEF(removeDisplayOnRequest, Screen)
> + };
> +};
> +namespace powerd {
> +struct Interface
> +{
> + static std::string& name()
> + {
> + static std::string s = "com.canonical.powerd";
> + return s;
> + }
> +
> + static const core::dbus::types::ObjectPath& path()
> + {
> + static core::dbus::types::ObjectPath p{"/com/canonical/powerd"};
> + return p;
> + }
> +
> + DBUS_CPP_METHOD_DEF(requestSysState, com::canonical::powerd::Interface)
> + DBUS_CPP_METHOD_DEF(clearSysState, com::canonical::powerd::Interface)
> +};
> +}}}
> +
> +namespace
> +{
> +namespace impl
> +{
> +struct DisplayStateLock : public media::power::StateController::Lock<media::power::DisplayState>,
> + public std::enable_shared_from_this<DisplayStateLock>
> +{
> + // To safe us some typing
> + typedef std::shared_ptr<DisplayStateLock> Ptr;
> +
> + // We postpone releasing the display for this amount of time.
> + static boost::posix_time::seconds timeout_for_release()
> + {
> + return boost::posix_time::seconds{4};
> + }
> +
> + // The invalid cookie marker.
> + static constexpr const std::int32_t the_invalid_cookie{-1};
> +
> + DisplayStateLock(const media::power::StateController::Ptr& parent,
> + boost::asio::io_service& io_service,
> + const core::dbus::Object::Ptr& object)
> + : parent{parent},
> + timeout{io_service},
> + object{object},
> + cookie{the_invalid_cookie}
> + {
> + }
> +
> + // From core::ubuntu::media::power::StateController::Lock<DisplayState>
> + void request_acquire(media::power::DisplayState state) override
> + {
> + if (state == media::power::DisplayState::off)
> + return;
> +
> + std::weak_ptr<DisplayStateLock> wp{shared_from_this()};
> +
> + object->invoke_method_asynchronously_with_callback<com::canonical::Unity::Screen::keepDisplayOn, std::int32_t>([wp, state](const core::dbus::Result<std::int32_t>& result)
The lambda would read better if it were pushed to a new line like so:
[wp, state](const core::dbus::Result<std::int32_t>& result)
{
...
}
> + {
> + if (result.is_error()) // We should at least log the error case here.
Please log the error to stderr like your comment suggests.
> + return;
> +
> + if (auto sp = wp.lock())
> + {
> + sp->cookie = result.value();
> + sp->signals.acquired(state);
> + }
> + });
> + }
> +
> + void request_release(media::power::DisplayState state) override
> + {
> + if (state == media::power::DisplayState::off)
> + return;
> +
> + if (cookie == the_invalid_cookie)
> + return;
> +
> + std::weak_ptr<DisplayStateLock> wp{shared_from_this()};
> +
> + auto current_cookie(cookie);
> +
> + timeout.expires_from_now(timeout_for_release());
> + timeout.async_wait([wp, state, current_cookie](const boost::system::error_code& ec)
> + {
> + if (not ec)
What happens to the held display wakelock in the case that there is an error_code returned here by async_wait? We don't want to just abandon trying to release it.
> + {
> + if (auto sp = wp.lock())
> + {
> + sp->object->invoke_method_asynchronously_with_callback<com::canonical::Unity::Screen::removeDisplayOnRequest, void>([wp, state, current_cookie](const core::dbus::Result<void>& result)
The lambda would read better if it were pushed to a new line like so:
[wp, state, current_cookie](const core::dbus::Result<void>& result)
{
...
}
> + {
> + if (result.is_error()) // We should at least log the error case here.
> + return;
> +
> + if (auto sp = wp.lock())
> + {
> + sp->signals.released(state);
> +
> + // We might have issued a different request before and
> + // only call the display state done if the original cookie
> + // corresponds to the one we just gave up.
> + if (sp->cookie == current_cookie)
> + sp->cookie = the_invalid_cookie;
> + }
> +
> + }, current_cookie);
> + }
> + }
> + });
> + }
> +
> + // Emitted whenever the acquire request completes.
> + const core::Signal<media::power::DisplayState>& acquired() const override
> + {
> + return signals.acquired;
> + }
> +
> + // Emitted whenever the release request completes.
> + const core::Signal<media::power::DisplayState>& released() const override
> + {
> + return signals.released;
> + }
> +
> + media::power::StateController::Ptr parent;
> + boost::asio::deadline_timer timeout;
> + core::dbus::Object::Ptr object;
> + std::int32_t cookie;
> +
> + struct
> + {
> + core::Signal<media::power::DisplayState> acquired;
> + core::Signal<media::power::DisplayState> released;
> + } signals;
> +};
> +
> +struct SystemStateLock : public media::power::StateController::Lock<media::power::SystemState>
> +{
> + static constexpr const char* wake_lock_name
> + {
> + "media-hub-playback_lock"
> + };
> +
> + SystemStateLock(const media::power::StateController::Ptr& parent, const core::dbus::Object::Ptr& object)
> + : parent{parent},
> + object{object}
> + {
> + }
> +
> + // Informs the system that the caller would like
> + // the system to stay active.
> + void request_acquire(media::power::SystemState state) override
> + {
> + if (state == media::power::SystemState::suspend)
> + return;
> +
> + std::lock_guard<std::mutex> lg{cookie_store_guard};
> + if (cookie_store.count(state) > 0)
> + return;
> +
> + object->invoke_method_asynchronously_with_callback<com::canonical::powerd::Interface::requestSysState, std::string>([this, state](const core::dbus::Result<std::string>& result)
> + {
> + if (result.is_error()) // TODO(tvoss): We should log the error condition here.
> + return;
> +
> + std::lock_guard<std::mutex> lg{cookie_store_guard};
> +
> + cookie_store[state] = result.value();
> + signals.acquired(state);
> + }, std::string{wake_lock_name}, static_cast<std::int32_t>(state));
> + }
> +
> + // Informs the system that the caller does not
> + // require the system to stay active anymore.
> + void request_release(media::power::SystemState state) override
> + {
> + if (state == media::power::SystemState::suspend)
> + return;
> +
> + std::lock_guard<std::mutex> lg{cookie_store_guard};
> +
> + if (cookie_store.count(state) == 0)
> + return;
> +
> + object->invoke_method_asynchronously_with_callback<com::canonical::powerd::Interface::clearSysState, void>([this, state](const core::dbus::Result<void>& result)
> + {
> + if (result.is_error()) {/*TODO(tvoss): We should log the error condition here.*/}
Log to stderr like your comment suggests.
> +
> + std::lock_guard<std::mutex> lg{cookie_store_guard};
> +
> + cookie_store.erase(state);
> + signals.released(state);
> + }, cookie_store.at(state));
> + }
> +
> + // Emitted whenever the acquire request completes.
> + const core::Signal<media::power::SystemState>& acquired() const override
> + {
> + return signals.acquired;
> + }
> +
> + // Emitted whenever the release request completes.
> + const core::Signal<media::power::SystemState>& released() const override
> + {
> + return signals.released;
> + }
> +
> + std::mutex cookie_store_guard;
> + std::map<media::power::SystemState, std::string> cookie_store;
I think a clearer name would be "system_state_cookie_store" or even just "system_state_cookies". Also, please add a comment as to what the store is used for.
> + media::power::StateController::Ptr parent;
> + core::dbus::Object::Ptr object;
> + struct
> + {
> + core::Signal<media::power::SystemState> acquired;
> + core::Signal<media::power::SystemState> released;
> + } signals;
> +};
> +
> +struct StateController : public media::power::StateController,
> + public std::enable_shared_from_this<impl::StateController>
> +{
> + StateController(media::helper::ExternalServices& es)
> + : external_services{es},
> + powerd
> + {
> + core::dbus::Service::use_service<com::canonical::powerd::Interface>(external_services.system)
> + ->object_for_path(com::canonical::powerd::Interface::path())
> + },
> + unity_screen
> + {
> + core::dbus::Service::use_service<com::canonical::Unity::Screen>(external_services.system)
> + ->object_for_path(com::canonical::Unity::Screen::path())
> + }
> + {
> + }
> +
> + media::power::StateController::Lock<media::power::SystemState>::Ptr system_state_lock() override
> + {
> + return std::make_shared<impl::SystemStateLock>(shared_from_this(), powerd);
> + }
> +
> + media::power::StateController::Lock<media::power::DisplayState>::Ptr display_state_lock() override
> + {
> + return std::make_shared<impl::DisplayStateLock>(shared_from_this(), external_services.io_service, unity_screen);
> + }
> +
> + media::helper::ExternalServices& external_services;
> + core::dbus::Object::Ptr powerd;
> + core::dbus::Object::Ptr unity_screen;
> +};
> +}
> +}
> +
> +media::power::StateController::Ptr media::power::make_platform_default_state_controller(core::ubuntu::media::helper::ExternalServices& external_services)
> +{
> + return std::make_shared<impl::StateController>(external_services);
> +}
>
> === added file 'src/core/media/power/state_controller.h'
> --- src/core/media/power/state_controller.h 1970-01-01 00:00:00 +0000
> +++ src/core/media/power/state_controller.h 2014-11-26 12:42:47 +0000
> @@ -0,0 +1,102 @@
> +/*
> + * 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_POWER_STATE_CONTROLLER_H_
> +#define CORE_UBUNTU_MEDIA_POWER_STATE_CONTROLLER_H_
> +
> +#include <core/media/external_services.h>
> +
> +#include <core/property.h>
> +
> +#include <memory>
> +
> +namespace core
> +{
> +namespace ubuntu
> +{
> +namespace media
> +{
> +namespace power
> +{
> +// Enumerates all known power states of a display.
> +enum class DisplayState
> +{
> + // The display is off.
> + off = 0,
> + // The display is on.
> + on = 1
> +};
> +
> +// Enumerates known power states of the system.
> +enum class SystemState
> +{
> + // Note that callers will be notified of suspend state changes
> + // but may not request this state.
> + suspend = 0,
> + // The Active state will prevent system suspend
> + active = 1,
> + // Substate of Active with disabled proximity based blanking
> + blank_on_proximity = 2
> +};
> +
> +// Interface that enables observation of the system power state.
> +struct StateController
> +{
> + // To safe us some typing.
Typo, should be "save".
> + typedef std::shared_ptr<StateController> Ptr;
> +
> + // When acquired, ensures that the system stays active,
> + // and decreases the reference count when released.
> + template<typename State>
> + struct Lock
> + {
> + // To safe us some typing.
Typo, should be "save".
> + typedef std::shared_ptr<Lock> Ptr;
> +
> + Lock() = default;
> + virtual ~Lock() = default;
> +
> + // Informs the system that the caller would like
> + // the system to stay active.
> + virtual void request_acquire(State state) = 0;
> + // Informs the system that the caller does not
> + // require the system to stay active anymore.
> + virtual void request_release(State state) = 0;
> +
> + // Emitted whenever the acquire request completes.
> + virtual const core::Signal<State>& acquired() const = 0;
> + // Emitted whenever the release request completes.
> + virtual const core::Signal<State>& released() const = 0;
> + };
> +
> + StateController() = default;
> + virtual ~StateController() = default;
> +
> + // Returns a power::StateController::Lock<DisplayState> instance.
> + virtual Lock<DisplayState>::Ptr display_state_lock() = 0;
> + // Returns a power::StateController::Lock<SystemState> instance.
> + virtual Lock<SystemState>::Ptr system_state_lock() = 0;
> +};
> +
> +// Creates a StateController instance that connects to the platform default
> +// services to control system and display power states.
> +StateController::Ptr make_platform_default_state_controller(core::ubuntu::media::helper::ExternalServices&);
> +}
> +}
> +}
> +}
> +#endif // CORE_UBUNTU_MEDIA_POWER_STATE_CONTROLLER_H_
>
> === removed file 'src/core/media/powerd_service.h'
> --- src/core/media/powerd_service.h 2014-08-08 14:36:29 +0000
> +++ src/core/media/powerd_service.h 1970-01-01 00:00:00 +0000
> @@ -1,73 +0,0 @@
> -/*
> - * Copyright (C) 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/>.
> - *
> - * Author: Ricardo Mendoza <ricardo.mendoza at canonical.com>
> - */
> -
> -#include <core/dbus/dbus.h>
> -#include <core/dbus/fixture.h>
> -#include <core/dbus/object.h>
> -#include <core/dbus/property.h>
> -#include <core/dbus/service.h>
> -#include <core/dbus/interfaces/properties.h>
> -#include <core/dbus/types/stl/tuple.h>
> -#include <core/dbus/types/stl/vector.h>
> -
> -#include <core/dbus/asio/executor.h>
> -
> -#include <string>
> -#include <vector>
> -#include <chrono>
> -
> -namespace core
> -{
> -
> -struct Powerd
> -{
> - static std::string& name()
> - {
> - static std::string s = "com.canonical.powerd";
> - return s;
> - }
> -
> - struct requestSysState
> - {
> - static std::string name()
> - {
> - static std::string s = "requestSysState";
> - return s;
> - }
> -
> - static const std::chrono::milliseconds default_timeout() { return std::chrono::seconds{1}; }
> -
> - typedef Powerd Interface;
> - };
> -
> - struct clearSysState
> - {
> - static std::string name()
> - {
> - static std::string s = "clearSysState";
> - return s;
> - }
> -
> - static const std::chrono::milliseconds default_timeout() { return std::chrono::seconds{1}; }
> -
> - typedef Powerd Interface;
> - };
> -
> -};
> -
> -}
>
> === modified file 'src/core/media/server/server.cpp'
> --- src/core/media/server/server.cpp 2014-04-04 14:31:43 +0000
> +++ src/core/media/server/server.cpp 2014-11-26 12:42:47 +0000
> @@ -20,24 +20,127 @@
> #include <core/media/player.h>
> #include <core/media/track_list.h>
>
> -#include <hybris/media/media_codec_layer.h>
> -
> #include "core/media/service_implementation.h"
>
> +#include <core/posix/signal.h>
> +
> #include <iostream>
>
> namespace media = core::ubuntu::media;
>
> using namespace std;
>
> +#if defined(MEDIA_HUB_HAVE_HYBRIS_MEDIA_COMPAT_LAYER)
> +#include <hybris/media/media_codec_layer.h>
> +
> +namespace
> +{
> +// All platform-specific initialization routines go here.
> +void platform_init()
> +{
> + decoding_service_init();
> +}
> +}
> +#else // MEDIA_HUB_HAVE_HYBRIS_MEDIA_COMPAT_LAYER
> +namespace
> +{
> +// All platform-specific initialization routines go here.
> +void platform_init()
> +{
> + // Consciously left empty
> +}
> +}
> +#endif // MEDIA_HUB_HAVE_HYBRIS_MEDIA_COMPAT_LAYER
> +
> int main()
> {
> - // Init hybris-level DecodingService
> - decoding_service_init();
> - cout << "Starting DecodingService..." << endl;
> -
> - auto service = std::make_shared<media::ServiceImplementation>();
> - service->run();
> + auto trap = core::posix::trap_signals_for_all_subsequent_threads(
> + {
> + core::posix::Signal::sig_int,
> + core::posix::Signal::sig_term
> + });
> +
> + trap->signal_raised().connect([trap](core::posix::Signal)
> + {
> + trap->stop();
> + });
> +
> + // Init platform-specific functionality.
> + platform_init();
> +
> + // We keep track of our state.
> + bool shutdown_requested{false};
> +
> + // Our helper for connecting to external services.
> + core::ubuntu::media::helper::ExternalServices external_services;
> +
> + std::thread external_services_worker
Please add a comment for what the external_services_worker thread is used for.
> + {
> + [&shutdown_requested, &external_services]()
> + {
> + while (not shutdown_requested)
> + {
> + try
> + {
> + external_services.io_service.run();
Add a comment like "Start our server side event loop"
> + }
> + catch (const std::exception& e)
> + {
> + std::cerr << "Error while executing the underlying io_service: " << e.what() << std::endl;
> + }
> + catch (...)
> + {
> + std::cerr << "Error while executing the underlying io_service." << std::endl;
> + }
> + }
> + }
> + };
> +
> + // We assemble the configuration for executing the service now.
> + media::ServiceImplementation::Configuration service_config
> + {
> + external_services
> + };
> +
> + auto service = std::make_shared<media::ServiceImplementation>(service_config);
> +
> + std::thread service_worker
Please add a comment for what the service_worker thread is used for.
> + {
> + [&shutdown_requested, service]()
> + {
> + while (not shutdown_requested)
> + {
> + try
> + {
> + service->run();
> + }
> + catch (const std::exception& e)
> + {
> + std::cerr << "Recoverable error while executing the service: " << e.what() << std::endl;
> + }
> + catch (...)
> + {
> + std::cerr << "Recoverable error while executing the service." << std::endl;
> + }
> + }
> + }
> + };
> +
> + // We block on waiting for signals telling us to gracefully shutdown.
Who would signal trap->run() to return? Where does it happen in the code. Add to the comment here to document this.
> + trap->run();
> +
> + // Inform our workers that we should shutdown gracefully
> + shutdown_requested = true;
> +
> + // And stop execution of helper and actual service.
> + external_services.stop();
> + service->stop();
> +
> + if (external_services_worker.joinable())
Wouldn't you want to reverse the order of these two joins?
> + external_services_worker.join();
> +
> + if (service_worker.joinable())
> + service_worker.join();
>
> return 0;
> }
>
> === modified file 'src/core/media/service_implementation.cpp'
> --- src/core/media/service_implementation.cpp 2014-11-26 12:42:47 +0000
> +++ src/core/media/service_implementation.cpp 2014-11-26 12:42:47 +0000
> @@ -22,6 +22,7 @@
>
> #include "service_implementation.h"
>
> +#include "client_death_observer.h"
> #include "indicator_power_service.h"
> #include "call-monitor/call_monitor.h"
> #include "player_configuration.h"
> @@ -40,7 +41,6 @@
> #include <pulse/pulseaudio.h>
>
> #include "util/timeout.h"
> -#include "unity_screen_service.h"
>
> namespace media = core::ubuntu::media;
>
> @@ -48,10 +48,13 @@
>
> struct media::ServiceImplementation::Private
> {
> - Private()
> - : resume_key(std::numeric_limits<std::uint32_t>::max()),
> - keep_alive(io_service),
> - disp_cookie(0),
> + Private(const ServiceImplementation::Configuration& configuration)
> + : configuration(configuration),
> + resume_key(std::numeric_limits<std::uint32_t>::max()),
> + power_state_controller(media::power::make_platform_default_state_controller(configuration.external_services)),
> + display_state_lock(power_state_controller->display_state_lock()),
> + client_death_observer(media::platform_default_client_death_observer()),
> + recorder_observer(media::make_platform_default_recorder_observer()),
> pulse_mainloop_api(nullptr),
> pulse_context(nullptr),
> headphones_connected(false),
> @@ -59,13 +62,6 @@
> primary_idx(-1),
> call_monitor(new CallMonitor)
> {
> - bus = std::shared_ptr<dbus::Bus>(new dbus::Bus(core::dbus::WellKnownBus::session));
> - bus->install_executor(dbus::asio::make_executor(bus, io_service));
> - worker = std::move(std::thread([this]()
> - {
> - bus->run();
> - }));
> -
> // Spawn pulse watchdog
> pulse_mainloop = nullptr;
> pulse_worker = std::move(std::thread([this]()
> @@ -129,19 +125,11 @@
>
> // Connect the property change signal that will allow media-hub to take appropriate action
> // when the battery level reaches critical
> - auto stub_service = dbus::Service::use_service(bus, "com.canonical.indicator.power");
> + auto stub_service = dbus::Service::use_service(configuration.external_services.session, "com.canonical.indicator.power");
> indicator_power_session = stub_service->object_for_path(dbus::types::ObjectPath("/com/canonical/indicator/power/Battery"));
> power_level = indicator_power_session->get_property<core::IndicatorPower::PowerLevel>();
> is_warning = indicator_power_session->get_property<core::IndicatorPower::IsWarning>();
>
> - // Obtain session with Unity.Screen so that we request state when doing recording
> - auto bus = std::shared_ptr<dbus::Bus>(new dbus::Bus(core::dbus::WellKnownBus::system));
> - bus->install_executor(dbus::asio::make_executor(bus));
> -
> - auto uscreen_stub_service = dbus::Service::use_service(bus, dbus::traits::Service<core::UScreen>::interface_name());
> - uscreen_session = uscreen_stub_service->object_for_path(dbus::types::ObjectPath("/com/canonical/Unity/Screen"));
> -
> - recorder_observer = media::make_platform_default_recorder_observer();
> recorder_observer->recording_state().changed().connect([this](media::RecordingState state)
> {
> media_recording_state_changed(state);
> @@ -159,40 +147,16 @@
> pulse_mainloop = nullptr;
> }
>
> - bus->stop();
> -
> - if (worker.joinable())
> - worker.join();
> -
> if (pulse_worker.joinable())
> pulse_worker.join();
> }
>
> void media_recording_state_changed(media::RecordingState state)
> {
> - if (uscreen_session == nullptr)
> - return;
> -
> if (state == media::RecordingState::started)
> - {
> - if (disp_cookie > 0)
> - return;
> -
> - auto result = uscreen_session->invoke_method_synchronously<core::UScreen::keepDisplayOn, int>();
> - if (result.is_error())
> - throw std::runtime_error(result.error().print());
> - disp_cookie = result.value();
> - }
> + display_state_lock->request_acquire(media::power::DisplayState::on);
> else if (state == media::RecordingState::stopped)
> - {
> - if (disp_cookie != -1)
> - {
> - timeout(4000, true, [this](){
> - this->uscreen_session->invoke_method_synchronously<core::UScreen::removeDisplayOnRequest, void>(this->disp_cookie);
> - this->disp_cookie = -1;
> - });
> - }
> - }
> + display_state_lock->request_release(media::power::DisplayState::off);
> }
>
> pa_threaded_mainloop *mainloop()
> @@ -438,18 +402,16 @@
> }
> }
>
> + 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;
> - std::thread worker;
> - dbus::Bus::Ptr bus;
> - boost::asio::io_service io_service;
> - boost::asio::io_service::work keep_alive;
> + media::Player::PlayerKey resume_key;
> std::shared_ptr<dbus::Object> indicator_power_session;
> std::shared_ptr<core::dbus::Property<core::IndicatorPower::PowerLevel>> power_level;
> std::shared_ptr<core::dbus::Property<core::IndicatorPower::IsWarning>> is_warning;
> - int disp_cookie;
> - std::shared_ptr<dbus::Object> uscreen_session;
> + media::power::StateController::Ptr power_state_controller;
> + media::power::StateController::Lock<media::power::DisplayState>::Ptr display_state_lock;
> + media::ClientDeathObserver::Ptr client_death_observer;
> media::RecorderObserver::Ptr recorder_observer;
> // Pulse-specific
> pa_mainloop_api *pulse_mainloop_api;
> @@ -470,7 +432,7 @@
> std::list<media::Player::PlayerKey> paused_sessions;
> };
>
> -media::ServiceImplementation::ServiceImplementation() : d(new Private())
> +media::ServiceImplementation::ServiceImplementation(const Configuration& configuration) : d(new Private(configuration))
> {
> d->power_level->changed().connect([this](const core::IndicatorPower::PowerLevel::ValueType &level)
> {
> @@ -513,8 +475,16 @@
> std::shared_ptr<media::Player> media::ServiceImplementation::create_session(
> const media::Player::Configuration& conf)
> {
> - auto player = std::make_shared<media::PlayerImplementation>(
> - conf.identity, conf.bus, conf.session, shared_from_this(), conf.key);
> + auto player = std::make_shared<media::PlayerImplementation>(media::PlayerImplementation::Configuration
> + {
> + conf.identity,
> + conf.bus,
> + conf.session,
> + shared_from_this(),
> + conf.key,
> + d->client_death_observer,
> + d->power_state_controller
> + });
>
> auto key = conf.key;
> player->on_client_disconnected().connect([this, key]()
> @@ -524,7 +494,7 @@
> // 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->io_service.post([this, key]()
> + d->configuration.external_services.io_service.post([this, key]()
> {
> remove_player_for_key(key);
> });
>
> === modified file 'src/core/media/service_implementation.h'
> --- src/core/media/service_implementation.h 2014-10-31 07:49:33 +0000
> +++ src/core/media/service_implementation.h 2014-11-26 12:42:47 +0000
> @@ -20,6 +20,7 @@
> #define CORE_UBUNTU_MEDIA_SERVICE_IMPLEMENTATION_H_
>
> #include "service_skeleton.h"
> +#include "external_services.h"
>
> namespace core
> {
> @@ -27,13 +28,18 @@
> {
> namespace media
> {
> -
> class Player;
>
> class ServiceImplementation : public ServiceSkeleton
> {
> public:
> - ServiceImplementation ();
> + // All creation time arguments go here.
> + struct Configuration
> + {
> + helper::ExternalServices& external_services;
> + };
> +
> + ServiceImplementation (const Configuration& configuration);
> ~ServiceImplementation ();
>
> std::shared_ptr<Player> create_session(const Player::Configuration&);
>
> === removed file 'src/core/media/unity_screen_service.h'
> --- src/core/media/unity_screen_service.h 2014-06-16 22:28:53 +0000
> +++ src/core/media/unity_screen_service.h 1970-01-01 00:00:00 +0000
> @@ -1,72 +0,0 @@
> -/*
> - * Copyright (C) 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/>.
> - *
> - * Author: Alberto Aguirre <alberto.aguirre at canonical.com>
> - */
> -
> -#include <core/dbus/dbus.h>
> -#include <core/dbus/fixture.h>
> -#include <core/dbus/object.h>
> -#include <core/dbus/property.h>
> -#include <core/dbus/service.h>
> -#include <core/dbus/interfaces/properties.h>
> -#include <core/dbus/types/stl/tuple.h>
> -#include <core/dbus/types/stl/vector.h>
> -
> -#include <core/dbus/asio/executor.h>
> -
> -#include <string>
> -#include <vector>
> -#include <chrono>
> -
> -namespace core
> -{
> -
> -struct UScreen
> -{
> - static std::string& name()
> - {
> - static std::string s = "com.canonical.Unity.Screen";
> - return s;
> - }
> -
> - struct keepDisplayOn
> - {
> - static std::string name()
> - {
> - static std::string s = "keepDisplayOn";
> - return s;
> - }
> -
> - static const std::chrono::milliseconds default_timeout() { return std::chrono::seconds{1}; }
> -
> - typedef UScreen Interface;
> - };
> -
> - struct removeDisplayOnRequest
> - {
> - static std::string name()
> - {
> - static std::string s = "removeDisplayOnRequest";
> - return s;
> - }
> -
> - static const std::chrono::milliseconds default_timeout() { return std::chrono::seconds{1}; }
> -
> - typedef UScreen Interface;
> - };
> -};
> -
> -}
>
> === modified file 'tests/CMakeLists.txt'
> --- tests/CMakeLists.txt 2014-02-14 08:12:35 +0000
> +++ tests/CMakeLists.txt 2014-11-26 12:42:47 +0000
> @@ -50,5 +50,5 @@
> "COM_UBUNTU_MEDIA_SERVICE_AUDIO_SINK_NAME=fakesink"
> )
>
> -add_subdirectory(acceptance-tests)
> +# add_subdirectory(acceptance-tests)
> add_subdirectory(unit-tests)
>
--
https://code.launchpad.net/~thomas-voss/media-hub/introduce-power-controller-interface/+merge/242905
Your team Ubuntu Phablet Team is subscribed to branch lp:media-hub.
More information about the Ubuntu-reviews
mailing list