Merge pull request #1013 from OpenShot/caching-improvements

Caching Improvements (Forward & Backwards, Play & Pause)

src/CacheBase.h

@@ -93,6 +93,10 @@ namespace openshot {
 		/// @param end_frame_number The ending frame number of the cached frame
 		virtual void Remove(int64_t start_frame_number, int64_t end_frame_number) = 0;
 
+		/// @brief Move frame to front of queue (so it lasts longer)
+		/// @param frame_number The frame number of the cached frame
+		virtual void Touch(int64_t frame_number) = 0;
+
 		/// Gets the maximum bytes value
 		int64_t GetMaxBytes() { return max_bytes; };
 

src/CacheDisk.cpp

@@ -94,7 +94,7 @@ void CacheDisk::Add(std::shared_ptr<Frame> frame)
 	// Freshen frame if it already exists
 	if (frames.count(frame_number))
 		// Move frame to front of queue
-		MoveToFront(frame_number);
+		Touch(frame_number);
 
 	else
 	{
@@ -334,7 +334,7 @@ void CacheDisk::Remove(int64_t start_frame_number, int64_t end_frame_number)
 }
 
 // Move frame to front of queue (so it lasts longer)
-void CacheDisk::MoveToFront(int64_t frame_number)
+void CacheDisk::Touch(int64_t frame_number)
 {
 	// Does frame exists in cache?
 	if (frames.count(frame_number))

src/CacheDisk.h

@@ -91,7 +91,7 @@ namespace openshot {
 
 		/// @brief Move frame to front of queue (so it lasts longer)
 		/// @param frame_number The frame number of the cached frame
-		void MoveToFront(int64_t frame_number);
+		void Touch(int64_t frame_number);
 
 		/// @brief Remove a specific frame
 		/// @param frame_number The frame number of the cached frame

src/CacheMemory.cpp

@@ -52,7 +52,7 @@ void CacheMemory::Add(std::shared_ptr<Frame> frame)
 	// Freshen frame if it already exists
 	if (frames.count(frame_number))
 		// Move frame to front of queue
-		MoveToFront(frame_number);
+		Touch(frame_number);
 
 	else
 	{
@@ -192,7 +192,7 @@ void CacheMemory::Remove(int64_t start_frame_number, int64_t end_frame_number)
 }
 
 // Move frame to front of queue (so it lasts longer)
-void CacheMemory::MoveToFront(int64_t frame_number)
+void CacheMemory::Touch(int64_t frame_number)
 {
 	// Create a scoped lock, to protect the cache from multiple threads
 	const std::lock_guard<std::recursive_mutex> lock(*cacheMutex);

src/CacheMemory.h

@@ -74,7 +74,7 @@ namespace openshot {
 
 		/// @brief Move frame to front of queue (so it lasts longer)
 		/// @param frame_number The frame number of the cached frame
-		void MoveToFront(int64_t frame_number);
+		void Touch(int64_t frame_number);
 
 		/// @brief Remove a specific frame
 		/// @param frame_number The frame number of the cached frame

src/Qt/VideoCacheThread.h

@@ -1,12 +1,12 @@
 /**
  * @file
- * @brief Source file for VideoCacheThread class
+ * @brief Header file for VideoCacheThread class
  * @author Jonathan Thomas <jonathan@openshot.org>
  *
  * @ref License
  */
 
-// Copyright (c) 2008-2019 OpenShot Studios, LLC
+// Copyright (c) 2008-2025 OpenShot Studios, LLC
 //
 // SPDX-License-Identifier: LGPL-3.0-or-later
 
@@ -17,72 +17,163 @@
 
 #include <AppConfig.h>
 #include <juce_audio_basics/juce_audio_basics.h>
+#include <memory>
 
 namespace openshot
 {
     using juce::Thread;
-    using juce::WaitableEvent;
 
     /**
-     *  @brief The video cache class.
+     * @brief Handles prefetching and caching of video/audio frames for smooth playback.
+     *
+     * This thread continuously maintains a “window” of cached frames in the current playback
+     * direction (forward or backward). When paused, it continues to fill that same window;
+     * when seeking, it resets to cache around the new position.
      */
-    class VideoCacheThread : Thread
+    class VideoCacheThread : public Thread
     {
-	protected:
-	std::shared_ptr<Frame> last_cached_frame;
-	int speed;
-	int last_speed;
-	bool is_playing;
-	int64_t requested_display_frame;
-	int64_t current_display_frame;
-	int64_t cached_frame_count = 0;
-	ReaderBase *reader;
-	int64_t min_frames_ahead;
-	int64_t max_frames_ahead;
-	int64_t timeline_max_frame;
-	bool should_pause_cache;
-	bool should_break;
-
-	/// Constructor
-	VideoCacheThread();
-	/// Destructor
-	~VideoCacheThread();
-
-    /// Get the size in bytes of a frame (rough estimate)
-    int64_t getBytes(int width, int height, int sample_rate, int channels, float fps);
-
-    /// Get Speed (The speed and direction to playback a reader (1=normal, 2=fast, 3=faster, -1=rewind, etc...)
-    int getSpeed() const { return speed; }
-
-	/// Play the video
-	void Play();
-
-    /// Seek the reader to a particular frame number
-    void Seek(int64_t new_position);
-
-	/// Seek the reader to a particular frame number and optionally start the pre-roll
-	void Seek(int64_t new_position, bool start_preroll);
-
-	/// Set Speed (The speed and direction to playback a reader (1=normal, 2=fast, 3=faster, -1=rewind, etc...)
-	void setSpeed(int new_speed);
-
-	/// Stop the audio playback
-	void Stop();
-
-	/// Start the thread
-	void run();
-
-	/// Set the current thread's reader
-	void Reader(ReaderBase *new_reader) { reader=new_reader; Play(); };
-
-	/// Parent class of VideoCacheThread
-	friend class PlayerPrivate;
-	friend class QtPlayer;
-
     public:
-        /// Is cache ready for video/audio playback
+        /// Constructor: initializes member variables and assumes forward direction on first launch.
+        VideoCacheThread();
+        ~VideoCacheThread() override;
+
+        /// @return True if at least min_frames_ahead frames have been cached.
         bool isReady();
+
+        /// Set is_playing = true, so run() will begin caching/playback.
+        void Play();
+
+        /// Set is_playing = false, effectively pausing playback (caching still runs).
+        void Stop();
+
+        /**
+         * @brief Set playback speed/direction. Positive = forward, negative = rewind, zero = pause.
+         * @param new_speed
+         *
+         * If new_speed != 0, last_speed and last_dir are updated.
+         * If new_speed == 0, last_dir is left unchanged so that pausing does not flip direction.
+         */
+        void setSpeed(int new_speed);
+
+        /// @return The current speed (1=normal, 2=fast, –1=rewind, etc.)
+        int getSpeed() const { return speed; }
+
+        /// Seek to a specific frame (no preroll).
+        void Seek(int64_t new_position);
+
+        /**
+         * @brief Seek to a specific frame and optionally start a preroll (cache reset).
+         * @param new_position Frame index to jump to.
+         * @param start_preroll If true, forces cache to rebuild around new_position.
+         */
+        void Seek(int64_t new_position, bool start_preroll);
+
+        /**
+         * @brief Attach a ReaderBase (e.g. Timeline, FFmpegReader) and begin caching.
+         * @param new_reader
+         */
+        void Reader(ReaderBase* new_reader) { reader = new_reader; Play(); }
+
+    protected:
+        /// Thread entry point: loops until threadShouldExit() is true.
+        void run() override;
+
+        /**
+         * @brief Estimate memory usage for a single frame (video + audio).
+         * @param width       Frame width (pixels)
+         * @param height      Frame height (pixels)
+         * @param sample_rate Audio sample rate (e.g. 48000)
+         * @param channels    Number of audio channels
+         * @param fps         Frames per second
+         * @return Approximate size in bytes for one frame
+         */
+        int64_t getBytes(int width, int height, int sample_rate, int channels, float fps);
+
+        //---------- Helper functions, broken out for clarity & unit testing ----------
+
+        /// @return Effective playback direction (+1 forward, –1 backward), preserving last_dir if speed == 0.
+        int computeDirection() const;
+
+        /**
+         * @brief If userSeeked is true, reset last_cached_index just behind the playhead.
+         * @param playhead Current requested_display_frame
+         * @param dir      Effective direction (±1)
+         */
+        void handleUserSeek(int64_t playhead, int dir);
+
+        /**
+         * @brief When paused and playhead is outside current cache, clear all frames.
+         * @param playhead Current requested_display_frame
+         * @param paused   True if speed == 0
+         * @param cache    Pointer to CacheBase
+         * @return True if ClearAllCache() was invoked.
+         */
+        bool clearCacheIfPaused(int64_t playhead, bool paused, CacheBase* cache);
+
+        /**
+         * @brief Compute the “window” of frames to cache around playhead.
+         * @param playhead     Current requested_display_frame
+         * @param dir          Effective direction (±1)
+         * @param ahead_count  Number of frames ahead/back to cache
+         * @param timeline_end Last valid frame index
+         * @param[out] window_begin Lower bound (inclusive) of caching window
+         * @param[out] window_end   Upper bound (inclusive) of caching window
+         *
+         * If dir > 0: window = [playhead ... playhead + ahead_count]
+         * If dir < 0: window = [playhead – ahead_count ... playhead]
+         * Always clamps to [1 ... timeline_end].
+         */
+        void computeWindowBounds(int64_t playhead,
+                                 int dir,
+                                 int64_t ahead_count,
+                                 int64_t timeline_end,
+                                 int64_t& window_begin,
+                                 int64_t& window_end) const;
+
+        /**
+         * @brief Prefetch all missing frames in [window_begin ... window_end] or [window_end ... window_begin].
+         * @param cache          Pointer to CacheBase
+         * @param window_begin   Inclusive lower bound of the window
+         * @param window_end     Inclusive upper bound of the window
+         * @param dir            Effective direction (±1)
+         * @param reader         Pointer to ReaderBase to call GetFrame()
+         * @return True if the window was already full (no new frames added)
+         *
+         * Internally, this method iterates from last_cached_index + dir toward window_end (or window_begin)
+         * and calls GetFrame()/Add() for each missing frame until hitting the window boundary or an OOB.
+         * It also breaks early if threadShouldExit() or userSeeked becomes true.
+         */
+        bool prefetchWindow(CacheBase* cache,
+                            int64_t window_begin,
+                            int64_t window_end,
+                            int dir,
+                            ReaderBase* reader);
+
+        //---------- Internal state ----------
+
+        std::shared_ptr<Frame> last_cached_frame; ///< Last frame pointer added to cache.
+
+        int speed;            ///< Current playback speed (0=paused, >0 forward, <0 backward).
+        int last_speed;       ///< Last non-zero speed (for tracking).
+        int last_dir;         ///< Last direction sign (+1 forward, –1 backward).
+
+        bool is_playing;      ///< True if playback is “running” (affects thread loop, not caching).
+        bool userSeeked;      ///< True if Seek(..., true) was called (forces a cache reset).
+
+        int64_t requested_display_frame; ///< Frame index the user requested.
+        int64_t current_display_frame;   ///< Currently displayed frame (unused here, reserved).
+        int64_t cached_frame_count;      ///< Count of frames currently added to cache.
+
+        int64_t min_frames_ahead;        ///< Minimum number of frames considered “ready” (pre-roll).
+        int64_t max_frames_ahead;        ///< Maximum frames to attempt to cache (mem capped).
+        int64_t timeline_max_frame;      ///< Highest valid frame index in the timeline.
+
+        ReaderBase* reader;              ///< The source reader (e.g., Timeline, FFmpegReader).
+        bool force_directional_cache;    ///< (Reserved for future use).
+
+        int64_t last_cached_index;       ///< Index of the most recently cached frame.
     };
-}
+
+} // namespace openshot
 
 #endif // OPENSHOT_VIDEO_CACHE_THREAD_H

tests/CMakeLists.txt

@@ -42,6 +42,7 @@ set(OPENSHOT_TESTS
   Settings
   SphericalMetadata
   Timeline
+  VideoCacheThread
   # Effects
   ColorMap
   ChromaKey

tests/VideoCacheThread.cpp

@@ -0,0 +1,235 @@
+/**
+ * @file
+ * @brief Unit tests for VideoCacheThread helper methods
+ * @author Jonathan Thomas
+ *
+ * @ref License
+ */
+
+// Copyright (c) 2008-2025 OpenShot Studios, LLC
+//
+// SPDX-License-Identifier: LGPL-3.0-or-later
+
+#include <memory>
+#include "openshot_catch.h"
+
+#include "Qt/VideoCacheThread.h"
+#include "CacheMemory.h"
+#include "ReaderBase.h"
+#include "Frame.h"
+#include "Settings.h"
+#include "FFmpegReader.h"
+#include "Timeline.h"
+
+using namespace openshot;
+
+// ----------------------------------------------------------------------------
+// TestableVideoCacheThread: expose protected/internal members for testing
+//
+class TestableVideoCacheThread : public VideoCacheThread {
+public:
+    using VideoCacheThread::computeDirection;
+    using VideoCacheThread::computeWindowBounds;
+    using VideoCacheThread::clearCacheIfPaused;
+    using VideoCacheThread::prefetchWindow;
+    using VideoCacheThread::handleUserSeek;
+
+    int64_t getLastCachedIndex() const { return last_cached_index; }
+    void    setLastCachedIndex(int64_t v) { last_cached_index = v; }
+    void    setLastDir(int d) { last_dir = d; }
+    void    forceUserSeekFlag() { userSeeked = true; }
+};
+
+// ----------------------------------------------------------------------------
+// TESTS
+// ----------------------------------------------------------------------------
+
+TEST_CASE("computeDirection: respects speed and last_dir", "[VideoCacheThread]") {
+    TestableVideoCacheThread thread;
+
+    // Default: speed=0, last_dir initialized to +1
+    CHECK(thread.computeDirection() == 1);
+
+    // Positive speed
+    thread.setSpeed(3);
+    CHECK(thread.computeDirection() == 1);
+    CHECK(thread.getSpeed() == 3);
+
+    // Negative speed
+    thread.setSpeed(-2);
+    CHECK(thread.computeDirection() == -1);
+    CHECK(thread.getSpeed() == -2);
+
+    // Pause should preserve last_dir = -1
+    thread.setSpeed(0);
+    CHECK(thread.computeDirection() == -1);
+
+    // Manually override last_dir to +1, then pause
+    thread.setLastDir(1);
+    thread.setSpeed(0);
+    CHECK(thread.computeDirection() == 1);
+}
+
+TEST_CASE("computeWindowBounds: forward and backward bounds, clamped", "[VideoCacheThread]") {
+    TestableVideoCacheThread thread;
+    int64_t wb, we;
+
+    // Forward direction, normal case
+    thread.computeWindowBounds(/*playhead=*/10, /*dir=*/1, /*ahead_count=*/5, /*timeline_end=*/50, wb, we);
+    CHECK(wb == 10);
+    CHECK(we == 15);
+
+    // Forward direction, at timeline edge
+    thread.computeWindowBounds(/*playhead=*/47, /*dir=*/1, /*ahead_count=*/10, /*timeline_end=*/50, wb, we);
+    CHECK(wb == 47);
+    CHECK(we == 50);  // clamped to 50
+
+    // Backward direction, normal
+    thread.computeWindowBounds(/*playhead=*/20, /*dir=*/-1, /*ahead_count=*/7, /*timeline_end=*/100, wb, we);
+    CHECK(wb == 13);
+    CHECK(we == 20);
+
+    // Backward direction, window_begin < 1
+    thread.computeWindowBounds(/*playhead=*/3, /*dir=*/-1, /*ahead_count=*/10, /*timeline_end=*/100, wb, we);
+    CHECK(wb == 1);   // clamped
+    CHECK(we == 3);
+}
+
+TEST_CASE("clearCacheIfPaused: clears only when paused and not in cache", "[VideoCacheThread]") {
+    TestableVideoCacheThread thread;
+    CacheMemory cache(/*max_bytes=*/100000000);
+
+    // Create a Timeline so that clearCacheIfPaused can call ClearAllCache safely
+    Timeline timeline(/*width=*/1280, /*height=*/720, /*fps=*/Fraction(24,1),
+                      /*sample_rate=*/48000, /*channels=*/2, ChannelLayout::LAYOUT_STEREO);
+    timeline.SetCache(&cache);
+    thread.Reader(&timeline);
+
+    // Add a frame so Contains returns true for 5 and 10
+    cache.Add(std::make_shared<Frame>(5, 0, 0));
+    cache.Add(std::make_shared<Frame>(10, 0, 0));
+
+    // Paused, playhead not in cache → should clear all cache
+    bool didClear = thread.clearCacheIfPaused(/*playhead=*/42, /*paused=*/true, &cache);
+    CHECK(didClear);
+    CHECK(cache.Count() == 0);
+
+    // Re-add a frame for next checks
+    cache.Add(std::make_shared<Frame>(5, 0, 0));
+
+    // Paused, but playhead IS in cache → no clear
+    didClear = thread.clearCacheIfPaused(/*playhead=*/5, /*paused=*/true, &cache);
+    CHECK(!didClear);
+    CHECK(cache.Contains(5));
+
+    // Not paused → should not clear even if playhead missing
+    didClear = thread.clearCacheIfPaused(/*playhead=*/99, /*paused=*/false, &cache);
+    CHECK(!didClear);
+    CHECK(cache.Contains(5));
+}
+
+TEST_CASE("handleUserSeek: sets last_cached_index to playhead - dir", "[VideoCacheThread]") {
+    TestableVideoCacheThread thread;
+
+    thread.setLastCachedIndex(100);
+    thread.handleUserSeek(/*playhead=*/50, /*dir=*/1);
+    CHECK(thread.getLastCachedIndex() == 49);
+
+    thread.handleUserSeek(/*playhead=*/50, /*dir=*/-1);
+    CHECK(thread.getLastCachedIndex() == 51);
+}
+
+TEST_CASE("prefetchWindow: forward caching with FFmpegReader & CacheMemory", "[VideoCacheThread]") {
+    TestableVideoCacheThread thread;
+    CacheMemory cache(/*max_bytes=*/100000000);
+
+    // Use a real test file via FFmpegReader
+    std::string path = std::string(TEST_MEDIA_PATH) + "sintel_trailer-720p.mp4";
+    FFmpegReader reader(path);
+    reader.Open();
+
+    // Setup: window [1..5], dir=1, last_cached_index=0
+    thread.setLastCachedIndex(0);
+    int64_t window_begin = 1, window_end = 5;
+
+    bool wasFull = thread.prefetchWindow(&cache, window_begin, window_end, /*dir=*/1, &reader);
+    CHECK(!wasFull);
+
+    // Should have cached frames 1..5
+    CHECK(thread.getLastCachedIndex() == window_end);
+    for (int64_t f = 1; f <= 5; ++f) {
+        CHECK(cache.Contains(f));
+    }
+
+    // Now window is full; next prefetch should return true
+    wasFull = thread.prefetchWindow(&cache, window_begin, window_end, /*dir=*/1, &reader);
+    CHECK(wasFull);
+    CHECK(thread.getLastCachedIndex() == window_end);
+}
+
+TEST_CASE("prefetchWindow: backward caching with FFmpegReader & CacheMemory", "[VideoCacheThread]") {
+    TestableVideoCacheThread thread;
+    CacheMemory cache(/*max_bytes=*/100000000);
+
+    // Use a real test file via FFmpegReader
+    std::string path = std::string(TEST_MEDIA_PATH) + "sintel_trailer-720p.mp4";
+    FFmpegReader reader(path);
+    reader.Open();
+
+    // Setup: window [10..15], dir=-1, last_cached_index=16
+    thread.setLastCachedIndex(16);
+    int64_t window_begin = 10, window_end = 15;
+
+    bool wasFull = thread.prefetchWindow(&cache, window_begin, window_end, /*dir=*/-1, &reader);
+    CHECK(!wasFull);
+
+    // Should have cached frames 15..10
+    CHECK(thread.getLastCachedIndex() == window_begin);
+    for (int64_t f = 10; f <= 15; ++f) {
+        CHECK(cache.Contains(f));
+    }
+
+    // Next call should return true
+    wasFull = thread.prefetchWindow(&cache, window_begin, window_end, /*dir=*/-1, &reader);
+    CHECK(wasFull);
+    CHECK(thread.getLastCachedIndex() == window_begin);
+}
+
+TEST_CASE("prefetchWindow: interrupt on userSeeked flag", "[VideoCacheThread]") {
+    TestableVideoCacheThread thread;
+    CacheMemory cache(/*max_bytes=*/100000000);
+
+    // Use a real test file via FFmpegReader
+    std::string path = std::string(TEST_MEDIA_PATH) + "sintel_trailer-720p.mp4";
+    FFmpegReader reader(path);
+    reader.Open();
+
+    // Window [20..30], dir=1, last_cached_index=19
+    thread.setLastCachedIndex(19);
+    int64_t window_begin = 20, window_end = 30;
+
+    // Subclass CacheMemory to interrupt on frame 23
+    class InterruptingCache : public CacheMemory {
+    public:
+        TestableVideoCacheThread* tcb;
+        InterruptingCache(int64_t maxb, TestableVideoCacheThread* t)
+            : CacheMemory(maxb), tcb(t) {}
+        void Add(std::shared_ptr<openshot::Frame> frame) override {
+            int64_t idx = frame->number;  // use public member 'number'
+            CacheMemory::Add(frame);
+            if (idx == 23) {
+                tcb->forceUserSeekFlag();
+            }
+        }
+    } interruptingCache(/*max_bytes=*/100000000, &thread);
+
+    bool wasFull = thread.prefetchWindow(&interruptingCache,
+                                          window_begin,
+                                          window_end,
+                                          /*dir=*/1,
+                                          &reader);
+
+    // Should stop at 23
+    CHECK(thread.getLastCachedIndex() == 23);
+    CHECK(!wasFull);
+}