From: Carl Hetherington Date: Wed, 8 Jun 2022 07:58:16 +0000 (+0200) Subject: Set up TextDecoder position based on the time that the next thing will X-Git-Tag: v2.16.14 X-Git-Url: https://git.carlh.net/gitweb/?p=dcpomatic.git;a=commitdiff_plain;h=c8a036eb727ceddc64a0304d781c916eb952c001 Set up TextDecoder position based on the time that the next thing will be emitted, instead of the time that the last thing was (#2268). This is to avoid problems with the example shown in the test, where just because a subtitle in source A comes before a subtitle in source B, source A is pass()ed next and may then emit a subtitle which should be after the next one in B. --- diff --git a/src/lib/dcp_decoder.cc b/src/lib/dcp_decoder.cc index 636cc8ed2..ad7d3e112 100644 --- a/src/lib/dcp_decoder.cc +++ b/src/lib/dcp_decoder.cc @@ -80,8 +80,11 @@ DCPDecoder::DCPDecoder (shared_ptr film, shared_ptr(this, content->audio, fast); } for (auto i: content->text) { - /* XXX: this time here should be the time of the first subtitle, not 0 */ - text.push_back (make_shared(this, i, ContentTime())); + text.push_back (make_shared(this, i)); + /* We should really call maybe_set_position() on this TextDecoder to set the time + * of the first subtitle, but it probably doesn't matter since we'll always + * have regularly occurring video (and maybe audio) content. + */ } if (content->atmos) { atmos = make_shared(this, content); diff --git a/src/lib/dcp_subtitle_decoder.cc b/src/lib/dcp_subtitle_decoder.cc index 83faa1e93..4743ecf42 100644 --- a/src/lib/dcp_subtitle_decoder.cc +++ b/src/lib/dcp_subtitle_decoder.cc @@ -46,11 +46,8 @@ DCPSubtitleDecoder::DCPSubtitleDecoder (shared_ptr film, shared_ptr< _subtitles = c->subtitles (); _next = _subtitles.begin (); - ContentTime first; - if (_next != _subtitles.end()) { - first = content_time_period(*_next).from; - } - text.push_back (make_shared(this, content->only_text(), first)); + text.push_back (make_shared(this, content->only_text())); + update_position(); } @@ -64,6 +61,8 @@ DCPSubtitleDecoder::seek (ContentTime time, bool accurate) while (i != _subtitles.end() && ContentTime::from_seconds ((*_next)->in().as_seconds()) < time) { ++i; } + + update_position(); } @@ -104,6 +103,9 @@ DCPSubtitleDecoder::pass () } only_text()->emit_plain (p, s); + + update_position(); + return false; } @@ -129,3 +131,14 @@ DCPSubtitleDecoder::first () const return ContentTime::from_seconds(_subtitles[0]->in().as_seconds()); } + +void +DCPSubtitleDecoder::update_position() +{ + if (_next != _subtitles.end()) { + only_text()->maybe_set_position( + ContentTime::from_seconds((*_next)->in().as_seconds()) + ); + } +} + diff --git a/src/lib/dcp_subtitle_decoder.h b/src/lib/dcp_subtitle_decoder.h index 4f5964c6f..16f89b530 100644 --- a/src/lib/dcp_subtitle_decoder.h +++ b/src/lib/dcp_subtitle_decoder.h @@ -38,6 +38,7 @@ public: private: dcpomatic::ContentTimePeriod content_time_period (std::shared_ptr s) const; + void update_position(); std::vector> _subtitles; std::vector>::const_iterator _next; diff --git a/src/lib/ffmpeg_decoder.cc b/src/lib/ffmpeg_decoder.cc index bc7d28d7d..82eccb576 100644 --- a/src/lib/ffmpeg_decoder.cc +++ b/src/lib/ffmpeg_decoder.cc @@ -93,8 +93,10 @@ FFmpegDecoder::FFmpegDecoder (shared_ptr film, shared_ptronly_text()) { - /* XXX: this time here should be the time of the first subtitle, not 0 */ - text.push_back (make_shared(this, c->only_text(), ContentTime())); + text.push_back (make_shared(this, c->only_text())); + /* XXX: we should be calling maybe_set_position() on this TextDecoder, but we can't easily find + * the time of the first subtitle at this point. + */ } for (auto i: c->ffmpeg_audio_streams()) { diff --git a/src/lib/string_text_file_decoder.cc b/src/lib/string_text_file_decoder.cc index d5f320a27..2ec0ec1cb 100644 --- a/src/lib/string_text_file_decoder.cc +++ b/src/lib/string_text_file_decoder.cc @@ -40,11 +40,8 @@ StringTextFileDecoder::StringTextFileDecoder (shared_ptr film, share , StringTextFile (content) , _next (0) { - ContentTime first; - if (!_subtitles.empty()) { - first = content_time_period(_subtitles[0]).from; - } - text.push_back (make_shared(this, content->only_text(), first)); + text.push_back (make_shared(this, content->only_text())); + update_position(); } @@ -65,6 +62,8 @@ StringTextFileDecoder::seek (ContentTime time, bool accurate) while (_next < _subtitles.size() && ContentTime::from_seconds (_subtitles[_next].from.all_as_seconds ()) < time) { ++_next; } + + update_position(); } @@ -79,6 +78,9 @@ StringTextFileDecoder::pass () only_text()->emit_plain (p, _subtitles[_next]); ++_next; + + update_position(); + return false; } @@ -91,3 +93,15 @@ StringTextFileDecoder::content_time_period (sub::Subtitle s) const ContentTime::from_seconds (s.to.all_as_seconds()) ); } + + +void +StringTextFileDecoder::update_position () +{ + if (_next < _subtitles.size()) { + only_text()->maybe_set_position( + ContentTime::from_seconds(_subtitles[_next].from.all_as_seconds()) + ); + } +} + diff --git a/src/lib/string_text_file_decoder.h b/src/lib/string_text_file_decoder.h index 8fbf09608..c58021acb 100644 --- a/src/lib/string_text_file_decoder.h +++ b/src/lib/string_text_file_decoder.h @@ -36,6 +36,7 @@ public: private: dcpomatic::ContentTimePeriod content_time_period (sub::Subtitle s) const; + void update_position(); size_t _next; }; diff --git a/src/lib/text_decoder.cc b/src/lib/text_decoder.cc index e3b7d9958..b1b6cbcc4 100644 --- a/src/lib/text_decoder.cc +++ b/src/lib/text_decoder.cc @@ -41,12 +41,10 @@ using namespace dcpomatic; TextDecoder::TextDecoder ( Decoder* parent, - shared_ptr content, - ContentTime first + shared_ptr content ) : DecoderPart (parent) , _content (content) - , _position (first) { } @@ -63,7 +61,7 @@ void TextDecoder::emit_bitmap_start (ContentBitmapText const& bitmap) { BitmapStart (bitmap); - _position = bitmap.from(); + maybe_set_position(bitmap.from()); } @@ -116,7 +114,7 @@ TextDecoder::emit_plain_start (ContentTime from, vector sub } PlainStart(ContentStringText(from, string_texts)); - _position = from; + maybe_set_position(from); } @@ -274,7 +272,7 @@ TextDecoder::emit_plain_start (ContentTime from, sub::Subtitle const & sub_subti } PlainStart(ContentStringText(from, string_texts)); - _position = from; + maybe_set_position(from); } @@ -318,3 +316,13 @@ TextDecoder::seek () { _position = ContentTime (); } + + +void +TextDecoder::maybe_set_position (dcpomatic::ContentTime position) +{ + if (!_position || position > *_position) { + _position = position; + } +} + diff --git a/src/lib/text_decoder.h b/src/lib/text_decoder.h index 607d9e194..5f01c5b1f 100644 --- a/src/lib/text_decoder.h +++ b/src/lib/text_decoder.h @@ -42,11 +42,7 @@ class Image; class TextDecoder : public DecoderPart { public: - TextDecoder ( - Decoder* parent, - std::shared_ptr, - dcpomatic::ContentTime first - ); + TextDecoder (Decoder* parent, std::shared_ptr); boost::optional position (std::shared_ptr) const override { return _position; @@ -60,6 +56,8 @@ public: void emit_plain (dcpomatic::ContentTimePeriod period, sub::Subtitle const & subtitle); void emit_stop (dcpomatic::ContentTime to); + void maybe_set_position (dcpomatic::ContentTime position); + void seek () override; std::shared_ptr content () const { diff --git a/test/player_test.cc b/test/player_test.cc index e78e0ee35..80c7cf9eb 100644 --- a/test/player_test.cc +++ b/test/player_test.cc @@ -501,3 +501,40 @@ BOOST_AUTO_TEST_CASE (encrypted_dcp_with_no_kdm_gives_no_butler_error) BOOST_CHECK_NO_THROW(butler.rethrow()); } + +BOOST_AUTO_TEST_CASE (interleaved_subtitle_are_emitted_correctly) +{ + boost::filesystem::path paths[2] = { + "build/test/interleaved_subtitle_are_emitted_correctly1.srt", + "build/test/interleaved_subtitle_are_emitted_correctly2.srt" + }; + + dcp::File subs_file[2] = { dcp::File(paths[0], "w"), dcp::File(paths[1], "w") }; + + fprintf(subs_file[0].get(), "1\n00:00:01,000 -> 00:00:02,000\nSub 1/1\n\n"); + fprintf(subs_file[0].get(), "2\n00:00:05,000 -> 00:00:06,000\nSub 1/2\n\n"); + + fprintf(subs_file[1].get(), "1\n00:00:00,500 -> 00:00:01,500\nSub 2/1\n\n"); + fprintf(subs_file[1].get(), "2\n00:00:02,000 -> 00:00:03,000\nSub 2/2\n\n"); + + subs_file[0].close(); + subs_file[1].close(); + + auto subs1 = content_factory(paths[0]).front(); + auto subs2 = content_factory(paths[1]).front(); + auto film = new_test_film2("interleaved_subtitle_are_emitted_correctly", { subs1, subs2 }); + film->set_sequence(false); + subs1->set_position(film, DCPTime()); + subs2->set_position(film, DCPTime()); + + auto player = std::make_shared(film, Image::Alignment::COMPACT); + dcp::Time last; + player->Text.connect([&last](PlayerText text, TextType, optional, dcpomatic::DCPTimePeriod) { + for (auto sub: text.string) { + BOOST_CHECK(sub.in() >= last); + last = sub.in(); + } + }); + while (!player->pass()) {} +} +