Report zero- or negative-length subtitles as an error.v1.10.19

This is distinct from the Bv2.1 warning about subtitles being shorter
than 15 frames.  Also fix an assertion failure when negative-length
subtitles are seen.

4 files changed, 56 insertions, 15 deletions

diff --git a/src/verify.cc b/src/verify.cc
index 4be23861..b7ffeb0d 100644
--- a/src/verify.cc
+++ b/src/verify.cc
@@ -892,6 +892,7 @@ verify_text_details (
 	/* end of last subtitle (in editable units) */
 	optional<int64_t> last_out;
 	auto too_short = false;
+	auto too_short_bv21 = false;
 	auto too_close = false;
 	auto too_early = false;
 	auto reel_overlap = false;
@@ -902,7 +903,7 @@ verify_text_details (
 
 	std::function<void (cxml::ConstNodePtr, optional<int>, optional<Time>, int, bool, bool&, vector<string>&)> parse;
 
-	parse = [&parse, &last_out, &too_short, &too_close, &too_early, &empty_text, &reel_offset, &missing_load_font_id](
+	parse = [&parse, &last_out, &too_short, &too_short_bv21, &too_close, &too_early, &empty_text, &reel_offset, &missing_load_font_id](
 		cxml::ConstNodePtr node,
 		optional<int> tcr,
 		optional<Time> start_time,
@@ -924,8 +925,10 @@ verify_text_details (
 				too_early = true;
 			}
 			auto length = out - in;
-			if (length.as_editable_units_ceil(er) < 15) {
+			if (length.as_editable_units_ceil(er) <= 0) {
 				too_short = true;
+			} else if (length.as_editable_units_ceil(er) < 15) {
+				too_short_bv21 = true;
 			}
 			if (last_out) {
 				/* XXX: this feels dubious - is it really what Bv2.1 means? */
@@ -1024,7 +1027,11 @@ verify_text_details (
 	}
 
 	if (too_short) {
-		context.warning(VerificationNote::Code::INVALID_SUBTITLE_DURATION);
+		context.error(VerificationNote::Code::INVALID_SUBTITLE_DURATION);
+	}
+
+	if (too_short_bv21) {
+		context.warning(VerificationNote::Code::INVALID_SUBTITLE_DURATION_BV21);
 	}
 
 	if (too_close) {
@@ -1222,14 +1229,20 @@ dcp::verify_text_lines_and_characters(
 
 		if (i->start) {
 			/* end of a subtitle */
-			DCP_ASSERT (current.find(i->start->position) != current.end());
-			auto current_position = current[i->start->position];
-			auto iter = std::find(current_position.begin(), current_position.end(), i->start);
-			if (iter != current_position.end()) {
-				current_position.erase(iter);
-			}
-			if (current_position.empty()) {
-				current.erase(i->start->position);
+			auto iter = current.find(i->start->position);
+			/* It could be that there's no entry in current for this start position:
+			 * perhaps the end is before the start, or something else bad.
+			 */
+			if (iter != current.end()) {
+				DCP_ASSERT (current.find(i->start->position) != current.end());
+				auto current_position = current[i->start->position];
+				auto iter = std::find(current_position.begin(), current_position.end(), i->start);
+				if (iter != current_position.end()) {
+					current_position.erase(iter);
+				}
+				if (current_position.empty()) {
+					current.erase(i->start->position);
+				}
 			}
 		} else {
 			/* start of a subtitle */
@@ -1986,6 +1999,8 @@ dcp::note_to_string(VerificationNote note, function<string (string)> process_str
 	case VerificationNote::Code::INVALID_SUBTITLE_FIRST_TEXT_TIME:
 		return process_string("The first subtitle or closed caption is less than 4 seconds from the start of the DCP.");
 	case VerificationNote::Code::INVALID_SUBTITLE_DURATION:
+		return process_string("At least one subtitle has a zero or negative duration.");
+	case VerificationNote::Code::INVALID_SUBTITLE_DURATION_BV21:
 		return process_string("At least one subtitle lasts less than 15 frames.");
 	case VerificationNote::Code::INVALID_SUBTITLE_SPACING:
 		return process_string("At least one pair of subtitles is separated by less than 2 frames.");
diff --git a/src/verify.h b/src/verify.h
index 8a75dfa3..6499f815 100644
--- a/src/verify.h
+++ b/src/verify.h
@@ -238,8 +238,10 @@ public:
 		INVALID_SUBTITLE_START_TIME,
 		/** The first subtitle or closed caption happens before 4s into the first reel [Bv2.1_7.2.4] */
 		INVALID_SUBTITLE_FIRST_TEXT_TIME,
-		/** At least one subtitle is less than the minimum of 15 frames suggested by [Bv2.1_7.2.5] */
+		/** At least one subtitle has a zero or negative duration */
 		INVALID_SUBTITLE_DURATION,
+		/** At least one subtitle is less than the minimum of 15 frames suggested by [Bv2.1_7.2.5] */
+		INVALID_SUBTITLE_DURATION_BV21,
 		/** At least one pair of subtitles are separated by less than the the minimum of 2 frames suggested by [Bv2.1_7.2.5] */
 		INVALID_SUBTITLE_SPACING,
 		/** A subtitle lasts for longer than the reel which contains it */
diff --git a/test/combine_test.cc b/test/combine_test.cc
index dc72d2d4..b26520f3 100644
--- a/test/combine_test.cc
+++ b/test/combine_test.cc
@@ -92,7 +92,7 @@ check_no_errors (boost::filesystem::path path)
 	std::copy_if (notes.begin(), notes.end(), std::back_inserter(filtered_notes), [](dcp::VerificationNote const& i) {
 		return i.type() != dcp::VerificationNote::Type::OK &&
 			i.code() != dcp::VerificationNote::Code::INVALID_STANDARD &&
-			i.code() != dcp::VerificationNote::Code::INVALID_SUBTITLE_DURATION;
+			i.code() != dcp::VerificationNote::Code::INVALID_SUBTITLE_DURATION_BV21;
 	});
 	dump_notes (filtered_notes);
 	BOOST_CHECK (filtered_notes.empty());
diff --git a/test/verify_test.cc b/test/verify_test.cc
index 6c09cbe0..af3d135e 100644
--- a/test/verify_test.cc
+++ b/test/verify_test.cc
@@ -2783,9 +2783,33 @@ BOOST_AUTO_TEST_CASE (verify_valid_subtitle_spacing)
 }
 
 
-BOOST_AUTO_TEST_CASE (verify_invalid_subtitle_duration)
+BOOST_AUTO_TEST_CASE(verify_invalid_subtitle_duration)
 {
 	auto const dir = path("build/test/verify_invalid_subtitle_duration");
+	auto cpl = dcp_with_text<dcp::ReelSMPTETextAsset>(dcp::TextType::OPEN_SUBTITLE, dir, {{ 4 * 24, 4 * 24 - 1 }});
+	check_verify_result (
+		{dir},
+		{},
+		{
+			ok(dcp::VerificationNote::Code::NONE_ENCRYPTED, cpl),
+			ok(dcp::VerificationNote::Code::MATCHING_CPL_HASHES, cpl),
+			ok(dcp::VerificationNote::Code::VALID_CPL_ANNOTATION_TEXT, string{"hello"}, cpl),
+			ok(dcp::VerificationNote::Code::MATCHING_PKL_ANNOTATION_TEXT_WITH_CPL, cpl),
+			ok(dcp::VerificationNote::Code::VALID_CONTENT_KIND, string{"trailer"}, cpl),
+			ok(dcp::VerificationNote::Code::VALID_CONTENT_VERSION_LABEL_TEXT, cpl->content_version()->label_text, cpl),
+			dcp::VerificationNote(
+				dcp::VerificationNote::Type::ERROR, dcp::VerificationNote::Code::INVALID_SUBTITLE_DURATION
+				).set_cpl_id(cpl->id()),
+			dcp::VerificationNote(
+				dcp::VerificationNote::Type::BV21_ERROR, dcp::VerificationNote::Code::MISSING_CPL_METADATA, cpl->file().get()
+				).set_cpl_id(cpl->id())
+		});
+}
+
+
+BOOST_AUTO_TEST_CASE(verify_invalid_subtitle_duration_bv21)
+{
+	auto const dir = path("build/test/verify_invalid_subtitle_duration_bv21");
 	auto cpl = dcp_with_text<dcp::ReelSMPTETextAsset>(dcp::TextType::OPEN_SUBTITLE, dir, {{ 4 * 24, 4 * 24 + 1 }});
 	check_verify_result (
 		{dir},
@@ -2798,7 +2822,7 @@ BOOST_AUTO_TEST_CASE (verify_invalid_subtitle_duration)
 			ok(dcp::VerificationNote::Code::VALID_CONTENT_KIND, string{"trailer"}, cpl),
 			ok(dcp::VerificationNote::Code::VALID_CONTENT_VERSION_LABEL_TEXT, cpl->content_version()->label_text, cpl),
 			dcp::VerificationNote(
-				dcp::VerificationNote::Type::WARNING, dcp::VerificationNote::Code::INVALID_SUBTITLE_DURATION
+				dcp::VerificationNote::Type::WARNING, dcp::VerificationNote::Code::INVALID_SUBTITLE_DURATION_BV21
 				).set_cpl_id(cpl->id()),
 			dcp::VerificationNote(
 				dcp::VerificationNote::Type::BV21_ERROR, dcp::VerificationNote::Code::MISSING_CPL_METADATA, cpl->file().get()