Fix various bugs in subtitle/ccap verification.

Check that subtitles don't overlap reel boundaries, and fix a few
tests that trip this check.

Fix confusion when calculating subtitle timings during verification
where the picture asset frame rate was being used rather than the
subtitle asset's edit rate.

Do the subtitle timing verification for Interop as well as SMPTE
subtitles.

Take <StartTime> tags into account when checking subtitles, even
though Bv2.1 says they should be set to 0.

Rename Time::as_editable_units to Time::as_editable_units_ceil
and add a _floor variant, then use that to round down when checking
reel boundary overlaps.

7 files changed, 76 insertions, 31 deletions

diff --git a/src/dcp_time.cc b/src/dcp_time.cc
index 2895cd37..0bd0a409 100644
--- a/src/dcp_time.cc
+++ b/src/dcp_time.cc
@@ -340,7 +340,14 @@ Time::as_string (Standard standard) const
 
 
 int64_t
-Time::as_editable_units (int tcr_) const
+Time::as_editable_units_floor (int tcr_) const
+{
+	return floor(int64_t(e) * double(tcr_) / tcr) + int64_t(s) * tcr_ + int64_t(m) * 60 * tcr_ + int64_t(h) * 60 * 60 * tcr_;
+}
+
+
+int64_t
+Time::as_editable_units_ceil (int tcr_) const
 {
 	return ceil(int64_t(e) * double(tcr_) / tcr) + int64_t(s) * tcr_ + int64_t(m) * 60 * tcr_ + int64_t(h) * 60 * 60 * tcr_;
 }
diff --git a/src/dcp_time.h b/src/dcp_time.h
index dcbadf31..dbfdb7f0 100644
--- a/src/dcp_time.h
+++ b/src/dcp_time.h
@@ -127,10 +127,16 @@ public:
 	double as_seconds () const;
 
 	/** @param tcr_ Timecode rate with which the return value should be counted
+	 *  @return the total number of editable units that this time consists of at the specified timecode rate, rounded down
+	 *  to the nearest editable unit. For example, as_editable_units_floor(24) returns the total time in frames at 24fps.
+	 */
+	int64_t as_editable_units_floor (int tcr_) const;
+
+	/** @param tcr_ Timecode rate with which the return value should be counted
 	 *  @return the total number of editable units that this time consists of at the specified timecode rate, rounded up
-	 *  to the nearest editable unit. For example, as_editable_units (24) returns the total time in frames at 24fps.
+	 *  to the nearest editable unit. For example, as_editable_units_ceil(24) returns the total time in frames at 24fps.
 	 */
-	int64_t as_editable_units (int tcr_) const;
+	int64_t as_editable_units_ceil (int tcr_) const;
 
 	/** @param tcr_ New timecode rate
 	 *  @return A new Time which is this time at the spcified new timecode rate
diff --git a/src/reel_markers_asset.cc b/src/reel_markers_asset.cc
index 360a855a..c6339df5 100644
--- a/src/reel_markers_asset.cc
+++ b/src/reel_markers_asset.cc
@@ -110,7 +110,7 @@ ReelMarkersAsset::write_to_cpl (xmlpp::Node* node, Standard standard) const
 	for (auto const& i: _markers) {
 		auto m = ml->add_child("Marker");
 		m->add_child("Label")->add_child_text(marker_to_string(i.first));
-		m->add_child("Offset")->add_child_text(raw_convert<string>(i.second.as_editable_units(tcr)));
+		m->add_child("Offset")->add_child_text(raw_convert<string>(i.second.as_editable_units_ceil(tcr)));
 	}
 
 	return asset;
diff --git a/src/smpte_subtitle_asset.cc b/src/smpte_subtitle_asset.cc
index 15a366ba..a55b91ae 100644
--- a/src/smpte_subtitle_asset.cc
+++ b/src/smpte_subtitle_asset.cc
@@ -195,7 +195,7 @@ SMPTESubtitleAsset::parse_xml (shared_ptr<cxml::Document> xml)
 	}
 
 	/* Guess intrinsic duration */
-	_intrinsic_duration = latest_subtitle_out().as_editable_units (_edit_rate.numerator / _edit_rate.denominator);
+	_intrinsic_duration = latest_subtitle_out().as_editable_units_ceil(_edit_rate.numerator / _edit_rate.denominator);
 }
 
 
@@ -547,5 +547,5 @@ void
 SMPTESubtitleAsset::add (shared_ptr<Subtitle> s)
 {
 	SubtitleAsset::add (s);
-	_intrinsic_duration = latest_subtitle_out().as_editable_units (_edit_rate.numerator / _edit_rate.denominator);
+	_intrinsic_duration = latest_subtitle_out().as_editable_units_ceil(_edit_rate.numerator / _edit_rate.denominator);
 }
