Fix missing subtitles in some cases.

We were passing subtitles back from decoders to SubtitleDecoder
using dcp::SubtitleStrings and relying on their storage of time
to know when the subtitles were.  These times are quantised (by
the use of dcp::SubtitleString) and then compared with unquantised
times (kept as ContentTime) in the main checking loop in
SubtitleDecoder::get().

Fix this by storing periods as ContentTimePeriod as well as
in the dcp::SubtitleStrings.

diff --git a/src/lib/content_subtitle.cc b/src/lib/content_subtitle.cc
deleted file mode 100644 (file)
index 4eed8b4..0000000
--- a/src/lib/content_subtitle.cc
+++ /dev/null
@@ -1,31 +0,0 @@
-/*
-    Copyright (C) 2014 Carl Hetherington <cth@carlh.net>
-
-    This program is free software; you can redistribute it and/or modify
-    it under the terms of the GNU General Public License as published by
-    the Free Software Foundation; either version 2 of the License, or
-    (at your option) any later version.
-
-    This program is distributed in the hope that it will be useful,
-    but WITHOUT ANY WARRANTY; without even the implied warranty of
-    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-    GNU General Public License for more details.
-
-    You should have received a copy of the GNU General Public License
-    along with this program; if not, write to the Free Software
-    Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
-
-*/
-
-#include "content_subtitle.h"
-
-ContentTimePeriod
-ContentTextSubtitle::period () const
-{
-       /* XXX: assuming we have some subs and they are all at the same time */
-       DCPOMATIC_ASSERT (!subs.empty ());
-       return ContentTimePeriod (
-               ContentTime::from_seconds (subs.front().in().as_seconds()),
-               ContentTime::from_seconds (subs.front().out().as_seconds())
-               );
-}

diff --git a/src/lib/content_subtitle.h b/src/lib/content_subtitle.h
index ef904a9805e50476b064b7cdc589290694ff7777..3c0ce523e8497e969836cf0305957fc323f2c46e 100644 (file)
--- a/src/lib/content_subtitle.h
+++ b/src/lib/content_subtitle.h
@@ -31,37 +31,42 @@ class Image;
 class ContentSubtitle
 {
 public:
-       virtual ContentTimePeriod period () const = 0;
+       ContentSubtitle (ContentTimePeriod p)
+               : _period (p)
+       {}
+
+       ContentTimePeriod period () const {
+               return _period;
+       }
+
+private:
+       ContentTimePeriod _period;
 };
 
 class ContentImageSubtitle : public ContentSubtitle
 {
 public:
        ContentImageSubtitle (ContentTimePeriod p, boost::shared_ptr<Image> im, dcpomatic::Rect<double> r)
-               : sub (im, r)
-               , _period (p)
+               : ContentSubtitle (p)
+               , sub (im, r)
        {}
 
-       ContentTimePeriod period () const {
-               return _period;
-       }
-
        /* Our subtitle, with its rectangle unmodified by any offsets or scales that the content specifies */
        ImageSubtitle sub;
-
-private:
-       ContentTimePeriod _period;
 };
 
+/** A text subtitle.  We store the time period separately (as well as in the dcp::SubtitleStrings)
+ *  as the dcp::SubtitleString timings are sometimes quite heavily quantised and this causes problems
+ *  when we want to compare the quantised periods to the unquantised ones.
+ */
 class ContentTextSubtitle : public ContentSubtitle
 {
 public:
-       ContentTextSubtitle (std::list<dcp::SubtitleString> s)
-               : subs (s)
+       ContentTextSubtitle (ContentTimePeriod p, std::list<dcp::SubtitleString> s)
+               : ContentSubtitle (p)
+               , subs (s)
        {}
 
-       ContentTimePeriod period () const;
-
        std::list<dcp::SubtitleString> subs;
 };
 

