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.

9 files changed, 49 insertions, 64 deletions

diff --git a/src/lib/content_subtitle.cc b/src/lib/content_subtitle.cc
deleted file mode 100644
index 4eed8b4b2..000000000
--- 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 ef904a980..3c0ce523e 100644
--- 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 3c7bffdda..bb2537fc4 100644
--- 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 a27d6b2db..fb2213fa2 100644
--- 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 c2bd4f3e0..6542c1a8e 100644
--- 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 38ccca0a3..db8374c5c 100644
--- 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 dd2558505..d20196a63 100644
--- 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 c958419c7..5f11e27e7 100644
--- 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 85cbcb7b6..1f870462e 100644
--- 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