From 87aed17a46291f7ef8cc3eba6fb6adebb8aa2f2d Mon Sep 17 00:00:00 2001 From: Zaggy1024 Date: Fri, 11 Nov 2022 07:55:20 -0600 Subject: [PATCH] VideoPlayer: Make PlaybackManager use OwnPtr VideoPlayerWidget was keeping a reference to PlaybackManager when changing files, so the old and new managers would both send frames to be presented at the same time, causing it to flicker back and forth between the two videos. However, PlaybackManager no longer relies on event bubbling to pass events to its parent. By changing it to send events directly to an Object, it can avoid being ref counted, so that it will get destroyed with its containing object and stop sending events. --- .../VideoPlayer/VideoPlayerWidget.cpp | 9 +++++- .../VideoPlayer/VideoPlayerWidget.h | 3 +- .../Libraries/LibVideo/PlaybackManager.cpp | 29 +++++-------------- Userland/Libraries/LibVideo/PlaybackManager.h | 13 ++++----- 4 files changed, 23 insertions(+), 31 deletions(-) diff --git a/Userland/Applications/VideoPlayer/VideoPlayerWidget.cpp b/Userland/Applications/VideoPlayer/VideoPlayerWidget.cpp index 31d5972503..f4d53ca8c4 100644 --- a/Userland/Applications/VideoPlayer/VideoPlayerWidget.cpp +++ b/Userland/Applications/VideoPlayer/VideoPlayerWidget.cpp @@ -69,9 +69,15 @@ VideoPlayerWidget::VideoPlayerWidget(GUI::Window& window) m_volume_slider->set_fixed_width(100); } +void VideoPlayerWidget::close_file() +{ + if (m_playback_manager) + m_playback_manager = nullptr; +} + void VideoPlayerWidget::open_file(StringView filename) { - auto load_file_result = Video::PlaybackManager::from_file(this, filename); + auto load_file_result = Video::PlaybackManager::from_file(*this, filename); if (load_file_result.is_error()) { on_decoding_error(load_file_result.release_error()); @@ -81,6 +87,7 @@ void VideoPlayerWidget::open_file(StringView filename) m_path = filename; update_title(); + close_file(); m_playback_manager = load_file_result.release_value(); resume_playback(); } diff --git a/Userland/Applications/VideoPlayer/VideoPlayerWidget.h b/Userland/Applications/VideoPlayer/VideoPlayerWidget.h index e52f5e920c..9f3abe1ddc 100644 --- a/Userland/Applications/VideoPlayer/VideoPlayerWidget.h +++ b/Userland/Applications/VideoPlayer/VideoPlayerWidget.h @@ -22,6 +22,7 @@ class VideoPlayerWidget final : public GUI::Widget { C_OBJECT(VideoPlayerWidget) public: + void close_file(); void open_file(StringView filename); void resume_playback(); void pause_playback(); @@ -59,7 +60,7 @@ private: RefPtr m_cycle_sizing_modes_action; RefPtr m_volume_slider; - RefPtr m_playback_manager; + OwnPtr m_playback_manager; }; } diff --git a/Userland/Libraries/LibVideo/PlaybackManager.cpp b/Userland/Libraries/LibVideo/PlaybackManager.cpp index c6579a8aa0..51b717e8da 100644 --- a/Userland/Libraries/LibVideo/PlaybackManager.cpp +++ b/Userland/Libraries/LibVideo/PlaybackManager.cpp @@ -14,7 +14,7 @@ namespace Video { -DecoderErrorOr> PlaybackManager::from_file(Object* event_handler, StringView filename) +DecoderErrorOr> PlaybackManager::from_file(Core::Object& event_handler, StringView filename) { NonnullOwnPtr demuxer = TRY(MatroskaDemuxer::from_file(filename)); auto video_tracks = demuxer->get_tracks_for_type(TrackType::Video); @@ -24,12 +24,11 @@ DecoderErrorOr> PlaybackManager::from_file(Object dbgln_if(PLAYBACK_MANAGER_DEBUG, "Selecting video track number {}", track.identifier()); - NonnullOwnPtr decoder = make(); - return PlaybackManager::construct(event_handler, demuxer, track, decoder); + return make(event_handler, demuxer, track, make()); } -PlaybackManager::PlaybackManager(Object* event_handler, NonnullOwnPtr& demuxer, Track video_track, NonnullOwnPtr& decoder) - : Object(event_handler) +PlaybackManager::PlaybackManager(Core::Object& event_handler, NonnullOwnPtr& demuxer, Track video_track, NonnullOwnPtr&& decoder) + : m_event_handler(event_handler) , m_main_loop(Core::EventLoop::current()) , m_demuxer(move(demuxer)) , m_selected_video_track(video_track) @@ -68,22 +67,10 @@ void PlaybackManager::set_playback_status(PlaybackStatus status) m_present_timer->stop(); } - m_main_loop.post_event(*this, make(status, old_status)); + m_main_loop.post_event(m_event_handler, make(status, old_status)); } } -void PlaybackManager::event(Core::Event& event) -{ - if (event.type() == DecoderErrorOccurred) { - auto& error_event = static_cast(event); - VERIFY(error_event.error().category() != DecoderErrorCategory::EndOfStream); - set_playback_status(PlaybackStatus::Corrupted); - } - - // Allow events to bubble up in all cases. - event.ignore(); -} - void PlaybackManager::resume_playback() { set_playback_status(PlaybackStatus::Playing); @@ -127,7 +114,7 @@ void PlaybackManager::on_decoder_error(DecoderError error) break; default: set_playback_status(PlaybackStatus::Corrupted); - m_main_loop.post_event(*this, make(move(error))); + m_main_loop.post_event(m_event_handler, make(move(error))); break; } } @@ -153,7 +140,7 @@ void PlaybackManager::update_presented_frame() } if (!out_of_queued_frames && frame_item_to_display.has_value()) { - m_main_loop.post_event(*this, make(frame_item_to_display->bitmap())); + m_main_loop.post_event(m_event_handler, make(frame_item_to_display->bitmap())); m_last_present_in_media_time = current_playback_time(); m_last_present_in_real_time = Time::now_monotonic(); frame_item_to_display.clear(); @@ -197,7 +184,7 @@ void PlaybackManager::restart_playback() void PlaybackManager::post_decoder_error(DecoderError error) { - m_main_loop.post_event(*this, make(error)); + m_main_loop.post_event(m_event_handler, make(error)); } bool PlaybackManager::decode_and_queue_one_sample() diff --git a/Userland/Libraries/LibVideo/PlaybackManager.h b/Userland/Libraries/LibVideo/PlaybackManager.h index 320d409268..06be0d4921 100644 --- a/Userland/Libraries/LibVideo/PlaybackManager.h +++ b/Userland/Libraries/LibVideo/PlaybackManager.h @@ -91,15 +91,12 @@ private: static constexpr size_t FRAME_BUFFER_COUNT = 4; using VideoFrameQueue = Queue; -class PlaybackManager : public Core::Object { - C_OBJECT(PlaybackManager) - +class PlaybackManager { public: - static DecoderErrorOr> from_file(Object* event_handler, StringView file); - static DecoderErrorOr> from_data(Object* event_handler, Span data); + static DecoderErrorOr> from_file(Core::Object& event_handler, StringView file); + static DecoderErrorOr> from_data(Core::Object& event_handler, Span data); - PlaybackManager(Object* event_handler, NonnullOwnPtr& demuxer, Track video_track, NonnullOwnPtr& decoder); - ~PlaybackManager() override = default; + PlaybackManager(Core::Object& event_handler, NonnullOwnPtr& demuxer, Track video_track, NonnullOwnPtr&& decoder); void resume_playback(); void pause_playback(); @@ -110,7 +107,6 @@ public: u64 number_of_skipped_frames() const { return m_skipped_frames; } - void event(Core::Event& event) override; void on_decoder_error(DecoderError error); Time current_playback_time(); @@ -129,6 +125,7 @@ private: bool decode_and_queue_one_sample(); void on_decode_timer(); + Core::Object& m_event_handler; Core::EventLoop& m_main_loop; PlaybackStatus m_status { PlaybackStatus::Stopped };