diff --git a/src/lib/dcp_subtitle_decoder.cc b/src/lib/dcp_subtitle_decoder.cc
index 3c7bffdda440295cdc8033bb943922b6e5ba743b..bb2537fc4181368e4ca53390513f18941a9f58e0 100644 (file)
--- a/src/lib/dcp_subtitle_decoder.cc
+++ b/src/lib/dcp_subtitle_decoder.cc
@@ -54,7 +54,7 @@ DCPSubtitleDecoder::pass ()
 
        list<dcp::SubtitleString> s;
        s.push_back (*_next);
-       text_subtitle (s);
+       text_subtitle (content_time_period (*_next), s);
        ++_next;
 
        return false;
@@ -74,11 +74,7 @@ DCPSubtitleDecoder::text_subtitles_during (ContentTimePeriod p, bool starting) c
        list<ContentTimePeriod> d;
 
        for (list<dcp::SubtitleString>::const_iterator i = _subtitles.begin(); i != _subtitles.end(); ++i) {
-               ContentTimePeriod period (
-                       ContentTime::from_seconds (i->in().as_seconds ()),
-                       ContentTime::from_seconds (i->out().as_seconds ())
-                       );
-
+               ContentTimePeriod period = content_time_period (*i);
                if ((starting && p.contains (period.from)) || (!starting && p.overlaps (period))) {
                        d.push_back (period);
                }
@@ -87,3 +83,11 @@ DCPSubtitleDecoder::text_subtitles_during (ContentTimePeriod p, bool starting) c
        return d;
 }
 
+ContentTimePeriod
+DCPSubtitleDecoder::content_time_period (dcp::SubtitleString s) const
+{
+       return ContentTimePeriod (
+               ContentTime::from_seconds (s.in().as_seconds ()),
+               ContentTime::from_seconds (s.out().as_seconds ())
+               );
+}

diff --git a/src/lib/dcp_subtitle_decoder.h b/src/lib/dcp_subtitle_decoder.h
index a27d6b2dba44ab9fa66f8fbbebc5b68736b9ef0c..fb2213fa271d2f933d981d042ce268130aaf653f 100644 (file)
--- a/src/lib/dcp_subtitle_decoder.h
+++ b/src/lib/dcp_subtitle_decoder.h
@@ -34,6 +34,7 @@ protected:
 private:
        std::list<ContentTimePeriod> image_subtitles_during (ContentTimePeriod, bool starting) const;
        std::list<ContentTimePeriod> text_subtitles_during (ContentTimePeriod, bool starting) const;
+       ContentTimePeriod content_time_period (dcp::SubtitleString s) const;
 
        std::list<dcp::SubtitleString> _subtitles;
        std::list<dcp::SubtitleString>::const_iterator _next;

diff --git a/src/lib/subrip_decoder.cc b/src/lib/subrip_decoder.cc
index c2bd4f3e0bd7dc8fa35167bc726447bca72958d2..6542c1a8e2590e37b581cedd7efc942aa359d1be 100644 (file)
--- a/src/lib/subrip_decoder.cc
+++ b/src/lib/subrip_decoder.cc
@@ -82,7 +82,8 @@ SubRipDecoder::pass ()
                }
        }
 
-       text_subtitle (out);
+       text_subtitle (content_time_period (_subtitles[_next]), out);
+
        ++_next;
        return false;
 }
@@ -101,12 +102,7 @@ SubRipDecoder::text_subtitles_during (ContentTimePeriod p, bool starting) const
        list<ContentTimePeriod> d;
 
        for (vector<sub::Subtitle>::const_iterator i = _subtitles.begin(); i != _subtitles.end(); ++i) {
-
-               ContentTimePeriod t (
-                       ContentTime::from_seconds (i->from.all_as_seconds()),
-                       ContentTime::from_seconds (i->to.all_as_seconds())
-                       );
-
+               ContentTimePeriod t = content_time_period (*i);
                if ((starting && p.contains (t.from)) || (!starting && p.overlaps (t))) {
                        d.push_back (t);
                }
@@ -114,3 +110,12 @@ SubRipDecoder::text_subtitles_during (ContentTimePeriod p, bool starting) const
 
        return d;
 }
