[Merge] lp:~jhodapp/media-hub/fix-zero-position-while-seeking into lp:media-hub
Thomas Voß
thomas.voss at canonical.com
Tue Mar 10 20:36:51 UTC 2015
Review: Needs Fixing
One comment inline.
Diff comments:
> === modified file 'include/core/media/player.h'
> --- include/core/media/player.h 2015-03-10 20:29:56 +0000
> +++ include/core/media/player.h 2015-03-10 20:29:57 +0000
> @@ -168,6 +168,7 @@
> virtual core::Property<Lifetime>& lifetime() = 0;
>
> virtual const core::Signal<int64_t>& seeked_to() const = 0;
> + virtual const core::Signal<void>& about_to_finish() const = 0;
> virtual const core::Signal<void>& end_of_stream() const = 0;
> virtual core::Signal<PlaybackStatus>& playback_status_changed() = 0;
> virtual const core::Signal<video::Dimensions>& video_dimension_changed() const = 0;
>
> === modified file 'src/core/media/gstreamer/engine.cpp'
> --- src/core/media/gstreamer/engine.cpp 2015-03-10 20:29:56 +0000
> +++ src/core/media/gstreamer/engine.cpp 2015-03-10 20:29:57 +0000
> @@ -354,9 +354,9 @@
>
> if (result)
> {
> + d->playback_status_changed(media::Player::PlaybackStatus::playing);
> + cout << __PRETTY_FUNCTION__ << endl;
> d->state = media::Engine::State::playing;
> - cout << "play" << endl;
> - d->playback_status_changed(media::Player::PlaybackStatus::playing);
> }
>
> return result;
> @@ -372,8 +372,8 @@
>
> if (result)
> {
> + cout << __PRETTY_FUNCTION__ << endl;
> d->state = media::Engine::State::stopped;
> - cout << "stop" << endl;
> d->playback_status_changed(media::Player::PlaybackStatus::stopped);
> }
>
> @@ -386,8 +386,8 @@
>
> if (result)
> {
> + cout << __PRETTY_FUNCTION__ << endl;
> d->state = media::Engine::State::paused;
> - cout << "pause" << endl;
> d->playback_status_changed(media::Player::PlaybackStatus::paused);
> }
>
>
> === modified file 'src/core/media/gstreamer/playbin.cpp'
> --- src/core/media/gstreamer/playbin.cpp 2015-03-10 20:29:56 +0000
> +++ src/core/media/gstreamer/playbin.cpp 2015-03-10 20:29:57 +0000
> @@ -72,6 +72,7 @@
> void gstreamer::Playbin::about_to_finish(GstElement*, gpointer user_data)
> {
> auto thiz = static_cast<Playbin*>(user_data);
> + thiz->allow_seeking = false;
> thiz->signals.about_to_finish();
> }
>
> @@ -97,7 +98,9 @@
> this,
> std::placeholders::_1))),
> is_seeking(false),
> - player_lifetime(media::Player::Lifetime::normal)
> + allow_seeking(true),
> + player_lifetime(media::Player::Lifetime::normal),
> + is_eos(false)
> {
> if (!pipeline)
> throw std::runtime_error("Could not create pipeline for playbin.");
> @@ -159,6 +162,8 @@
> std::cout << "Failed to reset the pipeline state. Client reconnect may not function properly." << std::endl;
> }
> file_type = MEDIA_FILE_TYPE_NONE;
> + allow_seeking = true;
> + is_eos = false;
> }
>
> void gstreamer::Playbin::on_new_message(const Bus::Message& message)
> @@ -199,6 +204,7 @@
> }
> break;
> case GST_MESSAGE_EOS:
> + is_eos = true;
> signals.on_end_of_stream();
> default:
> break;
> @@ -334,6 +340,18 @@
> int64_t pos = 0;
> gst_element_query_position (pipeline, GST_FORMAT_TIME, &pos);
>
> + static uint64_t previous_pos = 0;
I don't think we can get away with a static here. The previous_pos would be shared across player instances, and one client-specific value would override another client-specific one. I think we will instead need an instance variable.
> + // This prevents a 0 position from being reported to the app which happens while seeking.
> + // This is covering over a GStreamer issue
> + if ((static_cast<uint64_t>(pos) < duration()) && is_seeking && pos == 0)
> + {
> + return previous_pos;
> + }
> +
> + // Save the current position to use just in case it's needed the next time position is
> + // requested
> + previous_pos = static_cast<uint64_t>(pos);
> +
> // FIXME: this should be int64_t, but dbus-cpp doesn't seem to handle it correctly
> return static_cast<uint64_t>(pos);
> }
> @@ -358,7 +376,7 @@
> file_type = MEDIA_FILE_TYPE_VIDEO;
> else if (is_audio_file(uri))
> file_type = MEDIA_FILE_TYPE_AUDIO;
> -
> +
> request_headers = headers;
> }
>
> @@ -366,7 +384,7 @@
> {
> if (source == NULL || request_headers.empty())
> return;
> -
> +
> if (request_headers.find("Cookie") != request_headers.end()) {
> if (g_object_class_find_property(G_OBJECT_GET_CLASS(source),
> "cookies") != NULL) {
> @@ -375,7 +393,7 @@
> g_strfreev(cookies);
> }
> }
> -
> +
> if (request_headers.find("User-Agent") != request_headers.end()) {
> if (g_object_class_find_property(G_OBJECT_GET_CLASS(source),
> "user-agent") != NULL) {
> @@ -426,10 +444,16 @@
> break;
> }
>
> + if (is_eos)
> + {
> + std::cout << "** is_eos == true, not sending video dimensions to client" << std::endl;
> + }
> +
> // We only should query the pipeline if we actually succeeded in
> // setting the requested state.
> - if (result && new_state == GST_STATE_PLAYING)
> + if (result && new_state == GST_STATE_PLAYING && !is_eos)
> {
> + allow_seeking = true;
> // Get the video height/width from the video sink
> try
> {
> @@ -457,10 +481,10 @@
> {
> is_seeking = true;
> return gst_element_seek_simple(
> - pipeline,
> - GST_FORMAT_TIME,
> - (GstSeekFlags)(GST_SEEK_FLAG_FLUSH | GST_SEEK_FLAG_KEY_UNIT),
> - ms.count() * 1000);
> + pipeline,
> + GST_FORMAT_TIME,
> + (GstSeekFlags)(GST_SEEK_FLAG_FLUSH | GST_SEEK_FLAG_KEY_UNIT),
> + ms.count() * 1000);
> }
>
> core::ubuntu::media::video::Dimensions gstreamer::Playbin::get_video_dimensions() const
>
> === modified file 'src/core/media/gstreamer/playbin.h'
> --- src/core/media/gstreamer/playbin.h 2015-03-10 20:29:56 +0000
> +++ src/core/media/gstreamer/playbin.h 2015-03-10 20:29:57 +0000
> @@ -112,8 +112,10 @@
> GstElement* video_sink;
> core::Connection on_new_message_connection;
> bool is_seeking;
> + bool allow_seeking;
> core::ubuntu::media::Player::HeadersType request_headers;
> core::ubuntu::media::Player::Lifetime player_lifetime;
> + bool is_eos;
> struct
> {
> core::Signal<void> about_to_finish;
>
> === modified file 'src/core/media/mpris/player.h'
> --- src/core/media/mpris/player.h 2015-03-10 20:29:56 +0000
> +++ src/core/media/mpris/player.h 2015-03-10 20:29:57 +0000
> @@ -136,6 +136,7 @@
> struct Signals
> {
> DBUS_CPP_SIGNAL_DEF(Seeked, Player, std::int64_t)
> + DBUS_CPP_SIGNAL_DEF(AboutToFinish, Player, void)
> DBUS_CPP_SIGNAL_DEF(EndOfStream, Player, void)
> DBUS_CPP_SIGNAL_DEF(PlaybackStatusChanged, Player, core::ubuntu::media::Player::PlaybackStatus)
> DBUS_CPP_SIGNAL_DEF(VideoDimensionChanged, Player, core::ubuntu::media::video::Dimensions)
> @@ -246,6 +247,7 @@
> signals
> {
> configuration.object->template get_signal<Signals::Seeked>(),
> + configuration.object->template get_signal<Signals::AboutToFinish>(),
> configuration.object->template get_signal<Signals::EndOfStream>(),
> configuration.object->template get_signal<Signals::PlaybackStatusChanged>(),
> configuration.object->template get_signal<Signals::VideoDimensionChanged>(),
> @@ -373,6 +375,7 @@
> struct
> {
> typename core::dbus::Signal<Signals::Seeked, Signals::Seeked::ArgumentType>::Ptr seeked_to;
> + typename core::dbus::Signal<Signals::AboutToFinish, Signals::AboutToFinish::ArgumentType>::Ptr about_to_finish;
> typename core::dbus::Signal<Signals::EndOfStream, Signals::EndOfStream::ArgumentType>::Ptr end_of_stream;
> typename core::dbus::Signal<Signals::PlaybackStatusChanged, Signals::PlaybackStatusChanged::ArgumentType>::Ptr playback_status_changed;
> typename core::dbus::Signal<Signals::VideoDimensionChanged, Signals::VideoDimensionChanged::ArgumentType>::Ptr video_dimension_changed;
>
> === modified file 'src/core/media/player_implementation.cpp'
> --- src/core/media/player_implementation.cpp 2015-03-10 20:29:56 +0000
> +++ src/core/media/player_implementation.cpp 2015-03-10 20:29:57 +0000
> @@ -206,7 +206,7 @@
> if (--system_wakelock_count == 0)
> {
> std::cout << "Clearing system wakelock." << std::endl;
> - system_state_lock->request_release(media::power::SystemState::active);
> + system_state_lock->request_release(media::power::SystemState::active);
> }
> break;
> case wakelock_clear_t::WAKELOCK_CLEAR_DISPLAY:
> @@ -359,6 +359,8 @@
>
> d->engine->about_to_finish_signal().connect([this]()
> {
> + Parent::about_to_finish()();
> +
> if (d->track_list->has_next())
> {
> Track::UriType uri = d->track_list->query_uri_for_track(d->track_list->next());
> @@ -397,7 +399,7 @@
> });
>
> d->engine->error_signal().connect([this](const Player::Error& e)
> - {
> + {
> Parent::error()(e);
> });
> }
>
> === modified file 'src/core/media/player_skeleton.cpp'
> --- src/core/media/player_skeleton.cpp 2015-03-10 20:29:56 +0000
> +++ src/core/media/player_skeleton.cpp 2015-03-10 20:29:57 +0000
> @@ -58,6 +58,7 @@
> signals
> {
> skeleton.signals.seeked_to,
> + skeleton.signals.about_to_finish,
> skeleton.signals.end_of_stream,
> skeleton.signals.playback_status_changed,
> skeleton.signals.video_dimension_changed,
> @@ -237,11 +238,13 @@
> {
> typedef core::dbus::Signal<mpris::Player::Signals::Seeked, mpris::Player::Signals::Seeked::ArgumentType> DBusSeekedToSignal;
> typedef core::dbus::Signal<mpris::Player::Signals::EndOfStream, mpris::Player::Signals::EndOfStream::ArgumentType> DBusEndOfStreamSignal;
> + typedef core::dbus::Signal<mpris::Player::Signals::AboutToFinish, mpris::Player::Signals::AboutToFinish::ArgumentType> DBusAboutToFinishSignal;
> 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;
>
> 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,
> @@ -252,6 +255,11 @@
> remote_seeked->emit(value);
> });
>
> + about_to_finish.connect([remote_atf]()
> + {
> + remote_atf->emit();
> + });
> +
> end_of_stream.connect([remote_eos]()
> {
> remote_eos->emit();
> @@ -274,6 +282,7 @@
> }
>
> core::Signal<int64_t> seeked_to;
> + core::Signal<void> about_to_finish;
> core::Signal<void> end_of_stream;
> core::Signal<media::Player::PlaybackStatus> playback_status_changed;
> core::Signal<media::video::Dimensions> video_dimension_changed;
> @@ -558,6 +567,16 @@
> return d->signals.seeked_to;
> }
>
> +const core::Signal<void>& media::PlayerSkeleton::about_to_finish() const
> +{
> + return d->signals.about_to_finish;
> +}
> +
> +core::Signal<void>& media::PlayerSkeleton::about_to_finish()
> +{
> + return d->signals.about_to_finish;
> +}
> +
> const core::Signal<void>& media::PlayerSkeleton::end_of_stream() const
> {
> return d->signals.end_of_stream;
>
> === modified file 'src/core/media/player_skeleton.h'
> --- src/core/media/player_skeleton.h 2015-03-10 20:29:56 +0000
> +++ src/core/media/player_skeleton.h 2015-03-10 20:29:57 +0000
> @@ -92,8 +92,8 @@
> virtual core::Property<Lifetime>& lifetime();
>
> virtual const core::Signal<int64_t>& seeked_to() const;
> + virtual const core::Signal<void>& about_to_finish() const;
> virtual const core::Signal<void>& end_of_stream() const;
> - virtual core::Signal<PlaybackStatus>& playback_status_changed();
> virtual const core::Signal<video::Dimensions>& video_dimension_changed() const;
> virtual const core::Signal<Error>& error() const;
>
> @@ -115,7 +115,9 @@
> virtual core::Property<Orientation>& orientation();
>
> virtual core::Signal<int64_t>& seeked_to();
> + virtual core::Signal<void>& about_to_finish();
> virtual core::Signal<void>& end_of_stream();
> + virtual core::Signal<PlaybackStatus>& playback_status_changed();
> virtual core::Signal<video::Dimensions>& video_dimension_changed();
> virtual core::Signal<Error>& error();
>
>
> === modified file 'src/core/media/player_stub.cpp'
> --- src/core/media/player_stub.cpp 2015-03-10 20:29:56 +0000
> +++ src/core/media/player_stub.cpp 2015-03-10 20:29:57 +0000
> @@ -76,6 +76,7 @@
> signals
> {
> object->get_signal<mpris::Player::Signals::Seeked>(),
> + object->get_signal<mpris::Player::Signals::AboutToFinish>(),
> object->get_signal<mpris::Player::Signals::EndOfStream>(),
> object->get_signal<mpris::Player::Signals::PlaybackStatusChanged>(),
> object->get_signal<mpris::Player::Signals::VideoDimensionChanged>(),
> @@ -123,16 +124,19 @@
> {
> typedef core::dbus::Signal<mpris::Player::Signals::Seeked, mpris::Player::Signals::Seeked::ArgumentType> DBusSeekedToSignal;
> typedef core::dbus::Signal<mpris::Player::Signals::EndOfStream, mpris::Player::Signals::EndOfStream::ArgumentType> DBusEndOfStreamSignal;
> + typedef core::dbus::Signal<mpris::Player::Signals::AboutToFinish, mpris::Player::Signals::AboutToFinish::ArgumentType> DBusAboutToFinishSignal;
> 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;
>
> 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<DBusErrorSignal>& e)
> : seeked_to(),
> + about_to_finish(),
> end_of_stream(),
> playback_status_changed(),
> video_dimension_changed(),
> @@ -140,6 +144,7 @@
> dbus
> {
> seeked,
> + atf,
> eos,
> status,
> d,
> @@ -158,6 +163,12 @@
> end_of_stream();
> });
>
> + dbus.about_to_finish->connect([this]()
> + {
> + std::cout << "AboutToFinish signal arrived via the bus." << std::endl;
> + about_to_finish();
> + });
> +
> dbus.playback_status_changed->connect([this](const media::Player::PlaybackStatus& status)
> {
> std::cout << "PlaybackStatusChanged signal arrived via the bus." << std::endl;
> @@ -178,6 +189,7 @@
> }
>
> core::Signal<int64_t> seeked_to;
> + core::Signal<void> about_to_finish;
> core::Signal<void> end_of_stream;
> core::Signal<media::Player::PlaybackStatus> playback_status_changed;
> core::Signal<media::video::Dimensions> video_dimension_changed;
> @@ -186,6 +198,7 @@
> struct DBus
> {
> std::shared_ptr<DBusSeekedToSignal> seeked_to;
> + std::shared_ptr<DBusAboutToFinishSignal> about_to_finish;
> std::shared_ptr<DBusEndOfStreamSignal> end_of_stream;
> std::shared_ptr<DBusPlaybackStatusChangedSignal> playback_status_changed;
> std::shared_ptr<DBusVideoDimensionChangedSignal> video_dimension_changed;
> @@ -434,6 +447,11 @@
> return d->signals.seeked_to;
> }
>
> +const core::Signal<void>& media::PlayerStub::about_to_finish() const
> +{
> + return d->signals.about_to_finish;
> +}
> +
> const core::Signal<void>& media::PlayerStub::end_of_stream() const
> {
> return d->signals.end_of_stream;
>
> === modified file 'src/core/media/player_stub.h'
> --- src/core/media/player_stub.h 2015-03-10 20:29:56 +0000
> +++ src/core/media/player_stub.h 2015-03-10 20:29:57 +0000
> @@ -86,6 +86,7 @@
> virtual core::Property<Lifetime>& lifetime();
>
> virtual const core::Signal<int64_t>& seeked_to() const;
> + virtual const core::Signal<void>& about_to_finish() const;
> virtual const core::Signal<void>& end_of_stream() const;
> virtual core::Signal<PlaybackStatus>& playback_status_changed();
> virtual const core::Signal<video::Dimensions>& video_dimension_changed() const;
>
> === modified file 'src/core/media/power/state_controller.cpp'
> --- src/core/media/power/state_controller.cpp 2015-03-10 20:29:56 +0000
> +++ src/core/media/power/state_controller.cpp 2015-03-10 20:29:57 +0000
> @@ -128,42 +128,38 @@
> if (cookie == the_invalid_cookie)
> return;
>
> - std::weak_ptr<DisplayStateLock> wp{shared_from_this()};
> + // We make sure that we keep ourselves alive to make sure
> + // that release requests are always correctly issued.
> + auto sp = 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)
> + timeout.async_wait([sp, state, current_cookie](const boost::system::error_code& ec)
> {
> // We only return early from the timeout handler if the operation has been
> // explicitly aborted before.
> if (ec == boost::asio::error::operation_aborted)
> return;
>
> - 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)
> + sp->object->invoke_method_asynchronously_with_callback<com::canonical::Unity::Screen::removeDisplayOnRequest, void>(
> + [sp, state, current_cookie](const core::dbus::Result<void>& result)
> + {
> + if (result.is_error())
> {
> - if (result.is_error())
> - {
> - std::cerr << result.error().print() << std::endl;
> - 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);
> - }
> + std::cerr << result.error().print() << std::endl;
> + return;
> + }
> +
> + 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);
> });
> }
>
>
> === modified file 'src/core/media/service_implementation.cpp'
> --- src/core/media/service_implementation.cpp 2015-03-10 20:29:56 +0000
> +++ src/core/media/service_implementation.cpp 2015-03-10 20:29:57 +0000
> @@ -80,7 +80,7 @@
> media::audio::OutputObserver::Ptr audio_output_observer;
> media::apparmor::ubuntu::RequestContextResolver::Ptr request_context_resolver;
> media::apparmor::ubuntu::RequestAuthenticator::Ptr request_authenticator;
> - media::audio::OutputObserver audio_output_state;
> + media::audio::OutputState audio_output_state;
>
> media::telephony::CallMonitor::Ptr call_monitor;
> std::list<media::Player::PlayerKey> paused_sessions;
>
--
https://code.launchpad.net/~jhodapp/media-hub/fix-zero-position-while-seeking/+merge/252509
Your team Ubuntu Phablet Team is subscribed to branch lp:media-hub.
More information about the Ubuntu-reviews
mailing list