From ff22d416da189837cee31b11fd061212f7618a5e Mon Sep 17 00:00:00 2001 From: Jonathan Thomas Date: Thu, 26 Feb 2026 13:22:06 -0600 Subject: [PATCH] - Playback updates no longer act like seeks. - Scrubbing inside cached area keeps cache. - Scrubbing outside cached area clears cache immediately. - Preroll now starts on seek commit (release), not during drag. - Temporary debug logs were removed. - Tests were added for these behaviors. --- src/Qt/PlayerPrivate.cpp | 33 +++++++-- src/Qt/VideoCacheThread.cpp | 141 ++++++++++++++++++++++++++++++++---- src/Qt/VideoCacheThread.h | 10 ++- src/QtPlayer.cpp | 7 +- src/QtPlayer.h | 3 + tests/VideoCacheThread.cpp | 85 ++++++++++++++++++++++ 6 files changed, 254 insertions(+), 25 deletions(-) diff --git a/src/Qt/PlayerPrivate.cpp b/src/Qt/PlayerPrivate.cpp index 222bfd5e..da672bb5 100644 --- a/src/Qt/PlayerPrivate.cpp +++ b/src/Qt/PlayerPrivate.cpp @@ -74,9 +74,13 @@ namespace openshot // Pausing Code (which re-syncs audio/video times) // - If speed is zero or speed changes // - If pre-roll is not ready (This should allow scrubbing of the timeline without waiting on pre-roll) - if ((speed == 0 && video_position == last_video_position) || - (speed != 0 && last_speed != speed) || - (speed != 0 && !is_dirty && !videoCache->isReady())) + bool wait_paused_hold = (speed == 0 && video_position == last_video_position); + bool wait_speed_change = (speed != 0 && last_speed != speed); + bool cache_ready = videoCache->isReady(); + bool wait_preroll = (speed != 0 && !is_dirty && !cache_ready); + bool should_wait = (wait_paused_hold || wait_speed_change || wait_preroll); + + if (should_wait) { // Sleep for a fraction of frame duration std::this_thread::sleep_for(frame_duration / 4); @@ -97,7 +101,16 @@ namespace openshot // Set the video frame on the video thread and render frame videoPlayback->frame = frame; + videoPlayback->rendered.reset(); videoPlayback->render.signal(); + // Keep decode/position advancement aligned with actual preview updates. + // This avoids occasional "silent advance then jump" behavior when + // preroll transitions and rendering are briefly out-of-sync. + const int render_wait_ms = std::max( + 1, + static_cast(frame_duration.count() / 1000.0 * 2.0) + ); + videoPlayback->rendered.wait(render_wait_ms); // Keep track of the last displayed frame last_video_position = video_position; @@ -116,6 +129,12 @@ namespace openshot // Protect against invalid or too-long sleep times std::this_thread::sleep_for(max_sleep); } + } else { + // If we're behind schedule (e.g. preroll/render stall), do not + // burst through delayed frames. Resync timing baseline so + // playback continues smoothly at normal cadence. + start_time = std::chrono::time_point_cast(current_time); + playback_frames = 0; } } } @@ -150,8 +169,8 @@ namespace openshot // Increment playback frames (always in the positive direction) playback_frames += std::abs(speed); - // Update cache on which frame was retrieved - videoCache->Seek(video_position); + // Update playhead hint for cache window tracking without triggering seek behavior. + videoCache->NotifyPlaybackPosition(video_position); // return frame from reader return reader->GetFrame(video_position); @@ -170,7 +189,9 @@ namespace openshot { video_position = new_position; last_video_position = 0; - is_dirty = true; + // If actively playing, require cache readiness before advancing after seek. + // If paused, keep dirty so the requested frame is rendered immediately. + is_dirty = (speed == 0); } // Start video/audio playback diff --git a/src/Qt/VideoCacheThread.cpp b/src/Qt/VideoCacheThread.cpp index 6513e692..ab0ef95e 100644 --- a/src/Qt/VideoCacheThread.cpp +++ b/src/Qt/VideoCacheThread.cpp @@ -30,6 +30,8 @@ namespace openshot , last_dir(1) // assume forward (+1) on first launch , userSeeked(false) , preroll_on_next_fill(false) + , clear_cache_on_next_fill(false) + , scrub_active(false) , requested_display_frame(1) , current_display_frame(1) , cached_frame_count(0) @@ -131,40 +133,90 @@ namespace openshot bool should_mark_seek = false; bool should_preroll = false; int64_t new_cached_count = cached_frame_count.load(); + bool entering_scrub = false; + bool leaving_scrub = false; + bool cache_contains = false; + bool should_clear_cache = false; + CacheBase* cache = reader ? reader->GetCache() : nullptr; + if (cache) { + cache_contains = cache->Contains(new_position); + } if (start_preroll) { should_mark_seek = true; - CacheBase* cache = reader ? reader->GetCache() : nullptr; - if (cache && !cache->Contains(new_position)) - { - // If user initiated seek, and current frame not found ( - if (Timeline* timeline = dynamic_cast(reader)) { - timeline->ClearAllCache(); - } + if (cache && !cache_contains) { + // Uncached commit seek: avoid blocking this call path with a + // synchronous ClearAllCache(). The cache thread will reconcile + // window contents on the next iteration around the new playhead. new_cached_count = 0; should_preroll = true; + should_clear_cache = true; } else if (cache) { new_cached_count = cache->Count(); } + leaving_scrub = true; + } + else { + // Scrub preview: keep preroll disabled and interrupt current fill. + // Do not synchronously clear cache here, as that can block seeks. + should_mark_seek = true; + if (cache && !cache_contains) { + new_cached_count = 0; + should_clear_cache = true; + } + else if (cache) { + new_cached_count = cache->Count(); + } + entering_scrub = true; } { std::lock_guard guard(seek_state_mutex); + // Invalidate ready-state baseline immediately on seek so isReady() + // cannot pass/fail against stale cached_index from a prior window. + const int dir = computeDirection(); + last_cached_index.store(new_position - dir); requested_display_frame.store(new_position); cached_frame_count.store(new_cached_count); - if (start_preroll) { - preroll_on_next_fill.store(should_preroll); - userSeeked.store(should_mark_seek); + preroll_on_next_fill.store(should_preroll); + // Clear behavior follows the latest seek intent. + clear_cache_on_next_fill.store(should_clear_cache); + userSeeked.store(should_mark_seek); + if (entering_scrub) { + scrub_active.store(true); + } + if (leaving_scrub) { + scrub_active.store(false); } } } void VideoCacheThread::Seek(int64_t new_position) { - Seek(new_position, false); + NotifyPlaybackPosition(new_position); + } + + void VideoCacheThread::NotifyPlaybackPosition(int64_t new_position) + { + if (new_position <= 0) { + return; + } + if (scrub_active.load()) { + return; + } + + int64_t new_cached_count = cached_frame_count.load(); + if (CacheBase* cache = reader ? reader->GetCache() : nullptr) { + new_cached_count = cache->Count(); + } + { + std::lock_guard guard(seek_state_mutex); + requested_display_frame.store(new_position); + cached_frame_count.store(new_cached_count); + } } int VideoCacheThread::computeDirection() const @@ -257,10 +309,12 @@ namespace openshot int64_t window_begin, int64_t window_end, int dir, - ReaderBase* reader) + ReaderBase* reader, + int64_t max_frames_to_fetch) { bool window_full = true; int64_t next_frame = last_cached_index.load() + dir; + int64_t fetched_this_pass = 0; // Advance from last_cached_index toward window boundary while ((dir > 0 && next_frame <= window_end) || @@ -280,6 +334,7 @@ namespace openshot auto framePtr = reader->GetFrame(next_frame); cache->Add(framePtr); cached_frame_count.store(cache->Count()); + ++fetched_this_pass; } catch (const OutOfBoundsFrame&) { break; @@ -292,6 +347,12 @@ namespace openshot last_cached_index.store(next_frame); next_frame += dir; + + // In active playback, avoid long uninterrupted prefetch bursts + // that can delay player thread frame retrieval. + if (max_frames_to_fetch > 0 && fetched_this_pass >= max_frames_to_fetch) { + break; + } } return window_full; @@ -305,6 +366,21 @@ namespace openshot while (!threadShouldExit()) { Settings* settings = Settings::Instance(); CacheBase* cache = reader ? reader->GetCache() : nullptr; + Timeline* timeline = dynamic_cast(reader); + + // Process deferred clears even when caching is currently disabled + // (e.g. active scrub mode), so stale ranges are removed promptly. + bool should_clear_cache = clear_cache_on_next_fill.exchange(false); + if (should_clear_cache && timeline) { + const int dir_on_clear = computeDirection(); + const int64_t clear_playhead = requested_display_frame.load(); + timeline->ClearAllCache(); + cached_frame_count.store(0); + // Reset ready baseline immediately after clear. Otherwise a + // stale last_cached_index from the old cache window can make + // isReady() report true before new preroll is actually filled. + last_cached_index.store(clear_playhead - dir_on_clear); + } // If caching disabled or no reader, mark cache as ready and sleep briefly if (!settings->ENABLE_PLAYBACK_CACHING || !cache) { @@ -317,7 +393,6 @@ namespace openshot // init local vars min_frames_ahead.store(settings->VIDEO_CACHE_MIN_PREROLL_FRAMES); - Timeline* timeline = dynamic_cast(reader); if (!timeline) { std::this_thread::sleep_for(double_micro_sec(50000)); continue; @@ -366,7 +441,10 @@ namespace openshot } } if (did_user_seek) { - if (use_preroll) { + // During active playback, prioritize immediate forward readiness + // from the playhead. Use directional preroll offset only while + // paused/scrubbing contexts. + if (use_preroll && paused) { handleUserSeekWithPreroll(playhead, dir, timeline_end, preroll_frames); } else { @@ -395,6 +473,22 @@ namespace openshot } } + // If a clear was requested by a seek that arrived after the loop + // began, apply it now before any additional prefetch work. This + // avoids "build then suddenly clear" behavior during playback. + bool should_clear_mid_loop = clear_cache_on_next_fill.exchange(false); + if (should_clear_mid_loop && timeline) { + timeline->ClearAllCache(); + cached_frame_count.store(0); + last_cached_index.store(playhead - dir); + } + + // While user is dragging/scrubbing, skip cache prefetch work. + if (scrub_active.load()) { + std::this_thread::sleep_for(double_micro_sec(10000)); + continue; + } + // If capacity is insufficient, sleep and retry if (capacity < 1) { std::this_thread::sleep_for(double_micro_sec(50000)); @@ -411,7 +505,8 @@ namespace openshot ready_target = 0; } int64_t configured_min = settings->VIDEO_CACHE_MIN_PREROLL_FRAMES; - min_frames_ahead.store(std::min(configured_min, ready_target)); + const int64_t required_ahead = std::min(configured_min, ready_target); + min_frames_ahead.store(required_ahead); // If paused and playhead is no longer in cache, clear everything bool did_clear = clearCacheIfPaused(playhead, paused, cache); @@ -429,7 +524,21 @@ namespace openshot window_end); // Attempt to fill any missing frames in that window - bool window_full = prefetchWindow(cache, window_begin, window_end, dir, reader); + int64_t max_frames_to_fetch = -1; + if (!paused) { + // Keep cache thread responsive during playback seeks so player + // can start as soon as pre-roll is met instead of waiting for a + // full cache window pass. + max_frames_to_fetch = 8; + } + bool window_full = prefetchWindow( + cache, + window_begin, + window_end, + dir, + reader, + max_frames_to_fetch + ); // If paused and window was already full, keep playhead fresh if (paused && window_full) { diff --git a/src/Qt/VideoCacheThread.h b/src/Qt/VideoCacheThread.h index a9dc2dd6..58b7308f 100644 --- a/src/Qt/VideoCacheThread.h +++ b/src/Qt/VideoCacheThread.h @@ -61,7 +61,7 @@ namespace openshot /// @return The current speed (1=normal, 2=fast, –1=rewind, etc.) int getSpeed() const { return speed.load(); } - /// Seek to a specific frame (no preroll). + /// Backward-compatible alias for playback position updates (no seek side effects). void Seek(int64_t new_position); /** @@ -71,6 +71,9 @@ namespace openshot */ void Seek(int64_t new_position, bool start_preroll); + /// Update playback position without triggering seek behavior or cache invalidation. + void NotifyPlaybackPosition(int64_t new_position); + /// Start the cache thread at high priority. Returns true if it’s actually running. bool StartThread(); @@ -171,7 +174,8 @@ namespace openshot int64_t window_begin, int64_t window_end, int dir, - ReaderBase* reader); + ReaderBase* reader, + int64_t max_frames_to_fetch = -1); //---------- Internal state ---------- @@ -182,6 +186,8 @@ namespace openshot std::atomic last_dir; ///< Last direction sign (+1 forward, –1 backward). std::atomic userSeeked; ///< True if Seek(..., true) was called (forces a cache reset). std::atomic preroll_on_next_fill; ///< True if next cache rebuild should include preroll offset. + std::atomic clear_cache_on_next_fill; ///< True if next cache loop should clear existing cache ranges. + std::atomic scrub_active; ///< True while user is dragging/scrubbing the playhead. std::atomic requested_display_frame; ///< Frame index the user requested. int64_t current_display_frame; ///< Currently displayed frame (unused here, reserved). diff --git a/src/QtPlayer.cpp b/src/QtPlayer.cpp index d015d3a3..042e7351 100644 --- a/src/QtPlayer.cpp +++ b/src/QtPlayer.cpp @@ -164,11 +164,16 @@ namespace openshot } void QtPlayer::Seek(int64_t new_frame) + { + Seek(new_frame, true); + } + + void QtPlayer::Seek(int64_t new_frame, bool start_preroll) { // Check for seek if (reader && threads_started && new_frame > 0) { // Notify cache thread that seek has occurred - p->videoCache->Seek(new_frame, true); + p->videoCache->Seek(new_frame, start_preroll); // Notify audio thread that seek has occurred p->audioPlayback->Seek(new_frame); diff --git a/src/QtPlayer.h b/src/QtPlayer.h index 2f7ba769..a59e8bf6 100644 --- a/src/QtPlayer.h +++ b/src/QtPlayer.h @@ -75,6 +75,9 @@ namespace openshot /// Seek to a specific frame in the player void Seek(int64_t new_frame); + /// Seek to a specific frame, optionally triggering preroll cache rebuild. + void Seek(int64_t new_frame, bool start_preroll); + /// Set the source URL/path of this player (which will create an internal Reader) void SetSource(const std::string &source); diff --git a/tests/VideoCacheThread.cpp b/tests/VideoCacheThread.cpp index 5493a141..21e36aa4 100644 --- a/tests/VideoCacheThread.cpp +++ b/tests/VideoCacheThread.cpp @@ -42,6 +42,10 @@ public: void setMinFramesAhead(int64_t v) { min_frames_ahead.store(v); } void setLastDir(int d) { last_dir.store(d); } void forceUserSeekFlag() { userSeeked.store(true); } + bool isScrubbing() const { return scrub_active.load(); } + bool getUserSeekedFlag() const { return userSeeked.load(); } + bool getPrerollOnNextFill() const { return preroll_on_next_fill.load(); } + int64_t getRequestedDisplayFrame() const { return requested_display_frame.load(); } }; // ---------------------------------------------------------------------------- @@ -313,3 +317,84 @@ TEST_CASE("prefetchWindow: interrupt on userSeeked flag", "[VideoCacheThread]") CHECK(thread.getLastCachedIndex() == 23); CHECK(!wasFull); } + +TEST_CASE("Seek preview: inside cache is cheap and does not invalidate", "[VideoCacheThread]") { + TestableVideoCacheThread thread; + CacheMemory cache(/*max_bytes=*/100000000); + Timeline timeline(/*width=*/1280, /*height=*/720, /*fps=*/Fraction(24,1), + /*sample_rate=*/48000, /*channels=*/2, ChannelLayout::LAYOUT_STEREO); + timeline.SetCache(&cache); + thread.Reader(&timeline); + + cache.Add(std::make_shared(100, 0, 0)); + cache.Add(std::make_shared(101, 0, 0)); + REQUIRE(cache.Count() >= 2); + + thread.Seek(/*new_position=*/100, /*start_preroll=*/false); + + CHECK(thread.isScrubbing()); + CHECK(thread.getUserSeekedFlag()); + CHECK(!thread.getPrerollOnNextFill()); + CHECK(cache.Contains(100)); + CHECK(cache.Count() >= 2); +} + +TEST_CASE("Seek preview: outside cache marks uncached without preroll", "[VideoCacheThread]") { + TestableVideoCacheThread thread; + CacheMemory cache(/*max_bytes=*/100000000); + Timeline timeline(/*width=*/1280, /*height=*/720, /*fps=*/Fraction(24,1), + /*sample_rate=*/48000, /*channels=*/2, ChannelLayout::LAYOUT_STEREO); + timeline.SetCache(&cache); + thread.Reader(&timeline); + + cache.Add(std::make_shared(10, 0, 0)); + cache.Add(std::make_shared(11, 0, 0)); + REQUIRE(cache.Count() >= 2); + + thread.Seek(/*new_position=*/300, /*start_preroll=*/false); + + CHECK(thread.isScrubbing()); + CHECK(thread.getUserSeekedFlag()); + CHECK(!thread.getPrerollOnNextFill()); + CHECK(cache.Count() >= 2); +} + +TEST_CASE("Seek commit: exits scrub mode and enables preroll when uncached", "[VideoCacheThread]") { + TestableVideoCacheThread thread; + CacheMemory cache(/*max_bytes=*/100000000); + Timeline timeline(/*width=*/1280, /*height=*/720, /*fps=*/Fraction(24,1), + /*sample_rate=*/48000, /*channels=*/2, ChannelLayout::LAYOUT_STEREO); + timeline.SetCache(&cache); + thread.Reader(&timeline); + + thread.Seek(/*new_position=*/200, /*start_preroll=*/false); + CHECK(thread.isScrubbing()); + + thread.Seek(/*new_position=*/200, /*start_preroll=*/true); + + CHECK(!thread.isScrubbing()); + CHECK(thread.getUserSeekedFlag()); + CHECK(thread.getPrerollOnNextFill()); +} + +TEST_CASE("NotifyPlaybackPosition: ignored while scrubbing, applied after commit", "[VideoCacheThread]") { + TestableVideoCacheThread thread; + CacheMemory cache(/*max_bytes=*/100000000); + Timeline timeline(/*width=*/1280, /*height=*/720, /*fps=*/Fraction(24,1), + /*sample_rate=*/48000, /*channels=*/2, ChannelLayout::LAYOUT_STEREO); + timeline.SetCache(&cache); + thread.Reader(&timeline); + + thread.Seek(/*new_position=*/120, /*start_preroll=*/false); + REQUIRE(thread.isScrubbing()); + CHECK(thread.getRequestedDisplayFrame() == 120); + + thread.NotifyPlaybackPosition(/*new_position=*/25); + CHECK(thread.getRequestedDisplayFrame() == 120); + + thread.Seek(/*new_position=*/120, /*start_preroll=*/true); + REQUIRE(!thread.isScrubbing()); + + thread.NotifyPlaybackPosition(/*new_position=*/25); + CHECK(thread.getRequestedDisplayFrame() == 25); +}