+
+ContentTimePeriod
+SubRipDecoder::content_time_period (sub::Subtitle s) const
+{
+       return ContentTimePeriod (
+               ContentTime::from_seconds (s.from.all_as_seconds()),
+               ContentTime::from_seconds (s.to.all_as_seconds())
+               );
+}

diff --git a/src/lib/subrip_decoder.h b/src/lib/subrip_decoder.h
index 38ccca0a38521c0c98caafdc4ce5c1e06c2b6618..db8374c5c86fac05cbac154ca31329618caea02c 100644 (file)
--- a/src/lib/subrip_decoder.h
+++ b/src/lib/subrip_decoder.h
@@ -37,6 +37,7 @@ protected:
 private:
        std::list<ContentTimePeriod> image_subtitles_during (ContentTimePeriod, bool starting) const;
        std::list<ContentTimePeriod> text_subtitles_during (ContentTimePeriod, bool starting) const;
+       ContentTimePeriod content_time_period (sub::Subtitle s) const;
 
        size_t _next;
 };

diff --git a/src/lib/subtitle_decoder.cc b/src/lib/subtitle_decoder.cc
index dd2558505d8d460ca3b1ba0e85eae4b2207e2ebc..d20196a63597f41a768e1aa7e1b98ab7c52010f0 100644 (file)
--- a/src/lib/subtitle_decoder.cc
+++ b/src/lib/subtitle_decoder.cc
@@ -17,9 +17,10 @@
 
 */
 
-#include <boost/shared_ptr.hpp>
 #include "subtitle_decoder.h"
 #include "subtitle_content.h"
+#include <boost/shared_ptr.hpp>
+#include <boost/foreach.hpp>
 
 using std::list;
 using std::cout;
@@ -46,9 +47,9 @@ SubtitleDecoder::image_subtitle (ContentTimePeriod period, shared_ptr<Image> ima
 }
 
 void
-SubtitleDecoder::text_subtitle (list<dcp::SubtitleString> s)
+SubtitleDecoder::text_subtitle (ContentTimePeriod period, list<dcp::SubtitleString> s)
 {
-       _decoded_text_subtitles.push_back (ContentTextSubtitle (s));
+       _decoded_text_subtitles.push_back (ContentTextSubtitle (period, s));
 }
 
 /** @param sp Full periods of subtitles that are showing or starting during the specified period */

diff --git a/src/lib/subtitle_decoder.h b/src/lib/subtitle_decoder.h
index c958419c75e6b0dcb4fc1a3a20f2996248ca8524..5f11e27e7157a3c607175d994fbefdb63ea8f83e 100644 (file)
--- a/src/lib/subtitle_decoder.h
+++ b/src/lib/subtitle_decoder.h
@@ -42,7 +42,7 @@ protected:
        void seek (ContentTime, bool);
 
        void image_subtitle (ContentTimePeriod period, boost::shared_ptr<Image>, dcpomatic::Rect<double>);
-       void text_subtitle (std::list<dcp::SubtitleString>);
+       void text_subtitle (ContentTimePeriod period, std::list<dcp::SubtitleString>);
 
        std::list<ContentImageSubtitle> _decoded_image_subtitles;
        std::list<ContentTextSubtitle> _decoded_text_subtitles;

diff --git a/src/lib/wscript b/src/lib/wscript
index 85cbcb7b6a20476305c09497ff7ce26b33676d56..1f870462edf106b22703e7c87e43254c6f957ff4 100644 (file)
--- a/src/lib/wscript
+++ b/src/lib/wscript
@@ -36,7 +36,6 @@ sources = """
           config.cc
           content.cc
           content_factory.cc
-          content_subtitle.cc
           cross.cc
           data.cc
           dcp_content.cc
@@ -132,7 +131,7 @@ def build(bld):
     obj.name = 'libdcpomatic2'
     obj.export_includes = ['..']
     obj.uselib = """
