Report every frame (with index) that is over (or nearly over) the size limit (DoM #2698).v1.8.91

2 files changed, 60 insertions, 27 deletions

diff --git a/src/verify.cc b/src/verify.cc
index 5901ffde..f20c046d 100644
--- a/src/verify.cc
+++ b/src/verify.cc
@@ -440,7 +440,6 @@ verify_language_tag (string tag, vector<VerificationNote>& notes)
 static void
 verify_picture_asset(shared_ptr<const ReelFileAsset> reel_file_asset, boost::filesystem::path file, int64_t start_frame, vector<VerificationNote>& notes, function<void (float)> progress)
 {
-	int biggest_frame = 0;
 	auto asset = dynamic_pointer_cast<PictureAsset>(reel_file_asset->asset_ref().asset());
 	auto const duration = asset->intrinsic_duration ();
 
@@ -452,11 +451,30 @@ verify_picture_asset(shared_ptr<const ReelFileAsset> reel_file_asset, boost::fil
 		}
 	};
 
+	int const max_frame =   rint(250 * 1000000 / (8 * asset->edit_rate().as_float()));
+	int const risky_frame = rint(230 * 1000000 / (8 * asset->edit_rate().as_float()));
+
+	auto check_frame_size = [max_frame, risky_frame, file, start_frame](int index, int size, int frame_rate, vector<VerificationNote>& notes) {
+		if (size > max_frame) {
+			notes.push_back(
+				VerificationNote(
+					VerificationNote::Type::ERROR, VerificationNote::Code::INVALID_PICTURE_FRAME_SIZE_IN_BYTES, file
+					).set_frame(start_frame + index).set_frame_rate(frame_rate)
+			);
+		} else if (size > risky_frame) {
+			notes.push_back(
+				VerificationNote(
+					VerificationNote::Type::WARNING, VerificationNote::Code::NEARLY_INVALID_PICTURE_FRAME_SIZE_IN_BYTES, file
+					).set_frame(start_frame + index).set_frame_rate(frame_rate)
+			);
+		}
+	};
+
 	if (auto mono_asset = dynamic_pointer_cast<MonoPictureAsset>(reel_file_asset->asset_ref().asset())) {
 		auto reader = mono_asset->start_read ();
 		for (int64_t i = 0; i < duration; ++i) {
 			auto frame = reader->get_frame (i);
-			biggest_frame = max(biggest_frame, frame->size());
+			check_frame_size(i, frame->size(), mono_asset->frame_rate().numerator, notes);
 			if (!mono_asset->encrypted() || mono_asset->key()) {
 				vector<VerificationNote> j2k_notes;
 				verify_j2k(frame, start_frame, i, mono_asset->frame_rate().numerator, j2k_notes);
@@ -468,7 +486,8 @@ verify_picture_asset(shared_ptr<const ReelFileAsset> reel_file_asset, boost::fil
 		auto reader = stereo_asset->start_read ();
 		for (int64_t i = 0; i < duration; ++i) {
 			auto frame = reader->get_frame (i);
-			biggest_frame = max(biggest_frame, max(frame->left()->size(), frame->right()->size()));
+			check_frame_size(i, frame->left()->size(), stereo_asset->frame_rate().numerator, notes);
+			check_frame_size(i, frame->right()->size(), stereo_asset->frame_rate().numerator, notes);
 			if (!stereo_asset->encrypted() || stereo_asset->key()) {
 				vector<VerificationNote> j2k_notes;
 				verify_j2k(frame->left(), start_frame, i, stereo_asset->frame_rate().numerator, j2k_notes);
@@ -479,18 +498,6 @@ verify_picture_asset(shared_ptr<const ReelFileAsset> reel_file_asset, boost::fil
 		}
 
 	}
-
-	static const int max_frame =   rint(250 * 1000000 / (8 * asset->edit_rate().as_float()));
-	static const int risky_frame = rint(230 * 1000000 / (8 * asset->edit_rate().as_float()));
-	if (biggest_frame > max_frame) {
-		notes.push_back ({
-			VerificationNote::Type::ERROR, VerificationNote::Code::INVALID_PICTURE_FRAME_SIZE_IN_BYTES, file
-		});
-	} else if (biggest_frame > risky_frame) {
-		notes.push_back ({
-			VerificationNote::Type::WARNING, VerificationNote::Code::NEARLY_INVALID_PICTURE_FRAME_SIZE_IN_BYTES, file
-		});
-	}
 }
 
 
@@ -1937,9 +1944,19 @@ dcp::note_to_string (VerificationNote note)
 	case VerificationNote::Code::INVALID_DURATION:
 		return String::compose("The duration of the asset %1 is less than 1 second.", note.note().get());
 	case VerificationNote::Code::INVALID_PICTURE_FRAME_SIZE_IN_BYTES:
-		return String::compose("The instantaneous bit rate of the picture asset %1 is larger than the limit of 250Mbit/s in at least one place.", note.file()->filename());
+		return String::compose(
+			"Frame %1 (timecode %2) in asset %3 has an instantaneous bit rate that is larger than the limit of 250Mbit/s.",
+			note.frame().get(),
+			dcp::Time(note.frame().get(), note.frame_rate().get(), note.frame_rate().get()).as_string(dcp::Standard::SMPTE),
+			note.file()->filename()
+			);
 	case VerificationNote::Code::NEARLY_INVALID_PICTURE_FRAME_SIZE_IN_BYTES:
-		return String::compose("The instantaneous bit rate of the picture asset %1 is close to the limit of 250Mbit/s in at least one place.", note.file()->filename());
+		return String::compose(
+			"Frame %1 (timecode %2) in asset %3 has an instantaneous bit rate that is close to the limit of 250Mbit/s.",
+			note.frame().get(),
+			dcp::Time(note.frame().get(), note.frame_rate().get(), note.frame_rate().get()).as_string(dcp::Standard::SMPTE),
+			note.file()->filename()
+			);
 	case VerificationNote::Code::EXTERNAL_ASSET:
 		return String::compose("The asset %1 that this DCP refers to is not included in the DCP.  It may be a VF.", note.note().get());
 	case VerificationNote::Code::THREED_ASSET_MARKED_AS_TWOD:
@@ -2187,7 +2204,11 @@ dcp::operator< (dcp::VerificationNote const& a, dcp::VerificationNote const& b)
 		return a.id().get_value_or("") < b.id().get_value_or("");
 	}
 
-	return a.other_id().get_value_or("") < b.other_id().get_value_or("");
+	if (a.other_id() != b.other_id()) {
+		return a.other_id().get_value_or("") < b.other_id().get_value_or("");
+	}
+
+	return a.frame_rate().get_value_or(0) != b.frame_rate().get_value_or(0);
 }
 
 
diff --git a/test/verify_test.cc b/test/verify_test.cc
index c382b6d1..0e15624d 100644
--- a/test/verify_test.cc
+++ b/test/verify_test.cc
@@ -644,9 +644,13 @@ BOOST_AUTO_TEST_CASE (verify_invalid_picture_frame_size_in_bytes)
 			);
 	}
 
-	expected.push_back(
-		{ dcp::VerificationNote::Type::ERROR, dcp::VerificationNote::Code::INVALID_PICTURE_FRAME_SIZE_IN_BYTES, canonical(dir / "pic.mxf") }
-	);
+	for (auto i = 0; i < 24; ++i) {
+		expected.push_back(
+			dcp::VerificationNote(
+				dcp::VerificationNote::Type::ERROR, dcp::VerificationNote::Code::INVALID_PICTURE_FRAME_SIZE_IN_BYTES, canonical(dir / "pic.mxf")
+				).set_frame(i).set_frame_rate(24)
+			);
+	}
 
 	expected.push_back(
 		{ dcp::VerificationNote::Type::BV21_ERROR, dcp::VerificationNote::Code::MISSING_CPL_METADATA, cpl->id(), cpl->file().get() }
@@ -684,9 +688,13 @@ BOOST_AUTO_TEST_CASE (verify_nearly_invalid_picture_frame_size_in_bytes)
 			);
 	}
 
