From 90d0710d782a3563b66387d2682071fa9004154c Mon Sep 17 00:00:00 2001 From: Jonathan Thomas Date: Sun, 1 Mar 2026 16:56:42 -0600 Subject: [PATCH] Fixing some regressions in performance of Mask effect, and flickering cached frames during updates (i.e. property slider updates) --- src/Clip.cpp | 33 +++++------- src/Clip.h | 3 -- src/EffectBase.cpp | 37 +++++++++++-- src/EffectBase.h | 6 +++ src/Qt/PlayerPrivate.cpp | 5 +- src/Qt/VideoCacheThread.cpp | 102 ++++++++++++++++++++++++++++-------- src/effects/Mask.cpp | 15 ------ src/effects/Mask.h | 4 +- 8 files changed, 133 insertions(+), 72 deletions(-) diff --git a/src/Clip.cpp b/src/Clip.cpp index 8f863c7b..937f955c 100644 --- a/src/Clip.cpp +++ b/src/Clip.cpp @@ -512,24 +512,27 @@ std::shared_ptr Clip::GetFrame(std::shared_ptr backgroun final_cache.Add(frame); } - if (!background_frame) { + const bool has_external_background = (background_frame != nullptr); + if (!background_frame) { // Create missing background_frame w/ transparent color (if needed) background_frame = std::make_shared(frame->number, frame->GetWidth(), frame->GetHeight(), "#00000000", frame->GetAudioSamplesCount(), frame->GetAudioChannelsCount()); } - const bool from_timeline = (options != nullptr); - if (from_timeline) { - // Timeline path composites into destination only, preserving cached clip pixels. - compose_onto_background(frame, background_frame); - return frame; + // Direct callers that pass their own background receive a copied frame + // to avoid mutating cached clip frames. + if (!options && has_external_background) { + auto output = std::make_shared(*frame.get()); + apply_background(output, background_frame); + return output; } - // Direct clip callers expect the returned frame image to include compositing. - auto output = std::make_shared(*frame.get()); - apply_background(output, background_frame); - return output; + // Apply background canvas (i.e. flatten this image onto previous layer image) + apply_background(frame, background_frame); + + // Return processed 'frame' + return frame; } else // Throw error if reader not initialized @@ -1283,16 +1286,6 @@ void Clip::apply_background(std::shared_ptr frame, std::shared_ frame->AddImage(background_canvas); } -void Clip::compose_onto_background(std::shared_ptr frame, std::shared_ptr background_frame) { - // Composite onto background without mutating source clip frame image. - auto canvas = background_frame->GetImage(); - QPainter painter(canvas.get()); - painter.setCompositionMode(static_cast(composite)); - painter.drawImage(0, 0, *frame->GetImage()); - painter.end(); - background_frame->AddImage(canvas); -} - // Apply effects to the source frame (if any) void Clip::apply_effects(std::shared_ptr frame, int64_t timeline_frame_number, TimelineInfoStruct* options, bool before_keyframes) { diff --git a/src/Clip.h b/src/Clip.h index 80749001..cfb37768 100644 --- a/src/Clip.h +++ b/src/Clip.h @@ -130,9 +130,6 @@ namespace openshot { /// Apply background image to the current clip image (i.e. flatten this image onto previous layer) void apply_background(std::shared_ptr frame, std::shared_ptr background_frame); - /// Composite clip image onto a destination frame without mutating clip frame image. - void compose_onto_background(std::shared_ptr frame, std::shared_ptr background_frame); - /// Apply effects to the source frame (if any) void apply_effects(std::shared_ptr frame, int64_t timeline_frame_number, TimelineInfoStruct* options, bool before_keyframes); diff --git a/src/EffectBase.cpp b/src/EffectBase.cpp index b65ad5ba..0b5aad92 100644 --- a/src/EffectBase.cpp +++ b/src/EffectBase.cpp @@ -328,6 +328,9 @@ void EffectBase::MaskReader(ReaderBase* new_reader) { } mask_reader = new_reader; + cached_single_mask_image.reset(); + cached_single_mask_width = 0; + cached_single_mask_height = 0; if (mask_reader) mask_reader->ParentClip(clip); } @@ -437,15 +440,26 @@ std::shared_ptr EffectBase::GetMaskImage(std::shared_ptr target_ return {}; std::shared_ptr source_mask; + bool used_cached_scaled = false; #pragma omp critical (open_effect_mask_reader) { try { if (!mask_reader->IsOpen()) mask_reader->Open(); - const int64_t mapped_frame = MapMaskFrameNumber(frame_number); - auto source_frame = mask_reader->GetFrame(mapped_frame); - if (source_frame && source_frame->GetImage() && !source_frame->GetImage()->isNull()) - source_mask = std::make_shared(*source_frame->GetImage()); + + if (mask_reader->info.has_single_image && + cached_single_mask_image && + cached_single_mask_width == target_image->width() && + cached_single_mask_height == target_image->height()) { + source_mask = cached_single_mask_image; + used_cached_scaled = true; + } + else { + const int64_t mapped_frame = MapMaskFrameNumber(frame_number); + auto source_frame = mask_reader->GetFrame(mapped_frame); + if (source_frame && source_frame->GetImage() && !source_frame->GetImage()->isNull()) + source_mask = std::make_shared(*source_frame->GetImage()); + } } catch (const std::exception& e) { ZmqLogger::Instance()->Log( std::string("EffectBase::GetMaskImage unable to read mask frame: ") + e.what()); @@ -456,10 +470,19 @@ std::shared_ptr EffectBase::GetMaskImage(std::shared_ptr target_ if (!source_mask || source_mask->isNull()) return {}; - return std::make_shared( + if (used_cached_scaled) + return source_mask; + + auto scaled_mask = std::make_shared( source_mask->scaled( target_image->width(), target_image->height(), Qt::IgnoreAspectRatio, Qt::SmoothTransformation)); + if (mask_reader->info.has_single_image) { + cached_single_mask_image = scaled_mask; + cached_single_mask_width = target_image->width(); + cached_single_mask_height = target_image->height(); + } + return scaled_mask; } void EffectBase::BlendWithMask(std::shared_ptr original_image, std::shared_ptr effected_image, @@ -499,6 +522,10 @@ std::shared_ptr EffectBase::ProcessFrame(std::shared_ptrGetImage(); if (!pre_image || pre_image->isNull()) return GetFrame(frame, frame_number); diff --git a/src/EffectBase.h b/src/EffectBase.h index f0ded7ee..77da0387 100644 --- a/src/EffectBase.h +++ b/src/EffectBase.h @@ -58,6 +58,9 @@ namespace openshot private: int order; ///< The order to evaluate this effect. Effects are processed in this order (when more than one overlap). ReaderBase* mask_reader = nullptr; ///< Optional common reader-based mask source. + std::shared_ptr cached_single_mask_image; ///< Cached scaled mask for still-image mask sources. + int cached_single_mask_width = 0; ///< Cached mask width. + int cached_single_mask_height = 0; ///< Cached mask height. /// Build or refresh a mask image that matches target_image dimensions. std::shared_ptr GetMaskImage(std::shared_ptr target_image, int64_t frame_number); @@ -93,6 +96,9 @@ namespace openshot virtual void ApplyCustomMaskBlend(std::shared_ptr original_image, std::shared_ptr effected_image, std::shared_ptr mask_image, int64_t frame_number) const {} + /// Optional override for effects that apply mask processing inside GetFrame(). + virtual bool HandlesMaskInternally() const { return false; } + public: /// Parent effect (which properties will set this effect properties) EffectBase* parentEffect; diff --git a/src/Qt/PlayerPrivate.cpp b/src/Qt/PlayerPrivate.cpp index 38d84a12..bacaef73 100644 --- a/src/Qt/PlayerPrivate.cpp +++ b/src/Qt/PlayerPrivate.cpp @@ -192,9 +192,8 @@ namespace openshot // Drop local frame reference so same-frame refreshes cannot reuse stale // content after timeline/clip property updates. frame.reset(); - // 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); + // Always force immediate refresh after seek/update, even while playing. + is_dirty = true; } // Start video/audio playback diff --git a/src/Qt/VideoCacheThread.cpp b/src/Qt/VideoCacheThread.cpp index ddc737b8..a3c9aabd 100644 --- a/src/Qt/VideoCacheThread.cpp +++ b/src/Qt/VideoCacheThread.cpp @@ -140,42 +140,98 @@ namespace openshot bool cache_contains = false; bool should_clear_cache = false; CacheBase* cache = reader ? reader->GetCache() : nullptr; + const int64_t current_requested = requested_display_frame.load(); + const bool same_frame_refresh = (new_position == current_requested); if (cache) { cache_contains = cache->Contains(new_position); } if (start_preroll) { - should_mark_seek = true; + if (same_frame_refresh) { + const bool is_paused = (speed.load() == 0); + if (is_paused) { + // Paused same-frame edits (dragging keyframed properties) + // must not reuse any stale composite/cache state. + if (Timeline* timeline = dynamic_cast(reader)) { + timeline->ClearAllCache(); + } + new_cached_count = 0; + should_mark_seek = true; + should_preroll = true; + should_clear_cache = false; + } else { + // Same-frame refresh during playback should stay lightweight. + should_mark_seek = false; + should_preroll = false; + should_clear_cache = false; + if (cache && cache_contains) { + cache->Remove(new_position); + } + if (cache) { + new_cached_count = cache->Count(); + } + } + } else { + should_mark_seek = true; - 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(); + 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) { - if (cache_contains) { + // Non-preroll seeks are used for: + // 1) paused scrubbing (needs seek/scrub semantics), and + // 2) live refreshes while playing (must stay lightweight). + const bool is_paused = (speed.load() == 0); + if (is_paused && same_frame_refresh) { + // Property updates at the same paused playhead frame should + // refresh that frame only, without full seek/scrub churn. + should_mark_seek = false; + should_preroll = false; + should_clear_cache = false; + if (cache && cache_contains) { cache->Remove(new_position); } - new_cached_count = cache->Count(); + if (cache) { + new_cached_count = cache->Count(); + } + leaving_scrub = true; + } + else if (is_paused) { + should_mark_seek = true; + if (cache && !cache_contains) { + new_cached_count = 0; + should_clear_cache = true; + } + else if (cache) { + if (cache_contains) { + cache->Remove(new_position); + } + new_cached_count = cache->Count(); + } + entering_scrub = true; + } else { + // During playback, avoid seek/scrub side effects that can + // churn cache state and cause visible flicker on updates. + should_mark_seek = false; + should_preroll = false; + should_clear_cache = false; + if (cache) { + new_cached_count = cache->Count(); + } + leaving_scrub = true; } - entering_scrub = true; } { diff --git a/src/effects/Mask.cpp b/src/effects/Mask.cpp index 91b8cfb3..2254c018 100644 --- a/src/effects/Mask.cpp +++ b/src/effects/Mask.cpp @@ -126,21 +126,6 @@ std::shared_ptr Mask::GetFrame(std::shared_ptr return frame; } -bool Mask::UseCustomMaskBlend(int64_t frame_number) const { - (void) frame_number; - // Mask effect already applies its own mask operation in GetFrame(). - return true; -} - -void Mask::ApplyCustomMaskBlend(std::shared_ptr original_image, std::shared_ptr effected_image, - std::shared_ptr mask_image, int64_t frame_number) const { - (void) original_image; - (void) effected_image; - (void) mask_image; - (void) frame_number; - // Intentionally no-op to skip base post-blend for Mask effects. -} - // Generate JSON string of this object std::string Mask::Json() const { diff --git a/src/effects/Mask.h b/src/effects/Mask.h index d6fa8a92..8e9eb88d 100644 --- a/src/effects/Mask.h +++ b/src/effects/Mask.h @@ -40,9 +40,7 @@ namespace openshot void init_effect_details(); protected: - bool UseCustomMaskBlend(int64_t frame_number) const override; - void ApplyCustomMaskBlend(std::shared_ptr original_image, std::shared_ptr effected_image, - std::shared_ptr mask_image, int64_t frame_number) const override; + bool HandlesMaskInternally() const override { return true; } public: bool replace_image; ///< Replace the frame image with a grayscale image representing the mask. Great for debugging a mask.