From cadca6e4f8c1d844f1b5fb9375023e627c674fa9 Mon Sep 17 00:00:00 2001 From: Carl Hetherington Date: Mon, 7 Dec 2020 01:18:38 +0100 Subject: [PATCH] Write subtitles and closed captions to a test DCP in the hints thread, then check the result for Bv2.1 violations (part of #1800). --- src/lib/hints.cc | 68 ++++++++++++++++++++++++++++++++-- src/lib/hints.h | 10 ++++- src/lib/reel_writer.cc | 36 +++++++++++++----- src/lib/reel_writer.h | 4 +- src/lib/util.h | 9 +++++ src/lib/writer.cc | 28 ++++++++------ src/lib/writer.h | 4 +- test/data | 2 +- test/hints_test.cc | 80 ++++++++++++++++++++++++++++++++++++---- test/reel_writer_test.cc | 2 +- 10 files changed, 205 insertions(+), 38 deletions(-) diff --git a/src/lib/hints.cc b/src/lib/hints.cc index 5c9d3d8a4..131548035 100644 --- a/src/lib/hints.cc +++ b/src/lib/hints.cc @@ -33,7 +33,12 @@ #include "util.h" #include "cross.h" #include "player.h" +#include "writer.h" +#include #include +#include +#include +#include #include #include #include @@ -55,8 +60,16 @@ using namespace dcpomatic; using namespace boost::placeholders; #endif + +/* When checking to see if things are too big, we'll say they are if they + * are more than the target size minus this "slack." + */ +#define SIZE_SLACK 4096 + + Hints::Hints (weak_ptr film) : WeakConstFilm (film) + , _writer (new Writer(film, weak_ptr(), true)) , _long_ccap (false) , _overlap_ccap (false) , _too_many_ccap_lines (false) @@ -226,7 +239,7 @@ Hints::check_big_font_files () BOOST_FOREACH (shared_ptr j, i->text) { BOOST_FOREACH (shared_ptr k, j->fonts()) { optional const p = k->file (); - if (p && boost::filesystem::file_size(p.get()) >= (640 * 1024)) { + if (p && boost::filesystem::file_size(p.get()) >= (MAX_FONT_FILE_SIZE - SIZE_SLACK)) { big_font_files = true; } } @@ -311,6 +324,14 @@ Hints::check_loudness () } +static +bool +subtitle_mxf_too_big (shared_ptr asset) +{ + return asset && asset->file() && boost::filesystem::file_size(*asset->file()) >= (MAX_TEXT_MXF_SIZE - SIZE_SLACK); +} + + void Hints::thread () { @@ -339,7 +360,7 @@ Hints::thread () shared_ptr player (new Player(film)); player->set_ignore_video (); player->set_ignore_audio (); - player->Text.connect (bind(&Hints::text, this, _1, _2, _4)); + player->Text.connect (bind(&Hints::text, this, _1, _2, _3, _4)); struct timeval last_pulse; gettimeofday (&last_pulse, 0); @@ -361,6 +382,45 @@ Hints::thread () store_current (); } + _writer->write (player->get_subtitle_fonts()); + + bool ccap_xml_too_big = false; + bool ccap_mxf_too_big = false; + bool subs_mxf_too_big = false; + + boost::filesystem::path dcp_dir = film->dir("hints") / dcpomatic::get_process_id(); + boost::filesystem::remove_all (dcp_dir); + _writer->finish (film->dir("hints") / dcpomatic::get_process_id()); + dcp::DCP dcp (dcp_dir); + dcp.read (); + DCPOMATIC_ASSERT (dcp.cpls().size() == 1); + BOOST_FOREACH (shared_ptr reel, dcp.cpls().front()->reels()) { + BOOST_FOREACH (shared_ptr ccap, reel->closed_captions()) { + if (ccap->asset() && ccap->asset()->xml_as_string().length() > static_cast(MAX_CLOSED_CAPTION_XML_SIZE - SIZE_SLACK) && !ccap_xml_too_big) { + hint (_( + "At least one of your closed caption files' XML part is larger than " MAX_CLOSED_CAPTION_XML_SIZE_TEXT + ". You should divide the DCP into shorter reels." + )); + ccap_xml_too_big = true; + } + if (subtitle_mxf_too_big(ccap->asset()) && !ccap_mxf_too_big) { + hint (_( + "At least one of your closed caption files is larger than " MAX_TEXT_MXF_SIZE_TEXT + " in total. You should divide the DCP into shorter reels." + )); + ccap_mxf_too_big = true; + } + } + if (reel->main_subtitle() && subtitle_mxf_too_big(reel->main_subtitle()->asset()) && !subs_mxf_too_big) { + hint (_( + "At least one of your subtitle files is larger than " MAX_TEXT_MXF_SIZE_TEXT " in total. " + "You should divide the DCP into shorter reels." + )); + subs_mxf_too_big = true; + } + } + boost::filesystem::remove_all (dcp_dir); + emit (bind(boost::ref(Finished))); } @@ -371,8 +431,10 @@ Hints::hint (string h) } void -Hints::text (PlayerText text, TextType type, DCPTimePeriod period) +Hints::text (PlayerText text, TextType type, optional track, DCPTimePeriod period) { + _writer->write (text, type, track, period); + switch (type) { case TEXT_CLOSED_CAPTION: closed_caption (text, period); diff --git a/src/lib/hints.h b/src/lib/hints.h index b8a831301..9164e2106 100644 --- a/src/lib/hints.h +++ b/src/lib/hints.h @@ -30,7 +30,10 @@ #include #include + class Film; +class Writer; + class Hints : public Signaller, public ExceptionStore, public WeakConstFilm { @@ -53,7 +56,7 @@ private: void thread (); void hint (std::string h); - void text (PlayerText text, TextType type, dcpomatic::DCPTimePeriod period); + void text (PlayerText text, TextType type, boost::optional track, dcpomatic::DCPTimePeriod period); void closed_caption (PlayerText text, dcpomatic::DCPTimePeriod period); void open_subtitle (PlayerText text, dcpomatic::DCPTimePeriod period); @@ -71,6 +74,11 @@ private: void check_ffec_and_ffmc_in_smpte_feature (); boost::thread _thread; + /** This is used to make a partial DCP containing only the subtitles and closed captions that + * our final DCP will have. This means we can see how big the files will be and warn if they + * will be too big. + */ + boost::shared_ptr _writer; bool _long_ccap; bool _overlap_ccap; diff --git a/src/lib/reel_writer.cc b/src/lib/reel_writer.cc index e12628c74..8e4370526 100644 --- a/src/lib/reel_writer.cc +++ b/src/lib/reel_writer.cc @@ -92,9 +92,13 @@ mxf_metadata () return meta; } -/** @param job Related job, or 0 */ +/** @param job Related job, or 0. + * @param text_only true to enable a special mode where the writer will expect only subtitles and closed captions to be written + * (no picture nor sound) and not give errors in that case. This is used by the hints system to check the potential sizes of + * subtitle / closed caption files. + */ ReelWriter::ReelWriter ( - weak_ptr weak_film, DCPTimePeriod period, shared_ptr job, int reel_index, int reel_count + weak_ptr weak_film, DCPTimePeriod period, shared_ptr job, int reel_index, int reel_count, bool text_only ) : WeakConstFilm (weak_film) , _period (period) @@ -102,6 +106,7 @@ ReelWriter::ReelWriter ( , _reel_count (reel_count) , _content_summary (film()->content_summary(period)) , _job (job) + , _text_only (text_only) { /* Create or find our picture asset in a subdirectory, named according to those film's parameters which affect the video @@ -441,7 +446,8 @@ maybe_add_text ( list > const & fonts, shared_ptr film, DCPTimePeriod period, - boost::filesystem::path output_dcp + boost::filesystem::path output_dcp, + bool text_only ) { Frame const period_duration = period.duration().frames_round(film->video_frame_rate()); @@ -494,7 +500,7 @@ maybe_add_text ( } if (reel_asset) { - if (reel_asset->actual_duration() != period_duration) { + if (!text_only && reel_asset->actual_duration() != period_duration) { throw ProgrammingError ( __FILE__, __LINE__, String::compose ("%1 vs %2", reel_asset->actual_duration(), period_duration) @@ -614,7 +620,7 @@ ReelWriter::create_reel_text ( ) const { shared_ptr subtitle = maybe_add_text ( - _subtitle_asset, duration, reel, refs, fonts, film(), _period, output_dcp + _subtitle_asset, duration, reel, refs, fonts, film(), _period, output_dcp, _text_only ); if (subtitle && !film()->subtitle_languages().empty()) { subtitle->set_language (film()->subtitle_languages().front()); @@ -622,7 +628,7 @@ ReelWriter::create_reel_text ( for (map >::const_iterator i = _closed_caption_assets.begin(); i != _closed_caption_assets.end(); ++i) { shared_ptr a = maybe_add_text ( - i->second, duration, reel, refs, fonts, film(), _period, output_dcp + i->second, duration, reel, refs, fonts, film(), _period, output_dcp, _text_only ); if (a) { a->set_annotation_text (i->first.name); @@ -666,10 +672,20 @@ ReelWriter::create_reel (list const & refs, list reel (new dcp::Reel()); - shared_ptr reel_picture_asset = create_reel_picture (reel, refs); - create_reel_sound (reel, refs); - create_reel_text (reel, refs, fonts, reel_picture_asset->actual_duration(), output_dcp); - create_reel_markers (reel); + + /* This is a bit of a hack; in the strange `_text_only' mode we have no picture, so we don't know + * how long the subtitle / CCAP assets should be. However, since we're only writing them to see + * how big they are, we don't care about that. + */ + int64_t duration = 0; + if (!_text_only) { + shared_ptr reel_picture_asset = create_reel_picture (reel, refs); + duration = reel_picture_asset->actual_duration (); + create_reel_sound (reel, refs); + create_reel_markers (reel); + } + + create_reel_text (reel, refs, fonts, duration, output_dcp); if (_atmos_asset) { reel->add (shared_ptr(new dcp::ReelAtmosAsset(_atmos_asset, 0))); diff --git a/src/lib/reel_writer.h b/src/lib/reel_writer.h index c65364567..6237c2943 100644 --- a/src/lib/reel_writer.h +++ b/src/lib/reel_writer.h @@ -64,7 +64,8 @@ public: dcpomatic::DCPTimePeriod period, boost::shared_ptr job, int reel_index, - int reel_count + int reel_count, + bool text_only ); void write (boost::shared_ptr encoded, Frame frame, Eyes eyes); @@ -122,6 +123,7 @@ private: int _reel_count; boost::optional _content_summary; boost::weak_ptr _job; + bool _text_only; boost::shared_ptr _picture_asset; /** picture asset writer, or 0 if we are not writing any picture because we already have one */ diff --git a/src/lib/util.h b/src/lib/util.h index 548dd0475..e23eff07c 100644 --- a/src/lib/util.h +++ b/src/lib/util.h @@ -65,6 +65,15 @@ namespace dcp { #define MAX_CLOSED_CAPTION_LINES 3 /** Maximum line length of closed caption viewers, according to SMPTE Bv2.1 */ #define MAX_CLOSED_CAPTION_LENGTH 32 +/** Maximum size of a subtitle / closed caption MXF in bytes, according to SMPTE Bv2.1 */ +#define MAX_TEXT_MXF_SIZE (115 * 1024 * 1024) +#define MAX_TEXT_MXF_SIZE_TEXT "115MB" +/** Maximum size of a font file, in bytes */ +#define MAX_FONT_FILE_SIZE (640 * 1024) +#define MAX_FONT_FILE_SIZE_TEXT "640KB" +/** Maximum size of the XML part of a closed caption file, according to SMPTE Bv2.1 */ +#define MAX_CLOSED_CAPTION_XML_SIZE (256 * 1024) +#define MAX_CLOSED_CAPTION_XML_SIZE_TEXT "256KB" extern std::string program_name; extern bool is_batch_converter; diff --git a/src/lib/writer.cc b/src/lib/writer.cc index 17184918f..0b85a7f32 100644 --- a/src/lib/writer.cc +++ b/src/lib/writer.cc @@ -79,8 +79,10 @@ ignore_progress (float) } -/** @param j Job to report progress to, or 0 */ -Writer::Writer (weak_ptr weak_film, weak_ptr j) +/** @param j Job to report progress to, or 0. + * @param text_only true to enable only the text (subtitle/ccap) parts of the writer. + */ +Writer::Writer (weak_ptr weak_film, weak_ptr j, bool text_only) : WeakConstFilm (weak_film) , _job (j) , _finish (false) @@ -92,13 +94,14 @@ Writer::Writer (weak_ptr weak_film, weak_ptr j) , _fake_written (0) , _repeat_written (0) , _pushed_to_disk (0) + , _text_only (text_only) { shared_ptr job = _job.lock (); int reel_index = 0; list const reels = film()->reels(); BOOST_FOREACH (DCPTimePeriod p, reels) { - _reels.push_back (ReelWriter(weak_film, p, job, reel_index++, reels.size())); + _reels.push_back (ReelWriter(weak_film, p, job, reel_index++, reels.size(), text_only)); } _last_written.resize (reels.size()); @@ -123,15 +126,19 @@ Writer::Writer (weak_ptr weak_film, weak_ptr j) void Writer::start () { - _thread = boost::thread (boost::bind(&Writer::thread, this)); + if (!_text_only) { + _thread = boost::thread (boost::bind(&Writer::thread, this)); #ifdef DCPOMATIC_LINUX - pthread_setname_np (_thread.native_handle(), "writer"); + pthread_setname_np (_thread.native_handle(), "writer"); #endif + } } Writer::~Writer () { - terminate_thread (false); + if (!_text_only) { + terminate_thread (false); + } } /** Pass a video frame to the writer for writing to disk at some point. @@ -522,14 +529,11 @@ Writer::terminate_thread (bool can_throw) void Writer::finish (boost::filesystem::path output_dcp) { - if (!_thread.joinable()) { - return; + if (_thread.joinable()) { + LOG_GENERAL_NC ("Terminating writer thread"); + terminate_thread (true); } - LOG_GENERAL_NC ("Terminating writer thread"); - - terminate_thread (true); - LOG_GENERAL_NC ("Finishing ReelWriters"); BOOST_FOREACH (ReelWriter& i, _reels) { diff --git a/src/lib/writer.h b/src/lib/writer.h index 1c290e6ca..3b5cc3dc3 100644 --- a/src/lib/writer.h +++ b/src/lib/writer.h @@ -99,7 +99,7 @@ bool operator== (QueueItem const & a, QueueItem const & b); class Writer : public ExceptionStore, public boost::noncopyable, public WeakConstFilm { public: - Writer (boost::weak_ptr, boost::weak_ptr); + Writer (boost::weak_ptr, boost::weak_ptr, bool text_only = false); ~Writer (); void start (); @@ -189,6 +189,8 @@ private: */ int _pushed_to_disk; + bool _text_only; + boost::mutex _digest_progresses_mutex; std::map _digest_progresses; diff --git a/test/data b/test/data index 66e5e6e6a..578c16ee9 160000 --- a/test/data +++ b/test/data @@ -1 +1 @@ -Subproject commit 66e5e6e6a7bd9fe817a00011c1c1c32c6f4e01c7 +Subproject commit 578c16ee95b416d4dcec31c72219dac91e632169 diff --git a/test/hints_test.cc b/test/hints_test.cc index 0e47125d0..aabcaaeac 100644 --- a/test/hints_test.cc +++ b/test/hints_test.cc @@ -21,7 +21,9 @@ #include "lib/content.h" #include "lib/content_factory.h" +#include "lib/cross.h" #include "lib/film.h" +#include "lib/font.h" #include "lib/hints.h" #include "lib/text_content.h" #include "lib/util.h" @@ -32,6 +34,7 @@ using std::string; using std::vector; +using boost::optional; using boost::shared_ptr; @@ -62,7 +65,7 @@ get_hints (shared_ptr film) static void -check (TextType type, string name, string expected_hint) +check (TextType type, string name, optional expected_hint = optional()) { shared_ptr film = new_test_film2 (name); shared_ptr content = content_factory("test/data/" + name + ".srt").front(); @@ -71,8 +74,12 @@ check (TextType type, string name, string expected_hint) BOOST_REQUIRE (!wait_for_jobs()); vector hints = get_hints (film); - BOOST_REQUIRE_EQUAL (hints.size(), 1); - BOOST_CHECK_EQUAL (hints[0], expected_hint); + if (expected_hint) { + BOOST_REQUIRE_EQUAL (hints.size(), 1); + BOOST_CHECK_EQUAL (hints[0], *expected_hint); + } else { + BOOST_CHECK (hints.empty()); + } } @@ -101,7 +108,7 @@ BOOST_AUTO_TEST_CASE (hint_subtitle_too_early) check ( TEXT_OPEN_SUBTITLE, "hint_subtitle_too_early", - "It is advisable to put your first subtitle at least 4 seconds after the start of the DCP to make sure it is seen." + string("It is advisable to put your first subtitle at least 4 seconds after the start of the DCP to make sure it is seen.") ); } @@ -111,7 +118,7 @@ BOOST_AUTO_TEST_CASE (hint_short_subtitles) check ( TEXT_OPEN_SUBTITLE, "hint_short_subtitles", - "At least one of your subtitles lasts less than 15 frames. It is advisable to make each subtitle at least 15 frames long." + string("At least one of your subtitles lasts less than 15 frames. It is advisable to make each subtitle at least 15 frames long.") ); } @@ -121,7 +128,7 @@ BOOST_AUTO_TEST_CASE (hint_subtitles_too_close) check ( TEXT_OPEN_SUBTITLE, "hint_subtitles_too_close", - "At least one of your subtitles starts less than 2 frames after the previous one. It is advisable to make the gap between subtitles at least 2 frames." + string("At least one of your subtitles starts less than 2 frames after the previous one. It is advisable to make the gap between subtitles at least 2 frames.") ); } @@ -131,7 +138,7 @@ BOOST_AUTO_TEST_CASE (hint_many_subtitle_lines) check ( TEXT_OPEN_SUBTITLE, "hint_many_subtitle_lines", - "At least one of your subtitles has more than 3 lines. It is advisable to use no more than 3 lines." + string("At least one of your subtitles has more than 3 lines. It is advisable to use no more than 3 lines.") ); } @@ -141,7 +148,64 @@ BOOST_AUTO_TEST_CASE (hint_subtitle_too_long) check ( TEXT_OPEN_SUBTITLE, "hint_subtitle_too_long", - "At least one of your subtitle lines has more than 52 characters. It is advisable to make each line 52 characters at most in length." + string("At least one of your subtitle lines has more than 52 characters. It is advisable to make each line 52 characters at most in length.") ); } + +BOOST_AUTO_TEST_CASE (hint_subtitle_mxf_too_big) +{ + string const name = "hint_subtitle_mxf_too_big"; + + shared_ptr film = new_test_film2 (name); + shared_ptr content = content_factory("test/data/" + name + ".srt").front(); + content->text.front()->set_type (TEXT_OPEN_SUBTITLE); + for (int i = 1; i < 512; ++i) { + shared_ptr font(new dcpomatic::Font(String::compose("font_%1", i))); + font->set_file ("test/data/LiberationSans-Regular.ttf"); + content->text.front()->add_font(font); + } + film->examine_and_add_content (content); + BOOST_REQUIRE (!wait_for_jobs()); + vector hints = get_hints (film); + + BOOST_REQUIRE_EQUAL (hints.size(), 1); + BOOST_CHECK_EQUAL ( + hints[0], + "At least one of your subtitle files is larger than " MAX_TEXT_MXF_SIZE_TEXT " in total. " + "You should divide the DCP into shorter reels." + ); +} + + +BOOST_AUTO_TEST_CASE (hint_closed_caption_xml_too_big) +{ + string const name = "hint_closed_caption_xml_too_big"; + + shared_ptr film = new_test_film2 (name); + + FILE* ccap = fopen_boost (String::compose("build/test/%1.srt", name), "w"); + BOOST_REQUIRE (ccap); + for (int i = 0; i < 2048; ++i) { + fprintf(ccap, "%d\n", i + 1); + int second = i * 2; + int minute = second % 60; + fprintf(ccap, "00:%02d:%02d,000 --> 00:%02d:%02d,000\n", minute, second, minute, second + 1); + fprintf(ccap, "Here are some closed captions.\n\n"); + } + fclose (ccap); + + shared_ptr content = content_factory("build/test/" + name + ".srt").front(); + content->text.front()->set_type (TEXT_CLOSED_CAPTION); + film->examine_and_add_content (content); + BOOST_REQUIRE (!wait_for_jobs()); + vector hints = get_hints (film); + + BOOST_REQUIRE_EQUAL (hints.size(), 1); + BOOST_CHECK_EQUAL ( + hints[0], + "At least one of your closed caption files' XML part is larger than " MAX_CLOSED_CAPTION_XML_SIZE_TEXT ". " + "You should divide the DCP into shorter reels." + ); +} + diff --git a/test/reel_writer_test.cc b/test/reel_writer_test.cc index 21e924380..a5a3ed83b 100644 --- a/test/reel_writer_test.cc +++ b/test/reel_writer_test.cc @@ -52,7 +52,7 @@ BOOST_AUTO_TEST_CASE (write_frame_info_test) { shared_ptr film = new_test_film2 ("write_frame_info_test"); dcpomatic::DCPTimePeriod const period (dcpomatic::DCPTime(0), dcpomatic::DCPTime(96000)); - ReelWriter writer (film, period, shared_ptr(), 0, 1); + ReelWriter writer (film, period, shared_ptr(), 0, 1, false); /* Write the first one */ -- 2.30.2