From 18a6a1dd1029f7da200680ff3f6b7d8e134ec6a3 Mon Sep 17 00:00:00 2001 From: Zaggy1024 Date: Wed, 9 Nov 2022 06:52:37 -0600 Subject: [PATCH] LibVideo: Handle corrupted video errors without spamming dialogs No longer will the video player explode with error dialogs that then lock the user out of closing them. To avoid issues where the playback state becomes invalid when an error occurs, I've made all decoder errors pass through the frame queue. This way, when a video is corrupted, there should be no chance that the playback state becomes invalid due to setting the state to Corrupted in the event handler while a presentation event is still pending. Or at least I think that was what caused some issues I was seeing :^) This system should be a lot more robust if any future errors need to be handled. --- .../VideoPlayer/VideoPlayerWidget.cpp | 2 +- .../VideoPlayer/VideoPlayerWidget.h | 2 +- .../Libraries/LibVideo/PlaybackManager.cpp | 81 ++++++++++--------- Userland/Libraries/LibVideo/PlaybackManager.h | 59 ++++++++++++-- 4 files changed, 98 insertions(+), 46 deletions(-) diff --git a/Userland/Applications/VideoPlayer/VideoPlayerWidget.cpp b/Userland/Applications/VideoPlayer/VideoPlayerWidget.cpp index 1145948777..31d5972503 100644 --- a/Userland/Applications/VideoPlayer/VideoPlayerWidget.cpp +++ b/Userland/Applications/VideoPlayer/VideoPlayerWidget.cpp @@ -127,7 +127,7 @@ void VideoPlayerWidget::toggle_pause() resume_playback(); } -void VideoPlayerWidget::on_decoding_error(Video::DecoderError error) +void VideoPlayerWidget::on_decoding_error(Video::DecoderError const& error) { StringView text_format; diff --git a/Userland/Applications/VideoPlayer/VideoPlayerWidget.h b/Userland/Applications/VideoPlayer/VideoPlayerWidget.h index 738ad06025..e52f5e920c 100644 --- a/Userland/Applications/VideoPlayer/VideoPlayerWidget.h +++ b/Userland/Applications/VideoPlayer/VideoPlayerWidget.h @@ -35,7 +35,7 @@ private: VideoPlayerWidget(GUI::Window&); void update_play_pause_icon(); - void on_decoding_error(Video::DecoderError); + void on_decoding_error(Video::DecoderError const&); void display_next_frame(); void cycle_sizing_modes(); diff --git a/Userland/Libraries/LibVideo/PlaybackManager.cpp b/Userland/Libraries/LibVideo/PlaybackManager.cpp index 2999fe3df5..244c66e6c3 100644 --- a/Userland/Libraries/LibVideo/PlaybackManager.cpp +++ b/Userland/Libraries/LibVideo/PlaybackManager.cpp @@ -14,20 +14,6 @@ namespace Video { -// We post DecoderErrors to the event queue to be handled, since some will occur off the main thread. -#define TRY_OR_POST_ERROR_AND_RETURN(expression, return_value) \ - ({ \ - auto _temporary_result = ((expression)); \ - if (_temporary_result.is_error()) { \ - dbgln("Playback error encountered: {}", _temporary_result.error().string_literal()); \ - m_main_loop.post_event(*this, make(_temporary_result.release_error())); \ - return return_value; \ - } \ - _temporary_result.release_value(); \ - }) - -#define TRY_OR_POST_ERROR(expression) TRY_OR_POST_ERROR_AND_RETURN(expression, ) - DecoderErrorOr> PlaybackManager::from_file(Object* event_handler, StringView filename) { NonnullOwnPtr demuxer = TRY(MatroskaDemuxer::from_file(filename)); @@ -69,7 +55,7 @@ void PlaybackManager::set_playback_status(PlaybackStatus status) dbgln_if(PLAYBACK_MANAGER_DEBUG, "Set playback status from {} to {}", playback_status_to_string(old_status), playback_status_to_string(m_status)); if (status == PlaybackStatus::Playing) { - if (old_status == PlaybackStatus::Stopped) { + if (old_status == PlaybackStatus::Stopped || old_status == PlaybackStatus::Corrupted) { restart_playback(); m_frame_queue->clear(); m_skipped_frames = 0; @@ -91,6 +77,7 @@ 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. @@ -114,7 +101,7 @@ bool PlaybackManager::prepare_next_frame() if (m_frame_queue->is_empty()) return false; auto frame_item = m_frame_queue->dequeue(); - m_next_frame.emplace(frame_item); + m_next_frame.emplace(move(frame_item)); m_decode_timer->start(); return true; } @@ -131,6 +118,20 @@ Time PlaybackManager::duration() return m_demuxer->duration(); } +void PlaybackManager::on_decoder_error(DecoderError error) +{ + dbgln("Playback error encountered: {}", error.string_literal()); + switch (error.category()) { + case DecoderErrorCategory::EndOfStream: + set_playback_status(PlaybackStatus::Stopped); + break; + default: + set_playback_status(PlaybackStatus::Corrupted); + m_main_loop.post_event(*this, make(move(error))); + break; + } +} + void PlaybackManager::update_presented_frame() { bool out_of_queued_frames = false; @@ -141,18 +142,18 @@ void PlaybackManager::update_presented_frame() if (out_of_queued_frames) break; VERIFY(m_next_frame.has_value()); - if (m_next_frame->timestamp > current_playback_time() || m_next_frame->is_eos_marker()) + if (m_next_frame->is_error() || m_next_frame->timestamp() > current_playback_time()) break; if (frame_item_to_display.has_value()) { - dbgln_if(PLAYBACK_MANAGER_DEBUG, "At {}ms: Dropped frame with timestamp {}ms for the next at {}ms", current_playback_time().to_milliseconds(), frame_item_to_display->timestamp.to_milliseconds(), m_next_frame->timestamp.to_milliseconds()); + dbgln_if(PLAYBACK_MANAGER_DEBUG, "At {}ms: Dropped {} in favor of {}", current_playback_time().to_milliseconds(), frame_item_to_display->debug_string(), m_next_frame->debug_string()); m_skipped_frames++; } frame_item_to_display = m_next_frame.release_value(); } 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(*this, 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(); @@ -161,20 +162,20 @@ void PlaybackManager::update_presented_frame() if (frame_item_to_display.has_value()) { VERIFY(!m_next_frame.has_value()); m_next_frame = frame_item_to_display; - dbgln_if(PLAYBACK_MANAGER_DEBUG, "Set next frame back to dequeued item at timestamp {}ms", m_next_frame->timestamp.to_milliseconds()); + dbgln_if(PLAYBACK_MANAGER_DEBUG, "Set next frame back to dequeued item {}", m_next_frame->debug_string()); } if (!is_playing()) return; if (!out_of_queued_frames) { - if (m_next_frame->is_eos_marker()) { - set_playback_status(PlaybackStatus::Stopped); + if (m_next_frame->is_error()) { + on_decoder_error(m_next_frame->release_error()); m_next_frame.clear(); return; } - auto frame_time_ms = (m_next_frame.value().timestamp - current_playback_time()).to_milliseconds(); + auto frame_time_ms = (m_next_frame->timestamp() - current_playback_time()).to_milliseconds(); VERIFY(frame_time_ms <= NumericLimits::max()); dbgln_if(PLAYBACK_MANAGER_DEBUG, "Time until next frame is {}ms", frame_time_ms); m_present_timer->start(max(static_cast(frame_time_ms), 0)); @@ -189,7 +190,9 @@ void PlaybackManager::restart_playback() { m_last_present_in_media_time = Time::zero(); m_last_present_in_real_time = Time::zero(); - TRY_OR_POST_ERROR(m_demuxer->seek_to_most_recent_keyframe(m_selected_video_track, 0)); + auto result = m_demuxer->seek_to_most_recent_keyframe(m_selected_video_track, 0); + if (result.is_error()) + on_decoder_error(result.release_error()); } bool PlaybackManager::decode_and_queue_one_sample() @@ -200,26 +203,28 @@ bool PlaybackManager::decode_and_queue_one_sample() auto start_time = Time::now_monotonic(); #endif - auto frame_sample_result = m_demuxer->get_next_video_sample_for_track(m_selected_video_track); - if (frame_sample_result.is_error()) { - if (frame_sample_result.error().category() == DecoderErrorCategory::EndOfStream) { - m_frame_queue->enqueue(FrameQueueItem::eos_marker()); - return false; - } - m_main_loop.post_event(*this, make(frame_sample_result.release_error())); - return false; - } - auto frame_sample = frame_sample_result.release_value(); +#define TRY_OR_ENQUEUE_ERROR(expression) \ + ({ \ + auto _temporary_result = ((expression)); \ + if (_temporary_result.is_error()) { \ + dbgln("Enqueued decoder error: {}", _temporary_result.error().string_literal()); \ + m_frame_queue->enqueue(FrameQueueItem::error_marker(_temporary_result.release_error())); \ + return false; \ + } \ + _temporary_result.release_value(); \ + }) - TRY_OR_POST_ERROR_AND_RETURN(m_decoder->receive_sample(frame_sample->data()), false); - auto decoded_frame = TRY_OR_POST_ERROR_AND_RETURN(m_decoder->get_decoded_frame(), false); + auto frame_sample = TRY_OR_ENQUEUE_ERROR(m_demuxer->get_next_video_sample_for_track(m_selected_video_track)); + + TRY_OR_ENQUEUE_ERROR(m_decoder->receive_sample(frame_sample->data())); + auto decoded_frame = TRY_OR_ENQUEUE_ERROR(m_decoder->get_decoded_frame()); auto& cicp = decoded_frame->cicp(); cicp.adopt_specified_values(frame_sample->container_cicp()); cicp.default_code_points_if_unspecified({ Video::ColorPrimaries::BT709, Video::TransferCharacteristics::BT709, Video::MatrixCoefficients::BT709, Video::ColorRange::Studio }); - auto bitmap = TRY_OR_POST_ERROR_AND_RETURN(decoded_frame->to_bitmap(), false); - m_frame_queue->enqueue(FrameQueueItem { bitmap, frame_sample->timestamp() }); + auto bitmap = TRY_OR_ENQUEUE_ERROR(decoded_frame->to_bitmap()); + m_frame_queue->enqueue(FrameQueueItem::frame(bitmap, frame_sample->timestamp())); #if PLAYBACK_MANAGER_DEBUG auto end_time = Time::now_monotonic(); diff --git a/Userland/Libraries/LibVideo/PlaybackManager.h b/Userland/Libraries/LibVideo/PlaybackManager.h index 77a91fc17a..1ac3256336 100644 --- a/Userland/Libraries/LibVideo/PlaybackManager.h +++ b/Userland/Libraries/LibVideo/PlaybackManager.h @@ -30,18 +30,62 @@ enum class PlaybackStatus { Buffering, Seeking, Stopped, + Corrupted, }; struct FrameQueueItem { - static FrameQueueItem eos_marker() + enum class Type { + Frame, + Error, + }; + + static FrameQueueItem frame(RefPtr bitmap, Time timestamp) { - return { nullptr, Time::max() }; + return FrameQueueItem(move(bitmap), timestamp); } - RefPtr bitmap; - Time timestamp; + static FrameQueueItem error_marker(DecoderError&& error) + { + return FrameQueueItem(move(error)); + } - bool is_eos_marker() const { return !bitmap; } + bool is_frame() const { return m_data.has(); } + RefPtr bitmap() const { return m_data.get().bitmap; } + Time timestamp() const { return m_data.get().timestamp; } + + bool is_error() const { return m_data.has(); } + DecoderError const& error() const { return m_data.get(); } + DecoderError release_error() + { + auto error = move(m_data.get()); + m_data.set(Empty()); + return error; + } + + String debug_string() const + { + if (is_error()) + return error().string_literal(); + return String::formatted("frame at {}ms", timestamp().to_milliseconds()); + } + +private: + struct FrameData { + RefPtr bitmap; + Time timestamp; + }; + + FrameQueueItem(RefPtr bitmap, Time timestamp) + : m_data(FrameData { move(bitmap), timestamp }) + { + } + + FrameQueueItem(DecoderError&& error) + : m_data(move(error)) + { + } + + Variant m_data; }; static constexpr size_t FRAME_BUFFER_COUNT = 4; @@ -67,6 +111,7 @@ 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(); Time duration(); @@ -119,7 +164,7 @@ public: } virtual ~DecoderErrorEvent() = default; - DecoderError error() { return m_error; } + DecoderError const& error() { return m_error; } private: DecoderError m_error; @@ -173,6 +218,8 @@ inline StringView playback_status_to_string(PlaybackStatus status) return "Seeking"sv; case PlaybackStatus::Stopped: return "Stopped"sv; + case PlaybackStatus::Corrupted: + return "Corrupted"sv; } return "Unknown"sv; };