Merge pull request #1077 from OpenShot/black-frames

Avoid Black Frames (Adding a new clip - and it does not appear)

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<int64_t>(std::llround(clip->Position() * fpsD)) + 1;
+	const int64_t ending_frame = static_cast<int64_t>(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

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)

tests/Timeline.cpp

@@ -203,6 +203,65 @@ public:
 	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<float>(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<openshot::Frame> GetFrame(int64_t number) override {
+		if (!is_open)
+			throw ReaderClosed("synthetic reader is closed");
+		auto frame = std::make_shared<Frame>(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)
@@ -1627,6 +1686,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<Frame> cached_before = t.GetFrame(10);
+	std::shared_ptr<Frame> 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<Frame> 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<Frame> 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<Frame> 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.