diff --git a/src/subtitle_asset_internal.cc b/src/subtitle_asset_internal.cc
index f1674789..80e861f1 100644
--- a/src/subtitle_asset_internal.cc
+++ b/src/subtitle_asset_internal.cc
@@ -233,8 +233,8 @@ order::Subtitle::as_xml (xmlpp::Element* parent, Context& context) const
 		e->set_attribute ("FadeUpTime", _fade_up.rebase(context.time_code_rate).as_string(context.standard));
 		e->set_attribute ("FadeDownTime", _fade_down.rebase(context.time_code_rate).as_string(context.standard));
 	} else {
-		e->set_attribute ("FadeUpTime", raw_convert<string> (_fade_up.as_editable_units(context.time_code_rate)));
-		e->set_attribute ("FadeDownTime", raw_convert<string> (_fade_down.as_editable_units(context.time_code_rate)));
+		e->set_attribute ("FadeUpTime", raw_convert<string> (_fade_up.as_editable_units_ceil(context.time_code_rate)));
+		e->set_attribute ("FadeDownTime", raw_convert<string> (_fade_down.as_editable_units_ceil(context.time_code_rate)));
 	}
 	return e;
 }
diff --git a/src/verify.cc b/src/verify.cc
index bddf1683..97758e61 100644
--- a/src/verify.cc
+++ b/src/verify.cc
@@ -763,7 +763,7 @@ static
 void
 verify_text_timing (
 	vector<shared_ptr<Reel>> reels,
-	optional<int> picture_frame_rate,
+	int edit_rate,
 	vector<VerificationNote>& notes,
 	std::function<bool (shared_ptr<Reel>)> check,
 	std::function<string (shared_ptr<Reel>)> xml,
@@ -775,32 +775,39 @@ verify_text_timing (
 	auto too_short = false;
 	auto too_close = false;
 	auto too_early = false;
+	auto reel_overlap = false;
 	/* current reel start time (in editable units) */
 	int64_t reel_offset = 0;
 
-	std::function<void (cxml::ConstNodePtr, int, int, bool)> parse;
-	parse = [&parse, &last_out, &too_short, &too_close, &too_early, &reel_offset](cxml::ConstNodePtr node, int tcr, int pfr, bool first_reel) {
+	std::function<void (cxml::ConstNodePtr, optional<int>, optional<Time>, int, bool)> parse;
+	parse = [&parse, &last_out, &too_short, &too_close, &too_early, &reel_offset](cxml::ConstNodePtr node, optional<int> tcr, optional<Time> start_time, int er, bool first_reel) {
 		if (node->name() == "Subtitle") {
 			Time in (node->string_attribute("TimeIn"), tcr);
+			if (start_time) {
+				in -= *start_time;
+			}
 			Time out (node->string_attribute("TimeOut"), tcr);
-			if (first_reel && in < Time(0, 0, 4, 0, tcr)) {
+			if (start_time) {
+				out -= *start_time;
+			}
+			if (first_reel && tcr && in < Time(0, 0, 4, 0, *tcr)) {
 				too_early = true;
 			}
 			auto length = out - in;
-			if (length.as_editable_units(pfr) < 15) {
+			if (length.as_editable_units_ceil(er) < 15) {
 				too_short = true;
 			}
 			if (last_out) {
 				/* XXX: this feels dubious - is it really what Bv2.1 means? */
-				auto distance = reel_offset + in.as_editable_units(pfr) - *last_out;
+				auto distance = reel_offset + in.as_editable_units_ceil(er) - *last_out;
 				if (distance >= 0 && distance < 2) {
 					too_close = true;
 				}
 			}
-			last_out = reel_offset + out.as_editable_units(pfr);
+			last_out = reel_offset + out.as_editable_units_floor(er);
 		} else {
 			for (auto i: node->node_children()) {
-				parse(i, tcr, pfr, first_reel);
+				parse(i, tcr, start_time, er, first_reel);
 			}
 		}
 	};
@@ -814,11 +821,31 @@ verify_text_timing (
 		 * read in by libdcp's parser.
 		 */
 
-		auto doc = make_shared<cxml::Document>("SubtitleReel");
-		doc->read_string (xml(reels[i]));
-		auto const tcr = doc->number_child<int>("TimeCodeRate");
-		parse (doc, tcr, picture_frame_rate.get_value_or(24), i == 0);
-		reel_offset += duration(reels[i]);
+		shared_ptr<cxml::Document> doc;
+		optional<int> tcr;
+		optional<Time> start_time;
+		try {
+			doc = make_shared<cxml::Document>("SubtitleReel");
+			doc->read_string (xml(reels[i]));
+			tcr = doc->number_child<int>("TimeCodeRate");
+			auto start_time_string = doc->optional_string_child("StartTime");
+			if (start_time_string) {
+				start_time = Time(*start_time_string, tcr);
+			}
+		} catch (...) {
+			doc = make_shared<cxml::Document>("DCSubtitle");
+			doc->read_string (xml(reels[i]));
+		}
+		parse (doc, tcr, start_time, edit_rate, i == 0);
+		auto end = reel_offset + duration(reels[i]);
+		if (last_out && *last_out > end) {
+			reel_overlap = true;
+		}
+		reel_offset = end;
+	}
+
+	if (last_out && *last_out > reel_offset) {
+		reel_overlap = true;
 	}
 
 	if (too_early) {
@@ -838,6 +865,12 @@ verify_text_timing (
 			VerificationNote::Type::WARNING, VerificationNote::Code::INVALID_SUBTITLE_SPACING
 		});
 	}
+
+	if (reel_overlap) {
+		notes.push_back ({
+			VerificationNote::Type::ERROR, VerificationNote::Code::SUBTITLE_OVERLAPS_REEL_BOUNDARY
+		});
+	}
 }
 
 
@@ -948,13 +981,8 @@ verify_text_timing (vector<shared_ptr<Reel>> reels, vector<VerificationNote>& no
 		return;
 	}
 
-	optional<int> picture_frame_rate;
-	if (reels[0]->main_picture()) {
-		picture_frame_rate = reels[0]->main_picture()->frame_rate().numerator;
-	}
-
 	if (reels[0]->main_subtitle()) {
-		verify_text_timing (reels, picture_frame_rate, notes,
+		verify_text_timing (reels, reels[0]->main_subtitle()->edit_rate().numerator, notes,
 			[](shared_ptr<Reel> reel) {
 				return static_cast<bool>(reel->main_subtitle());
 			},
@@ -968,7 +996,7 @@ verify_text_timing (vector<shared_ptr<Reel>> reels, vector<VerificationNote>& no
 	}
 
 	for (auto i = 0U; i < reels[0]->closed_captions().size(); ++i) {
-		verify_text_timing (reels, picture_frame_rate, notes,
+		verify_text_timing (reels, reels[0]->closed_captions()[i]->edit_rate().numerator, notes,
 			[i](shared_ptr<Reel> reel) {
 				return i < reel->closed_captions().size();
 			},
@@ -1262,6 +1290,8 @@ dcp::verify (
 				most_closed_captions = std::max (most_closed_captions, reel->closed_captions().size());
 			}
 
+			verify_text_timing (cpl->reels(), notes);
+
 			if (dcp->standard() == Standard::SMPTE) {
 
 				if (have_main_subtitle && have_no_main_subtitle) {
@@ -1292,14 +1322,12 @@ dcp::verify (
 				if (lfoc == markers_seen.end()) {
 					notes.push_back ({VerificationNote::Type::WARNING, VerificationNote::Code::MISSING_LFOC});
 				} else {
-					auto lfoc_time = lfoc->second.as_editable_units(lfoc->second.tcr);
+					auto lfoc_time = lfoc->second.as_editable_units_ceil(lfoc->second.tcr);
 					if (lfoc_time != (cpl->reels().back()->duration() - 1)) {
 						notes.push_back ({VerificationNote::Type::WARNING, VerificationNote::Code::INCORRECT_LFOC, raw_convert<string>(lfoc_time)});
 					}
 				}
 
-				verify_text_timing (cpl->reels(), notes);
-
 				LinesCharactersResult result;
 				for (auto reel: cpl->reels()) {
 					if (reel->main_subtitle() && reel->main_subtitle()->asset()) {
@@ -1458,6 +1486,8 @@ dcp::note_to_string (VerificationNote note)
 		return "At least one subtitle lasts less than 15 frames.";
 	case VerificationNote::Code::INVALID_SUBTITLE_SPACING:
 		return "At least one pair of subtitles is separated by less than 2 frames.";
+	case VerificationNote::Code::SUBTITLE_OVERLAPS_REEL_BOUNDARY:
+		return "At least one subtitle extends outside of its reel.";
 	case VerificationNote::Code::INVALID_SUBTITLE_LINE_COUNT:
 		return "There are more than 3 subtitle lines in at least one place in the DCP.";
 	case VerificationNote::Code::NEARLY_INVALID_SUBTITLE_LINE_LENGTH:
diff --git a/src/verify.h b/src/verify.h
index f96fd4d2..16700127 100644
--- a/src/verify.h
+++ b/src/verify.h
@@ -214,6 +214,8 @@ public:
 		INVALID_SUBTITLE_DURATION,
 		/** 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 */
+		SUBTITLE_OVERLAPS_REEL_BOUNDARY,
 		/** There are more than 3 subtitle lines in at least one place [Bv2.1_7.2.7] */
 		INVALID_SUBTITLE_LINE_COUNT,
 		/** There are more than 52 characters in at least one subtitle line [Bv2.1_7.2.7] */