[Merge] lp:~lorn-potter/media-hub/buffering-signal into lp:media-hub
Jim Hodapp
jim.hodapp at canonical.com
Tue Jun 7 23:50:01 UTC 2016
Review: Needs Fixing code
Several comment inline below.
Diff comments:
>
> === modified file 'src/core/media/gstreamer/engine.cpp'
> --- src/core/media/gstreamer/engine.cpp 2016-04-06 15:28:29 +0000
> +++ src/core/media/gstreamer/engine.cpp 2016-06-07 20:30:55 +0000
> @@ -347,7 +352,14 @@
> std::bind(
> &Private::on_video_dimension_changed,
> this,
> + std::placeholders::_1))),
> + on_buffering_changed_connection(
> + playbin.signals.on_buffering_changed.connect(
> + std::bind(
> + &Private::on_buffering_changed,
> + this,
> std::placeholders::_1)))
> +
Remove extra line
> {
> }
>
>
> === modified file 'src/core/media/gstreamer/playbin.cpp'
> --- src/core/media/gstreamer/playbin.cpp 2016-04-06 15:28:29 +0000
> +++ src/core/media/gstreamer/playbin.cpp 2016-06-07 20:30:55 +0000
> @@ -300,6 +300,11 @@
> break;
> case GST_MESSAGE_EOS:
> signals.on_end_of_stream();
> + break;
> + case GST_MESSAGE_BUFFERING:
> + MH_DEBUG("Got buffering message");
Remove this debug line.
> + signals.on_buffering_changed(message.detail.buffering.percent);
> + break;
> default:
> break;
> }
>
> === modified file 'src/core/media/mpris/player.h'
> --- src/core/media/mpris/player.h 2016-02-19 16:14:42 +0000
> +++ src/core/media/mpris/player.h 2016-06-07 20:30:55 +0000
> @@ -157,6 +157,7 @@
> DBUS_CPP_SIGNAL_DEF(PlaybackStatusChanged, Player, core::ubuntu::media::Player::PlaybackStatus)
> DBUS_CPP_SIGNAL_DEF(VideoDimensionChanged, Player, core::ubuntu::media::video::Dimensions)
> DBUS_CPP_SIGNAL_DEF(Error, Player, core::ubuntu::media::Player::Error)
> + DBUS_CPP_SIGNAL_DEF(Buffering, Player, std::int64_t)
Why is this int64_t when the actual internal bits not over the bus are just a standard int? A standard int should do just fine here as well.
> };
>
> struct Properties
>
> === modified file 'src/core/media/player_implementation.cpp'
> --- src/core/media/player_implementation.cpp 2016-05-03 19:08:00 +0000
> +++ src/core/media/player_implementation.cpp 2016-06-07 20:30:55 +0000
> @@ -552,6 +552,12 @@
> Parent::seeked_to()(value);
> });
>
> + d->engine->on_buffering_changed_signal().connect([this](int value)
> + {
> + MH_DEBUG("buffering %d",value);
Remove debug line.
> + Parent::buffering_changed()(value);
> + });
> +
> d->engine->end_of_stream_signal().connect([this]()
> {
> Parent::end_of_stream()();
>
> === modified file 'src/core/media/player_skeleton.cpp'
> --- src/core/media/player_skeleton.cpp 2016-04-06 15:28:29 +0000
> +++ src/core/media/player_skeleton.cpp 2016-06-07 20:30:55 +0000
> @@ -306,13 +307,15 @@
> typedef core::dbus::Signal<mpris::Player::Signals::PlaybackStatusChanged, mpris::Player::Signals::PlaybackStatusChanged::ArgumentType> DBusPlaybackStatusChangedSignal;
> typedef core::dbus::Signal<mpris::Player::Signals::VideoDimensionChanged, mpris::Player::Signals::VideoDimensionChanged::ArgumentType> DBusVideoDimensionChangedSignal;
> typedef core::dbus::Signal<mpris::Player::Signals::Error, mpris::Player::Signals::Error::ArgumentType> DBusErrorSignal;
> + typedef core::dbus::Signal<mpris::Player::Signals::Buffering, mpris::Player::Signals::Buffering::ArgumentType> DBusBufferingChangedSignal;
>
> Signals(const std::shared_ptr<DBusSeekedToSignal>& remote_seeked,
> const std::shared_ptr<DBusAboutToFinishSignal>& remote_atf,
> const std::shared_ptr<DBusEndOfStreamSignal>& remote_eos,
> const std::shared_ptr<DBusPlaybackStatusChangedSignal>& remote_playback_status_changed,
> const std::shared_ptr<DBusVideoDimensionChangedSignal>& remote_video_dimension_changed,
> - const std::shared_ptr<DBusErrorSignal>& remote_error)
> + const std::shared_ptr<DBusErrorSignal>& remote_error,
> + const std::shared_ptr<DBusBufferingChangedSignal>&remote_buffering_changed)
Add a space after &
> {
> seeked_to.connect([remote_seeked](std::uint64_t value)
> {
> @@ -675,3 +685,15 @@
> {
> return d->signals.error;
> }
> +
> +const core::Signal<int>& media::PlayerSkeleton::buffering_changed() const
> +{
> + MH_DEBUG("buffering changed");
Remove debug line.
> + return d->signals.buffering_changed;
> +}
> +
> +core::Signal<int>& media::PlayerSkeleton::buffering_changed()
> +{
> + MH_DEBUG("buffering changed");
Remove debug line.
> + return d->signals.buffering_changed;
> +}
>
> === modified file 'src/core/media/player_stub.cpp'
> --- src/core/media/player_stub.cpp 2016-04-06 15:28:29 +0000
> +++ src/core/media/player_stub.cpp 2016-06-07 20:30:55 +0000
> @@ -137,12 +138,14 @@
> typedef core::dbus::Signal<mpris::Player::Signals::PlaybackStatusChanged, mpris::Player::Signals::PlaybackStatusChanged::ArgumentType> DBusPlaybackStatusChangedSignal;
> typedef core::dbus::Signal<mpris::Player::Signals::VideoDimensionChanged, mpris::Player::Signals::VideoDimensionChanged::ArgumentType> DBusVideoDimensionChangedSignal;
> typedef core::dbus::Signal<mpris::Player::Signals::Error, mpris::Player::Signals::Error::ArgumentType> DBusErrorSignal;
> + typedef core::dbus::Signal<mpris::Player::Signals::Buffering, mpris::Player::Signals::Buffering::ArgumentType> DBusBufferingChangedSignal;
>
> Signals(const std::shared_ptr<DBusSeekedToSignal>& seeked,
> const std::shared_ptr<DBusAboutToFinishSignal>& atf,
> const std::shared_ptr<DBusEndOfStreamSignal>& eos,
> const std::shared_ptr<DBusPlaybackStatusChangedSignal>& status,
> const std::shared_ptr<DBusVideoDimensionChangedSignal>& d,
> + const std::shared_ptr<DBusBufferingChangedSignal>& bufferPercent,
Change to "bp" instead of bufferPercent. We don't tend to use camel-case for this code base.
> const std::shared_ptr<DBusErrorSignal>& e)
> : seeked_to(),
> about_to_finish(),
> @@ -157,7 +161,8 @@
> eos,
> status,
> d,
> - e
> + e,
> + bufferPercent
Same as last comment.
> }
> {
> dbus.seeked_to->connect([this](std::uint64_t value)
> @@ -196,6 +201,12 @@
> MH_DEBUG("Error signal arrived via the bus (error: %s)", e);
> error(e);
> });
> +
> + dbus.buffering_changed->connect([this](int bufferPercent)
> + {
> + MH_DEBUG("BufferingChanged signal arrived via the bus.");
Add the percent value to this debug line in the same manner as the other examples above. Also rename to "value" or something else that doesn't use camel-case.
> + buffering_changed(bufferPercent);
> + });
> }
>
> core::Signal<int64_t> seeked_to;
> @@ -516,3 +529,9 @@
> {
> return d->signals.error;
> }
> +
> +const core::Signal<int>& media::PlayerStub::buffering_changed() const
> +{
> + MH_DEBUG("buffering changed");
Remove debug line.
> + return d->signals.buffering_changed;
> +}
--
https://code.launchpad.net/~lorn-potter/media-hub/buffering-signal/+merge/296730
Your team Ubuntu Phablet Team is subscribed to branch lp:media-hub.
More information about the Ubuntu-reviews
mailing list