From 9a9fe084493203aa031209cbd5a8723a7ea198d9 Mon Sep 17 00:00:00 2001 From: Zaggy1024 Date: Mon, 14 Nov 2022 00:48:31 -0600 Subject: [PATCH] LibVideo: Rewrite the video frame present function to be more readable The PlaybackManager::update_presented_frame function was getting out of hand and adding seeking was making it illegible. This rewrites it to be (hopefully) quite a bit more readable, and adds a few comments to help future readers of the code. In addition, some helpful debugging prints were added that should help debug any future issues with the player. --- .../VideoPlayer/VideoPlayerWidget.cpp | 4 +- .../Libraries/LibVideo/PlaybackManager.cpp | 140 ++++++++++-------- Userland/Libraries/LibVideo/PlaybackManager.h | 4 +- 3 files changed, 81 insertions(+), 67 deletions(-) diff --git a/Userland/Applications/VideoPlayer/VideoPlayerWidget.cpp b/Userland/Applications/VideoPlayer/VideoPlayerWidget.cpp index bafc0f523a..11350c7e38 100644 --- a/Userland/Applications/VideoPlayer/VideoPlayerWidget.cpp +++ b/Userland/Applications/VideoPlayer/VideoPlayerWidget.cpp @@ -114,7 +114,7 @@ void VideoPlayerWidget::update_play_pause_icon() m_play_pause_action->set_enabled(true); - if (m_playback_manager->is_playing() || m_playback_manager->is_buffering()) + if (m_playback_manager->is_playing()) m_play_pause_action->set_icon(m_pause_icon); else m_play_pause_action->set_icon(m_play_icon); @@ -140,7 +140,7 @@ void VideoPlayerWidget::toggle_pause() { if (!m_playback_manager) return; - if (m_playback_manager->is_playing() || m_playback_manager->is_buffering()) + if (m_playback_manager->is_playing()) pause_playback(); else resume_playback(); diff --git a/Userland/Libraries/LibVideo/PlaybackManager.cpp b/Userland/Libraries/LibVideo/PlaybackManager.cpp index 7f95371268..10bc3af8f5 100644 --- a/Userland/Libraries/LibVideo/PlaybackManager.cpp +++ b/Userland/Libraries/LibVideo/PlaybackManager.cpp @@ -48,14 +48,14 @@ PlaybackManager::PlaybackManager(Core::Object& event_handler, NonnullOwnPtrstart(0); } else { @@ -78,23 +78,9 @@ void PlaybackManager::pause_playback() set_playback_status(PlaybackStatus::Paused); } -bool PlaybackManager::prepare_next_frame() -{ - if (m_next_frame.has_value()) - return true; - if (m_frame_queue->is_empty()) - return false; - auto frame_item = m_frame_queue->dequeue(); - m_next_frame.emplace(move(frame_item)); - m_decode_timer->start(0); - return true; -} - Time PlaybackManager::current_playback_time() { - if (m_last_present_in_media_time.is_negative()) - return Time::zero(); - if (is_playing()) + if (m_status == PlaybackStatus::Playing) return m_last_present_in_media_time + (Time::now_monotonic() - m_last_present_in_real_time); return m_last_present_in_media_time; } @@ -124,67 +110,91 @@ void PlaybackManager::on_decoder_error(DecoderError error) void PlaybackManager::update_presented_frame() { - bool out_of_queued_frames = false; - Optional frame_item_to_display; + Optional future_frame_item; + bool should_present_frame = false; - while (true) { - out_of_queued_frames = out_of_queued_frames || !prepare_next_frame(); - if (out_of_queued_frames) - break; - VERIFY(m_next_frame.has_value()); - if (m_next_frame->is_error() || m_next_frame->timestamp() > current_playback_time()) - break; + // Skip frames until we find a frame past the current playback time, and keep the one that precedes it to display. + while (m_status == PlaybackStatus::Playing && !m_frame_queue->is_empty()) { + future_frame_item.emplace(m_frame_queue->dequeue()); + m_decode_timer->start(0); - if (frame_item_to_display.has_value()) { - 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()); + if (future_frame_item->is_error() || future_frame_item->timestamp() >= current_playback_time()) { + dbgln_if(PLAYBACK_MANAGER_DEBUG, "Should present frame, future {} is after {}ms", future_frame_item->debug_string(), current_playback_time().to_milliseconds()); + should_present_frame = true; + break; + } + + if (m_next_frame.has_value()) { + dbgln_if(PLAYBACK_MANAGER_DEBUG, "At {}ms: Dropped {} in favor of {}", current_playback_time().to_milliseconds(), m_next_frame->debug_string(), future_frame_item->debug_string()); m_skipped_frames++; } - frame_item_to_display = m_next_frame.release_value(); + m_next_frame.emplace(future_frame_item.release_value()); } - if (!out_of_queued_frames && frame_item_to_display.has_value()) { - m_main_loop.post_event(m_event_handler, make(frame_item_to_display->bitmap())); + // If we don't have both of these items, we can't present, since we need to set a timer for + // the next frame. Check if we need to buffer based on the current state. + if (!m_next_frame.has_value() || !future_frame_item.has_value()) { +#if PLAYBACK_MANAGER_DEBUG + StringBuilder debug_string_builder; + debug_string_builder.append("We don't have "sv); + if (!m_next_frame.has_value()) { + debug_string_builder.append("a frame to present"sv); + if (!future_frame_item.has_value()) + debug_string_builder.append(" or a future frame"sv); + } else { + debug_string_builder.append("a future frame"sv); + } + debug_string_builder.append(", checking for error and buffering"sv); + dbgln_if(PLAYBACK_MANAGER_DEBUG, debug_string_builder.to_string()); +#endif + if (future_frame_item.has_value()) { + if (future_frame_item->is_error()) { + on_decoder_error(future_frame_item.release_value().release_error()); + return; + } + m_next_frame.emplace(future_frame_item.release_value()); + } + if (m_status == PlaybackStatus::Playing) + set_playback_status(PlaybackStatus::Buffering); + m_decode_timer->start(0); + return; + } + + // If we have a frame, send it for presentation. + if (should_present_frame) { m_last_present_in_media_time = current_playback_time(); m_last_present_in_real_time = Time::now_monotonic(); - frame_item_to_display.clear(); + m_main_loop.post_event(m_event_handler, make(m_next_frame.value().bitmap())); + dbgln_if(PLAYBACK_MANAGER_DEBUG, "Sent frame for presentation"); } - 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 {}", m_next_frame->debug_string()); - } - - if (!is_playing()) - return; - - if (!out_of_queued_frames) { - if (m_next_frame->is_error()) { - on_decoder_error(m_next_frame->release_error()); - m_next_frame.clear(); - return; - } - - if (m_last_present_in_media_time.is_negative()) { - m_last_present_in_media_time = m_next_frame->timestamp(); - dbgln_if(PLAYBACK_MANAGER_DEBUG, "We've seeked, set last media time to the next frame {}ms", m_last_present_in_media_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)); + // Now that we've presented the current frame, we can throw whatever error is next in queue. + // This way, we always display a frame before the stream ends, and should also show any frames + // we already had when a real error occurs. + if (future_frame_item->is_error()) { + on_decoder_error(future_frame_item.release_value().release_error()); return; } - set_playback_status(PlaybackStatus::Buffering); - m_decode_timer->start(0); + // The future frame item becomes the next one to present. + m_next_frame.emplace(future_frame_item.release_value()); + + if (m_status != PlaybackStatus::Playing) { + dbgln_if(PLAYBACK_MANAGER_DEBUG, "We're not playing! Starting the decode timer"); + m_decode_timer->start(0); + return; + } + + 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)); } void PlaybackManager::seek_to_timestamp(Time timestamp) { dbgln_if(PLAYBACK_MANAGER_DEBUG, "Seeking to {}ms", timestamp.to_milliseconds()); - m_last_present_in_media_time = Time::min(); + m_last_present_in_media_time = Time::zero(); m_last_present_in_real_time = Time::zero(); m_frame_queue->clear(); m_next_frame.clear(); @@ -208,8 +218,10 @@ void PlaybackManager::post_decoder_error(DecoderError error) bool PlaybackManager::decode_and_queue_one_sample() { - if (m_frame_queue->size() >= FRAME_BUFFER_COUNT) + if (m_frame_queue->size() >= FRAME_BUFFER_COUNT) { + dbgln_if(PLAYBACK_MANAGER_DEBUG, "Frame queue is full, stopping"); return false; + } #if PLAYBACK_MANAGER_DEBUG auto start_time = Time::now_monotonic(); #endif @@ -220,6 +232,7 @@ bool PlaybackManager::decode_and_queue_one_sample() if (_temporary_result.is_error()) { \ dbgln_if(PLAYBACK_MANAGER_DEBUG, "Enqueued decoder error: {}", _temporary_result.error().string_literal()); \ m_frame_queue->enqueue(FrameQueueItem::error_marker(_temporary_result.release_error())); \ + m_present_timer->start(0); \ return false; \ } \ _temporary_result.release_value(); \ @@ -267,10 +280,11 @@ bool PlaybackManager::decode_and_queue_one_sample() auto bitmap = TRY_OR_ENQUEUE_ERROR(decoded_frame->to_bitmap()); m_frame_queue->enqueue(FrameQueueItem::frame(bitmap, frame_sample->timestamp())); + m_present_timer->start(0); #if PLAYBACK_MANAGER_DEBUG auto end_time = Time::now_monotonic(); - dbgln("Decoding took {}ms", (end_time - start_time).to_milliseconds()); + dbgln("Decoding took {}ms, queue is {} items", (end_time - start_time).to_milliseconds(), m_frame_queue->size()); #endif return true; diff --git a/Userland/Libraries/LibVideo/PlaybackManager.h b/Userland/Libraries/LibVideo/PlaybackManager.h index d8686a7968..b5b400560f 100644 --- a/Userland/Libraries/LibVideo/PlaybackManager.h +++ b/Userland/Libraries/LibVideo/PlaybackManager.h @@ -102,9 +102,9 @@ public: void pause_playback(); void restart_playback(); void seek_to_timestamp(Time); - bool is_playing() const { return m_status == PlaybackStatus::Playing; } + bool is_playing() const { return m_status == PlaybackStatus::Playing || m_status == PlaybackStatus::Buffering; } bool is_buffering() const { return m_status == PlaybackStatus::Buffering; } - bool is_stopped() const { return m_status == PlaybackStatus::Stopped; } + bool is_stopped() const { return m_status == PlaybackStatus::Stopped || m_status == PlaybackStatus::Corrupted; } u64 number_of_skipped_frames() const { return m_skipped_frames; }