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 636cc8ed2b3f1f8d937d65943083e322e600bf9e..ad7d3e11277910fb62eb27611f0472a6ed97a94c 100644 (file)
--- a/src/lib/dcp_decoder.cc
+++ b/src/lib/dcp_decoder.cc
@@ -80,8 +80,11 @@ DCPDecoder::DCPDecoder (shared_ptr<const Film> film, shared_ptr<const DCPContent
                        audio = make_shared<AudioDecoder>(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<TextDecoder>(this, i, ContentTime()));
+                       text.push_back (make_shared<TextDecoder>(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<AtmosDecoder>(this, content);

diff --git a/src/lib/dcp_subtitle_decoder.cc b/src/lib/dcp_subtitle_decoder.cc
index 83faa1e93f19fbed0d3094b378ff1b97942e9cbd..4743ecf42c228fb1dcb2b65ee021ecb7289ce96f 100644 (file)
--- a/src/lib/dcp_subtitle_decoder.cc
+++ b/src/lib/dcp_subtitle_decoder.cc
@@ -46,11 +46,8 @@ DCPSubtitleDecoder::DCPSubtitleDecoder (shared_ptr<const Film> 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<TextDecoder>(this, content->only_text(), first));
+       text.push_back (make_shared<TextDecoder>(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 4f5964c6fb0019e70b46abaca8b46705ebf642db..16f89b53002b0023dd09e4baa37704aa6b3afdde 100644 (file)
--- 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<const dcp::Subtitle> s) const;
+       void update_position();
 
        std::vector<std::shared_ptr<const dcp::Subtitle>> _subtitles;
        std::vector<std::shared_ptr<const dcp::Subtitle>>::const_iterator _next;

diff --git a/src/lib/ffmpeg_decoder.cc b/src/lib/ffmpeg_decoder.cc
index bc7d28d7dc0ccf958b139d45c1c057d1f0c0b3a4..82eccb576d5df8248926315b72f6748406e9daaf 100644 (file)
--- a/src/lib/ffmpeg_decoder.cc
+++ b/src/lib/ffmpeg_decoder.cc
@@ -93,8 +93,10 @@ FFmpegDecoder::FFmpegDecoder (shared_ptr<const Film> film, shared_ptr<const FFmp
        }
 
        if (c->only_text()) {
-               /* XXX: this time here should be the time of the first subtitle, not 0 */
-               text.push_back (make_shared<TextDecoder>(this, c->only_text(), ContentTime()));
+               text.push_back (make_shared<TextDecoder>(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 d5f320a27f83ab83fa3c1705e8e9e0c0e3f1da3b..2ec0ec1cb74f10cc3f32769c378670f97204efd3 100644 (file)
--- a/src/lib/string_text_file_decoder.cc
+++ b/src/lib/string_text_file_decoder.cc
@@ -40,11 +40,8 @@ StringTextFileDecoder::StringTextFileDecoder (shared_ptr<const Film> film, share
        , StringTextFile (content)
        , _next (0)
 {
-       ContentTime first;
-       if (!_subtitles.empty()) {
-               first = content_time_period(_subtitles[0]).from;
-       }
-       text.push_back (make_shared<TextDecoder>(this, content->only_text(), first));
+       text.push_back (make_shared<TextDecoder>(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 8fbf09608cd36b108d33e74b9a6703499c29583f..c58021acbcde0c0cf8bbd926392fcd561b19a8df 100644 (file)
--- 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 e3b7d995879b7f5dcc61b919b68db5fc6f86e567..b1b6cbcc4d503d438fb46074dcc9ec8541444a35 100644 (file)
--- 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<const TextContent> content,
-       ContentTime first
+       shared_ptr<const TextContent> 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<dcp::SubtitleString> 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 607d9e194f95cadef876c147b2b151ce59fbcc3f..5f01c5b1f587d783cdbe3cb7f090ec47ec8ca360 100644 (file)
--- 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<const TextContent>,
-               dcpomatic::ContentTime first
-               );
+       TextDecoder (Decoder* parent, std::shared_ptr<const TextContent>);
 
        boost::optional<dcpomatic::ContentTime> position (std::shared_ptr<const Film>) 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<const TextContent> content () const {

diff --git a/test/player_test.cc b/test/player_test.cc
index e78e0ee355329035c5124beec2ff5df3cbb2cbac..80c7cf9eb44fc5509c7dd33a9fc1eb3493c08449 100644 (file)
--- 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<Player>(film, Image::Alignment::COMPACT);
+       dcp::Time last;
+       player->Text.connect([&last](PlayerText text, TextType, optional<DCPTextTrack>, dcpomatic::DCPTimePeriod) {
+               for (auto sub: text.string) {
+                       BOOST_CHECK(sub.in() >= last);
+                       last = sub.in();
+               }
+       });
+       while (!player->pass()) {}
+}
+