diff --git a/src/Timeline.cpp b/src/Timeline.cpp index 6693fb038..663a83b1e 100644 --- a/src/Timeline.cpp +++ b/src/Timeline.cpp @@ -353,6 +353,8 @@ void Timeline::AddClip(Clip* clip) apply_mapper_to_clip(clip); } + InvalidateCacheForClip(clip); + // Add clip to list clips.push_back(clip); @@ -771,12 +773,11 @@ void Timeline::update_open_clips(Clip *clip, bool does_clip_intersect) } else if (!clip_found && does_clip_intersect) { - // Add clip to 'opened' list, because it's missing - open_clips[clip] = clip; - try { // Open the clip clip->Open(); + // Add clip to 'opened' list only after a successful open. + open_clips[clip] = clip; } catch (const InvalidFile & e) { // ... @@ -1378,6 +1379,7 @@ void Timeline::ApplyJsonDiff(std::string value) { try { const Json::Value root = openshot::stringToJson(value); + const uint64_t initial_cache_epoch = CacheEpoch(); // Process the JSON change array, loop through each item for (const Json::Value change : root) { std::string change_key = change["key"][(uint)0].asString(); @@ -1398,7 +1400,7 @@ void Timeline::ApplyJsonDiff(std::string value) { } // Timeline content changed: notify cache clients to rescan active window. - if (!root.empty()) { + if (!root.empty() && CacheEpoch() == initial_cache_epoch) { BumpCacheEpoch(); } } @@ -1413,6 +1415,18 @@ void Timeline::BumpCacheEpoch() { cache_epoch.fetch_add(1, std::memory_order_relaxed); } +void Timeline::InvalidateCacheForClip(const Clip* clip) { + if (!clip || !final_cache) { + return; + } + + const double fpsD = info.fps.ToDouble(); + const int64_t starting_frame = static_cast(std::llround(clip->Position() * fpsD)) + 1; + const int64_t ending_frame = static_cast(std::llround((clip->Position() + clip->Duration()) * fpsD)) + 1; + final_cache->Remove(starting_frame - 8, ending_frame + 8); + BumpCacheEpoch(); +} + // Apply JSON diff to clips void Timeline::apply_json_to_clips(Json::Value change) { @@ -1501,11 +1515,6 @@ void Timeline::apply_json_to_clips(Json::Value change) { // Add clip to timeline AddClip(clip); - // Calculate start and end frames that this impacts, and remove those frames from the cache - int64_t new_starting_frame = (clip->Position() * info.fps.ToDouble()) + 1; - int64_t new_ending_frame = ((clip->Position() + clip->Duration()) * info.fps.ToDouble()) + 1; - final_cache->Remove(new_starting_frame - 8, new_ending_frame + 8); - } else if (change_type == "update") { // Update existing clip diff --git a/src/Timeline.h b/src/Timeline.h index 98957de5f..d797f0744 100644 --- a/src/Timeline.h +++ b/src/Timeline.h @@ -217,6 +217,9 @@ namespace openshot { /// Increment the cache invalidation epoch. void BumpCacheEpoch(); + /// Remove cached timeline frames covered by a clip and notify cache clients. + void InvalidateCacheForClip(const openshot::Clip* clip); + public: /// @brief Constructor for the timeline (which configures the default frame properties) diff --git a/tests/Timeline.cpp b/tests/Timeline.cpp index 0c3b2a88e..fcd6b418b 100644 --- a/tests/Timeline.cpp +++ b/tests/Timeline.cpp @@ -202,6 +202,65 @@ class TimelineConstantAudioReader : public ReaderBase { void SetJsonValue(const Json::Value root) override { ReaderBase::SetJsonValue(root); } }; +class TimelineFailFirstOpenReader : public ReaderBase { +private: + bool is_open = false; + bool fail_next_open = false; + CacheMemory cache; + QColor color; + +public: + TimelineFailFirstOpenReader(int width, + int height, + int fps_num, + int fps_den, + int64_t length_frames, + const QColor& fill_color) + : color(fill_color) { + info.has_video = true; + info.has_audio = false; + info.width = width; + info.height = height; + info.fps = Fraction(fps_num, fps_den); + info.video_length = length_frames; + info.duration = static_cast(length_frames / info.fps.ToDouble()); + info.sample_rate = 48000; + info.channels = 2; + info.audio_stream_index = -1; + } + + openshot::CacheBase* GetCache() override { return &cache; } + bool IsOpen() override { return is_open; } + std::string Name() override { return "TimelineFailFirstOpenReader"; } + void FailNextOpen() { fail_next_open = true; } + void Open() override { + if (fail_next_open) { + fail_next_open = false; + throw InvalidFile("synthetic first open failure", ""); + } + is_open = true; + } + void Close() override { is_open = false; } + + std::shared_ptr GetFrame(int64_t number) override { + if (!is_open) + throw ReaderClosed("synthetic reader is closed"); + auto frame = std::make_shared(number, info.width, info.height, "#00000000"); + frame->GetImage()->fill(color); + return frame; + } + + std::string Json() const override { return JsonValue().toStyledString(); } + Json::Value JsonValue() const override { + Json::Value root = ReaderBase::JsonValue(); + root["type"] = "TimelineFailFirstOpenReader"; + root["path"] = ""; + return root; + } + void SetJson(const std::string value) override { (void) value; } + void SetJsonValue(const Json::Value root) override { ReaderBase::SetJsonValue(root); } +}; + static double expected_equal_power_gain(int64_t frame_number, int64_t start_frame, int64_t end_frame, bool fades_in) { constexpr double kHalfPi = 1.57079632679489661923; if (end_frame <= start_frame) @@ -1562,6 +1621,81 @@ TEST_CASE( "ApplyJSONDiff insert invalidates overlapping timeline cache", "[libo CHECK(!t.GetCache()->Contains(10)); } +TEST_CASE( "AddClip replaces stale cached black timeline frames with new clip content", "[libopenshot][timeline][cache]" ) +{ + Timeline t(640, 480, Fraction(30, 1), 44100, 2, LAYOUT_STEREO); + t.Open(); + + // Cache two frames while the timeline has no clips. This reproduces the + // stale final_cache state VideoCacheThread can build before a simple edit. + std::shared_ptr cached_before = t.GetFrame(10); + std::shared_ptr cached_far = t.GetFrame(400); + REQUIRE(cached_before != nullptr); + REQUIRE(cached_far != nullptr); + REQUIRE(t.GetCache() != nullptr); + REQUIRE(t.GetCache()->Contains(10)); + REQUIRE(t.GetCache()->Contains(400)); + CHECK(cached_before->GetImage()->pixelColor(320, 240) == QColor(0, 0, 0, 255)); + + TimelineSolidColorReader red_reader( + /*width=*/640, /*height=*/480, /*fps_num=*/30, /*fps_den=*/1, /*length_frames=*/300, + QColor(220, 20, 30, 255) + ); + Clip clip(&red_reader); + clip.Id("ADDCLIP_CACHE_INVALIDATE"); + clip.Layer(1); + clip.Position(0.0); + clip.Start(0.0); + clip.End(10.0); + t.AddClip(&clip); + + CHECK(!t.GetCache()->Contains(10)); + CHECK(t.GetCache()->Contains(400)); + + std::shared_ptr refreshed = t.GetFrame(10); + REQUIRE(refreshed != nullptr); + const QColor refreshed_pixel = refreshed->GetImage()->pixelColor(320, 240); + CHECK(refreshed_pixel.red() == Approx(220).margin(2)); + CHECK(refreshed_pixel.green() == Approx(20).margin(2)); + CHECK(refreshed_pixel.blue() == Approx(30).margin(2)); + + t.RemoveClip(&clip); +} + +TEST_CASE( "Timeline retries opening intersecting clip after transient open failure", "[libopenshot][timeline]" ) +{ + Timeline t(640, 480, Fraction(30, 1), 44100, 2, LAYOUT_STEREO); + t.Open(); + t.AutoMapClips(false); + + TimelineFailFirstOpenReader reader( + /*width=*/640, /*height=*/480, /*fps_num=*/30, /*fps_den=*/1, /*length_frames=*/300, + QColor(30, 210, 40, 255) + ); + Clip clip(&reader); + reader.FailNextOpen(); + clip.Id("RETRY_OPEN_AFTER_FAILURE"); + clip.Layer(5000000); + clip.Position(0.0); + clip.Start(0.0); + clip.End(10.0); + t.AddClip(&clip); + + std::shared_ptr failed_open_frame = t.GetFrame(1); + REQUIRE(failed_open_frame != nullptr); + CHECK(failed_open_frame->GetImage()->pixelColor(320, 240) == QColor(0, 0, 0, 255)); + + t.GetCache()->Clear(); + std::shared_ptr retried_frame = t.GetFrame(1); + REQUIRE(retried_frame != nullptr); + const QColor retried_pixel = retried_frame->GetImage()->pixelColor(320, 240); + CHECK(retried_pixel.red() == Approx(30).margin(2)); + CHECK(retried_pixel.green() == Approx(210).margin(2)); + CHECK(retried_pixel.blue() == Approx(40).margin(2)); + + t.RemoveClip(&clip); +} + TEST_CASE( "ApplyJSONDiff alpha updates refresh fixed-frame preview content", "[libopenshot][timeline]" ) { // Deterministic solid-color readers avoid any fixture/image ambiguity.