-	expected.push_back(
-		{ dcp::VerificationNote::Type::WARNING, dcp::VerificationNote::Code::NEARLY_INVALID_PICTURE_FRAME_SIZE_IN_BYTES, canonical(dir / "pic.mxf") }
-	);
+	for (auto i = 0; i < 24; ++i) {
+		expected.push_back(
+			dcp::VerificationNote(
+				dcp::VerificationNote::Type::WARNING, dcp::VerificationNote::Code::NEARLY_INVALID_PICTURE_FRAME_SIZE_IN_BYTES, canonical(dir / "pic.mxf")
+				).set_frame(i).set_frame_rate(24)
+		);
+	}
 
 	expected.push_back(
 		{ dcp::VerificationNote::Type::BV21_ERROR, dcp::VerificationNote::Code::MISSING_CPL_METADATA, cpl->id(), cpl->file().get() }
@@ -3668,9 +3676,13 @@ BOOST_AUTO_TEST_CASE(verify_invalid_tile_part_size)
 
 	vector<dcp::VerificationNote> expected;
 
-	expected.push_back(
-		{ dcp::VerificationNote::Type::ERROR, dcp::VerificationNote::Code::INVALID_PICTURE_FRAME_SIZE_IN_BYTES, canonical(path / "video.mxf") }
-	);
+	for (auto frame = 0; frame < 24; frame++) {
+		expected.push_back(
+			dcp::VerificationNote(
+				dcp::VerificationNote::Type::ERROR, dcp::VerificationNote::Code::INVALID_PICTURE_FRAME_SIZE_IN_BYTES, canonical(path / "video.mxf")
+			).set_frame(frame).set_frame_rate(24)
+		);
+	}
 
 	int component_sizes[] = {
 		1321721,