From 2a228e8a6eec2646c55d1c1dbfbf59459e16508d Mon Sep 17 00:00:00 2001
From: Zaggy1024 <zaggy1024@gmail.com>
Date: Sun, 12 Feb 2023 00:44:16 -0600
Subject: [PATCH] LibVideo: Deduplicate logic for dispatching video frame queue
 items

Previously we had dispatch_decoder_error and on_decoder_error serving
the same function, with one not handling the end of stream properly.

There is also a new function to dispatch either an error or a frame to
the owner of this playback manager, so that PlaybackStateHandlers don't
have to duplicate this logic.
---
 .../Libraries/LibVideo/PlaybackManager.cpp    | 44 +++++++++++--------
 Userland/Libraries/LibVideo/PlaybackManager.h |  4 +-
 2 files changed, 28 insertions(+), 20 deletions(-)

diff --git a/Userland/Libraries/LibVideo/PlaybackManager.cpp b/Userland/Libraries/LibVideo/PlaybackManager.cpp
index 0024fd4a0f..522a6d2bd2 100644
--- a/Userland/Libraries/LibVideo/PlaybackManager.cpp
+++ b/Userland/Libraries/LibVideo/PlaybackManager.cpp
@@ -75,7 +75,7 @@ Time PlaybackManager::duration()
 {
     auto duration_result = m_demuxer->duration();
     if (duration_result.is_error())
-        on_decoder_error(duration_result.release_error());
+        dispatch_decoder_error(duration_result.release_error());
     return duration_result.release_value();
 }
 
@@ -89,7 +89,7 @@ void PlaybackManager::dispatch_fatal_error(Error error)
     m_event_handler.dispatch_event(event);
 }
 
-void PlaybackManager::on_decoder_error(DecoderError error)
+void PlaybackManager::dispatch_decoder_error(DecoderError error)
 {
     switch (error.category()) {
     case DecoderErrorCategory::EndOfStream:
@@ -104,6 +104,23 @@ void PlaybackManager::on_decoder_error(DecoderError error)
     }
 }
 
+void PlaybackManager::dispatch_new_frame(RefPtr<Gfx::Bitmap> frame)
+{
+    m_main_loop.post_event(m_event_handler, make<VideoFramePresentEvent>(frame));
+}
+
+bool PlaybackManager::dispatch_frame_queue_item(FrameQueueItem&& item)
+{
+    if (item.is_error()) {
+        dispatch_decoder_error(item.release_error());
+        return true;
+    }
+
+    dbgln_if(PLAYBACK_MANAGER_DEBUG, "Sent frame for presentation");
+    dispatch_new_frame(item.bitmap());
+    return false;
+}
+
 void PlaybackManager::timer_callback()
 {
     TRY_OR_FATAL_ERROR(m_playback_handler->on_timer_callback());
@@ -120,7 +137,7 @@ Optional<Time> PlaybackManager::seek_demuxer_to_most_recent_keyframe(Time timest
     //        mutex so that seeking can't happen while that thread is getting a sample.
     auto result = m_demuxer->seek_to_most_recent_keyframe(m_selected_video_track, timestamp, move(earliest_available_sample));
     if (result.is_error())
-        on_decoder_error(result.release_error());
+        dispatch_decoder_error(result.release_error());
     return result.release_value();
 }
 
@@ -129,16 +146,6 @@ void PlaybackManager::restart_playback()
     seek_to_timestamp(Time::zero());
 }
 
-void PlaybackManager::dispatch_decoder_error(DecoderError error)
-{
-    m_main_loop.post_event(m_event_handler, make<DecoderErrorEvent>(error));
-}
-
-void PlaybackManager::dispatch_new_frame(RefPtr<Gfx::Bitmap> frame)
-{
-    m_main_loop.post_event(m_event_handler, make<VideoFramePresentEvent>(frame));
-}
-
 void PlaybackManager::start_timer(int milliseconds)
 {
     m_present_timer->start(milliseconds);
@@ -341,7 +348,7 @@ private:
 #endif
             if (future_frame_item.has_value()) {
                 if (future_frame_item->is_error()) {
-                    manager().on_decoder_error(future_frame_item.release_value().release_error());
+                    manager().dispatch_decoder_error(future_frame_item.release_value().release_error());
                     return {};
                 }
                 manager().m_next_frame.emplace(future_frame_item.release_value());
@@ -355,15 +362,16 @@ private:
             auto now = Time::now_monotonic();
             manager().m_last_present_in_media_time += now - m_last_present_in_real_time;
             m_last_present_in_real_time = now;
-            manager().dispatch_new_frame(manager().m_next_frame.value().bitmap());
-            dbgln_if(PLAYBACK_MANAGER_DEBUG, "Sent frame for presentation");
+
+            if (manager().dispatch_frame_queue_item(manager().m_next_frame.release_value()))
+                return {};
         }
 
         // 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()) {
-            manager().on_decoder_error(future_frame_item.release_value().release_error());
+            manager().dispatch_decoder_error(future_frame_item.release_value().release_error());
             return {};
         }
 
@@ -498,7 +506,7 @@ private:
 
             if (item.is_error()) {
                 dbgln_if(PLAYBACK_MANAGER_DEBUG, "Encountered error while seeking: {}", item.error().description());
-                manager().on_decoder_error(item.release_error());
+                manager().dispatch_decoder_error(item.release_error());
                 return {};
             }
 
diff --git a/Userland/Libraries/LibVideo/PlaybackManager.h b/Userland/Libraries/LibVideo/PlaybackManager.h
index 99909f7c12..e91403fb3b 100644
--- a/Userland/Libraries/LibVideo/PlaybackManager.h
+++ b/Userland/Libraries/LibVideo/PlaybackManager.h
@@ -106,8 +106,6 @@ public:
 
     u64 number_of_skipped_frames() const { return m_skipped_frames; }
 
-    void on_decoder_error(DecoderError error);
-
     Time current_playback_time();
     Time duration();
 
@@ -130,6 +128,8 @@ private:
 
     void dispatch_decoder_error(DecoderError error);
     void dispatch_new_frame(RefPtr<Gfx::Bitmap> frame);
+    // Returns whether we changed playback states. If so, any PlaybackStateHandler processing must cease.
+    [[nodiscard]] bool dispatch_frame_queue_item(FrameQueueItem&&);
     void dispatch_fatal_error(Error);
 
     Core::Object& m_event_handler;