From: Carl Hetherington <cth@carlh.net>
Date: Sun, 31 Mar 2024 23:04:41 +0000 (+0200)
Subject: Fix hanging/overlapping dvb subtitles (#2792).
X-Git-Tag: v2.16.80~5
X-Git-Url: https://git.carlh.net/gitweb/?p=dcpomatic.git;a=commitdiff_plain;h=94618a724124cbf5fe9f0b47a3fdce601fcd5581

Fix hanging/overlapping dvb subtitles (#2792).

This reverts a change made in
8ca6fd6d97e6d42492afddb655fa85130946853c
"Fix doubled subtitles if subtitle stop times are specified."

That change breaks the case where a subtitle _does_ have a stop time,
but it's wrong (30s from the start time) and we want the next subtitle
to clear the previous one.

I can't now see how reverting this could cause doubled subtitles,
so maybe that problem wlil come back.  At least now there's a test
for #2792.
---

diff --git a/src/lib/ffmpeg_decoder.cc b/src/lib/ffmpeg_decoder.cc
index 7f7a07863..6130d8e5f 100644
--- a/src/lib/ffmpeg_decoder.cc
+++ b/src/lib/ffmpeg_decoder.cc
@@ -668,11 +668,11 @@ FFmpegDecoder::decode_and_process_subtitle_packet (AVPacket* packet)
 	*/
 	ContentTime from;
 	from = sub_period.from + _pts_offset;
+	_have_current_subtitle = true;
 	if (sub_period.to) {
 		_current_subtitle_to = *sub_period.to + _pts_offset;
 	} else {
 		_current_subtitle_to = optional<ContentTime>();
-		_have_current_subtitle = true;
 	}
 
 	ContentBitmapText bitmap_text(from);
diff --git a/test/subtitle_timing_test.cc b/test/subtitle_timing_test.cc
index 79cc68849..8bb18f304 100644
--- a/test/subtitle_timing_test.cc
+++ b/test/subtitle_timing_test.cc
@@ -21,8 +21,13 @@
 
 #include "lib/content.h"
 #include "lib/content_factory.h"
+#include "lib/content_text.h"
+#include "lib/dcpomatic_time.h"
 #include "lib/film.h"
+#include "lib/ffmpeg_content.h"
+#include "lib/ffmpeg_decoder.h"
 #include "lib/text_content.h"
+#include "lib/text_decoder.h"
 #include "lib/video_content.h"
 #include "test.h"
 #include <dcp/cpl.h>
@@ -33,6 +38,9 @@
 #include <iostream>
 
 
+using std::dynamic_pointer_cast;
+
+
 BOOST_AUTO_TEST_CASE (test_subtitle_timing_with_frame_rate_change)
 {
 	Cleanup cl;
@@ -72,3 +80,66 @@ BOOST_AUTO_TEST_CASE (test_subtitle_timing_with_frame_rate_change)
 	cl.run();
 }
 
+
+BOOST_AUTO_TEST_CASE(dvb_subtitles_replace_the_last)
+{
+	/* roh.mkv contains subtitles that come out of FFmpeg with incorrect stop times (30s
+	 * after the start, which seems to be some kind of DVB "standard" timeout).
+	 * Between actual subtitles it contains blanks that are apparently supposed to clear
+	 * the previous subtitle.  Make sure that happens.
+	 */
+	auto content = content_factory(TestPaths::private_data() / "roh.mkv");
+	BOOST_REQUIRE(!content.empty());
+	auto film = new_test_film2("dvb_subtitles_replace_the_last", { content[0] });
+
+	FFmpegDecoder decoder(film, dynamic_pointer_cast<FFmpegContent>(content[0]), false);
+	BOOST_REQUIRE(!decoder.text.empty());
+
+	struct Event {
+		std::string type;
+		dcpomatic::ContentTime time;
+
+		bool operator==(Event const& other) const {
+			return type == other.type && time == other.time;
+		}
+	};
+
+	std::vector<Event> events;
+
+	auto start = [&events](ContentBitmapText text) {
+		events.push_back({"start", text.from()});
+	};
+
+	auto stop = [&events](dcpomatic::ContentTime time) {
+		if (!events.empty() && events.back().type == "stop") {
+			/* We'll get a bad (too-late) stop time, then the correct one
+			 * when the "clearing" subtitle arrives.
+			 */
+			events.pop_back();
+		}
+		events.push_back({"stop", time});
+	};
+
+	decoder.text.front()->BitmapStart.connect(start);
+	decoder.text.front()->Stop.connect(stop);
+
+	while (!decoder.pass()) {}
+
+	using dcpomatic::ContentTime;
+
+	std::vector<Event> correct = {
+		{ "start", ContentTime(439872) },  // 4.582000s     actual subtitle #1
+		{ "stop",  ContentTime(998400) },  // 10.400000s    stop caused by incoming blank
+		{ "start", ContentTime(998400) },  // 10.400000s    blank
+		{ "stop",  ContentTime(1141248) }, // 11.888000s    stop caused by incoming subtitle #2
+		{ "start", ContentTime(1141248) }, // 11.888000s    subtitle #2
+		{ "stop",  ContentTime(1455936) }, // 15.166000s    ...
+		{ "start", ContentTime(1455936) }, // 15.166000s
+		{ "stop",  ContentTime(1626816) }, // 16.946000s
+		{ "start", ContentTime(1626816) }, // 16.946000s
+	};
+
+	BOOST_REQUIRE(events.size() > correct.size());
+	BOOST_CHECK(std::vector<Event>(events.begin(), events.begin() + correct.size()) == correct);
+}
+