Fix overlapping burnt-in subtitles in some cases (#959).

Firstly, when finding subtitles that exist during a period, only
return those which overlap more than half the period.  This means
that, in a fight over a frame, the longest-running subtitle in that
frame will win.

Secondly, make SubtitleDecoder::get pick the wanted subtitles from
the cache simply by comparing their periods to those that were
requested.  I think this is nicer than what was there before
(basically reevaulating 'what subtitle(s) for this period') and
also makes the first part of this commit effective.

diff --git a/ChangeLog b/ChangeLog
index 34fe178a42763cb8a66206e09b1d0bb947ebb8c3..d603234d0630dced2e9ca703fe68f1ee37e0e2f1 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2016-10-08  Carl Hetherington  <cth@carlh.net>
+
+       * Fix overlapping burnt-in subtitles in some cases (#959).
+
 2016-10-07  Carl Hetherington  <cth@carlh.net>
 
        * Fix XML subtitle output in some cases.

diff --git a/src/lib/dcpomatic_time.h b/src/lib/dcpomatic_time.h
index c13f2bbfe7a4e00e2b1b60e1ed218ede6e085d05..da903cf09316e0e34865e62a5443a7ff1ba5b64e 100644 (file)
--- a/src/lib/dcpomatic_time.h
+++ b/src/lib/dcpomatic_time.h
@@ -275,6 +275,10 @@ public:
        bool operator== (TimePeriod<T> const & other) const {
                return from == other.from && to == other.to;
        }
+
+       bool operator!= (TimePeriod<T> const & other) const {
+               return !(*this == other);
+       }
 };
 
 typedef TimePeriod<ContentTime> ContentTimePeriod;

diff --git a/src/lib/subtitle_decoder.cc b/src/lib/subtitle_decoder.cc
index bc4a75ca841b26ca2c7a7eb9004bb7a0f49c3db0..307226da6b184a25e12585dd14b2d83c4880c0a2 100644 (file)
--- a/src/lib/subtitle_decoder.cc
+++ b/src/lib/subtitle_decoder.cc
@@ -78,13 +78,15 @@ SubtitleDecoder::give_text (ContentTimePeriod period, list<dcp::SubtitleString>
        _decoded_text.push_back (ContentTextSubtitle (period, s));
 }
 
-/** @param sp Full periods of subtitles that are showing or starting during the specified period */
+/** Get the subtitles that correspond to a given list of periods.
+ *  @param subs Subtitles.
+ *  @param sp Periods for which to extract subtitles from subs.
+ */
 template <class T>
 list<T>
-SubtitleDecoder::get (list<T> const & subs, list<ContentTimePeriod> const & sp, ContentTimePeriod period, bool starting, bool accurate)
+SubtitleDecoder::get (list<T> const & subs, list<ContentTimePeriod> const & sp, ContentTimePeriod period, bool accurate)
 {
        if (sp.empty ()) {
-               /* Nothing in this period */
                return list<T> ();
        }
 
@@ -103,9 +105,13 @@ SubtitleDecoder::get (list<T> const & subs, list<ContentTimePeriod> const & sp,
        /* XXX: inefficient */
 
        list<T> out;
-       for (typename list<T>::const_iterator i = subs.begin(); i != subs.end(); ++i) {
-               if ((starting && period.contains(i->period().from)) || (!starting && period.overlap(i->period()))) {
-                       out.push_back (*i);
+       BOOST_FOREACH (ContentTimePeriod i, sp) {
+               typename list<T>::const_iterator j = subs.begin();
+               while (j != subs.end() && j->period() != i) {
+                       ++j;
+               }
+               if (j != subs.end()) {
+                       out.push_back (*j);
                }
        }
 
@@ -132,13 +138,13 @@ SubtitleDecoder::get (list<T> const & subs, list<ContentTimePeriod> const & sp,
 list<ContentTextSubtitle>
 SubtitleDecoder::get_text (ContentTimePeriod period, bool starting, bool accurate)
 {
-       return get<ContentTextSubtitle> (_decoded_text, _text_during (period, starting), period, starting, accurate);
+       return get<ContentTextSubtitle> (_decoded_text, _text_during (period, starting), period, accurate);
 }
 
 list<ContentImageSubtitle>
 SubtitleDecoder::get_image (ContentTimePeriod period, bool starting, bool accurate)
 {
-       return get<ContentImageSubtitle> (_decoded_image, _image_during (period, starting), period, starting, accurate);
+       return get<ContentImageSubtitle> (_decoded_image, _image_during (period, starting), period, accurate);
 }
 
 void
@@ -170,7 +176,7 @@ SubtitleDecoder::give_text (ContentTimePeriod period, sub::Subtitle const & subt
                }
        }
 
-       /* Find the lowest proportional postion */
+       /* Find the lowest proportional position */
        optional<float> lowest_proportional;
        BOOST_FOREACH (sub::Line i, subtitle.lines) {
                if (i.vertical_position.proportional) {
@@ -187,7 +193,7 @@ SubtitleDecoder::give_text (ContentTimePeriod period, sub::Subtitle const & subt
                BOOST_FOREACH (sub::Block j, i.blocks) {
 
                        if (!j.font_size.specified()) {
-                               /* Fallback default font size if none other has been specified */
+                               /* Fallback default font size if no other has been specified */
                                j.font_size.set_points (48);
                        }
 

diff --git a/src/lib/subtitle_decoder.h b/src/lib/subtitle_decoder.h
index 726e898b1dcc979366b45f3a4557a95722e1a569..eba36315e3d6ad0102a9993386878d8a297a89d6 100644 (file)
--- a/src/lib/subtitle_decoder.h
+++ b/src/lib/subtitle_decoder.h
@@ -69,7 +69,7 @@ private:
        boost::shared_ptr<const SubtitleContent> _content;
 
        template <class T>
-       std::list<T> get (std::list<T> const & subs, std::list<ContentTimePeriod> const & sp, ContentTimePeriod period, bool starting, bool accurate);
+       std::list<T> get (std::list<T> const & subs, std::list<ContentTimePeriod> const & sp, ContentTimePeriod period, bool accurate);
 
        boost::function<std::list<ContentTimePeriod> (ContentTimePeriod, bool)> _image_during;
        boost::function<std::list<ContentTimePeriod> (ContentTimePeriod, bool)> _text_during;

diff --git a/src/lib/text_subtitle_decoder.cc b/src/lib/text_subtitle_decoder.cc
index bec2ab9b7b32029802e2d8dbdb8a5efb62a378a7..05bde829cb6237dcb4a02d7d14ff4ab6372278bf 100644 (file)
--- a/src/lib/text_subtitle_decoder.cc
+++ b/src/lib/text_subtitle_decoder.cc
@@ -85,10 +85,20 @@ TextSubtitleDecoder::text_subtitles_during (ContentTimePeriod p, bool starting)
 
        list<ContentTimePeriod> d;
 
+       /* Only take `during' (not starting) subs if they overlap more than half the requested period;
+          here's the threshold for being significant.
+       */
+       ContentTime const significant (p.duration().get() / 2);
+
        for (vector<sub::Subtitle>::const_iterator i = _subtitles.begin(); i != _subtitles.end(); ++i) {
                ContentTimePeriod t = content_time_period (*i);
-               if ((starting && p.contains (t.from)) || (!starting && p.overlap (t))) {
+               if (starting && p.contains(t.from)) {
                        d.push_back (t);
+               } else if (!starting) {
+                       optional<ContentTimePeriod> const o = p.overlap (t);
+                       if (o && o->duration() > significant) {
+                               d.push_back (t);
+                       }
                }
        }