-                 AVCODEC AVUTIL AVFORMAT AVFILTER SWSCALE SWRESAMPLE 
+                 AVCODEC AVUTIL AVFORMAT AVFILTER SWSCALE SWRESAMPLE
                  BOOST_FILESYSTEM BOOST_THREAD BOOST_DATETIME BOOST_SIGNALS2
                  SNDFILE OPENJPEG POSTPROC TIFF MAGICK SSH DCP CXML GLIB LZMA XML++
                  CURL ZIP PANGOMM CAIROMM XMLSEC SUB

diff --git a/src/wx/about_dialog.cc b/src/wx/about_dialog.cc
index 142705a557008d90f12dee019242592d7223b02a..85cfc92be1958dc513e91957f689cdf555ac0fb2 100644 (file)
--- a/src/wx/about_dialog.cc
+++ b/src/wx/about_dialog.cc
@@ -227,6 +227,7 @@ AboutDialog::AboutDialog (wxWindow* parent)
        tested_by.Add (wxT ("Mauro Ottonello"));
        tested_by.Add (wxT ("Peter Puchner"));
        tested_by.Add (wxT ("Markus Raab"));
+       tested_by.Add (wxT ("Michael Reckert"));
        tested_by.Add (wxT ("Greg Rooke"));
        tested_by.Add (wxT ("Elad Saad"));
        tested_by.Add (wxT ("Karim Senoucci"));

diff --git a/test/srt_subtitle_test.cc b/test/srt_subtitle_test.cc
index 161d23d7769af904f7d3444ffe147121707b633f..1837d816b4a7ff7a05735148c04ec22773f4f575 100644 (file)
--- a/test/srt_subtitle_test.cc
+++ b/test/srt_subtitle_test.cc
@@ -27,7 +27,11 @@
 #include "lib/font.h"
 #include "test.h"
 #include <boost/test/unit_test.hpp>
+#include <boost/algorithm/string.hpp>
+#include <list>
 
+using std::string;
+using std::list;
 using boost::shared_ptr;
 
 /** Make a very short DCP with a single subtitle from .srt with no specified fonts */
@@ -73,3 +77,45 @@ BOOST_AUTO_TEST_CASE (srt_subtitle_test2)
        check_dcp ("test/data/srt_subtitle_test2", film->dir (film->dcp_name ()));
 }
 
+/** Make another DCP with a longer .srt file */
+BOOST_AUTO_TEST_CASE (srt_subtitle_test3)
+{
+       shared_ptr<Film> film = new_test_film ("srt_subtitle_test3");
+
+       film->set_container (Ratio::from_id ("185"));
+       film->set_dcp_content_type (DCPContentType::from_isdcf_name ("TLR"));
+       film->set_name ("frobozz");
+       film->set_interop (true);
+       shared_ptr<SubRipContent> content (new SubRipContent (film, private_data / "Ankoemmling.srt"));
+       film->examine_and_add_content (content);
+       wait_for_jobs ();
+
+       content->set_use_subtitles (true);
+       content->set_burn_subtitles (false);
+
+       film->make_dcp ();
+       wait_for_jobs ();
+
+       /* Find the subtitle file and check it */
+       for (
+               boost::filesystem::directory_iterator i = boost::filesystem::directory_iterator (film->directory() / film->dcp_name (false));
+               i != boost::filesystem::directory_iterator ();
+               ++i) {
+
+               if (boost::filesystem::is_directory (i->path ())) {
+                       for (
+                               boost::filesystem::directory_iterator j = boost::filesystem::directory_iterator (i->path ());
+                               j != boost::filesystem::directory_iterator ();
+                               ++j) {
+
+                               std::cout << j->path().string() << "\n";
+
+                               if (boost::algorithm::starts_with (j->path().leaf().string(), "sub_")) {
+                                       list<string> ignore;
+                                       ignore.push_back ("SubtitleID");
+                                       check_xml (*j, private_data / "Ankoemmling.xml", ignore);
+                               }
+                       }
+               }
+       }
+}