From: Carl Hetherington Date: Fri, 26 Jun 2015 21:02:00 +0000 (+0100) Subject: Fix missing subtitles in some cases. X-Git-Tag: v2.1.11~2 X-Git-Url: https://main.carlh.net/gitweb/?a=commitdiff_plain;h=c8ff422a42eac30517a7acde57ab84e55449f4e4;p=dcpomatic.git 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 index 4eed8b4b2..000000000 --- a/src/lib/content_subtitle.cc +++ /dev/null @@ -1,31 +0,0 @@ -/* - Copyright (C) 2014 Carl Hetherington - - 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 im, dcpomatic::Rect 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 s) - : subs (s) + ContentTextSubtitle (ContentTimePeriod p, std::list s) + : ContentSubtitle (p) + , subs (s) {} - ContentTimePeriod period () const; - std::list 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 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 d; for (list::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 image_subtitles_during (ContentTimePeriod, bool starting) const; std::list text_subtitles_during (ContentTimePeriod, bool starting) const; + ContentTimePeriod content_time_period (dcp::SubtitleString s) const; std::list _subtitles; std::list::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 d; for (vector::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 image_subtitles_during (ContentTimePeriod, bool starting) const; std::list 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 #include "subtitle_decoder.h" #include "subtitle_content.h" +#include +#include using std::list; using std::cout; @@ -46,9 +47,9 @@ SubtitleDecoder::image_subtitle (ContentTimePeriod period, shared_ptr ima } void -SubtitleDecoder::text_subtitle (list s) +SubtitleDecoder::text_subtitle (ContentTimePeriod period, list 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, dcpomatic::Rect); - void text_subtitle (std::list); + void text_subtitle (ContentTimePeriod period, std::list); std::list _decoded_image_subtitles; std::list _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 diff --git a/src/wx/about_dialog.cc b/src/wx/about_dialog.cc index 142705a55..85cfc92be 100644 --- 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 161d23d77..1837d816b 100644 --- a/test/srt_subtitle_test.cc +++ b/test/srt_subtitle_test.cc @@ -27,7 +27,11 @@ #include "lib/font.h" #include "test.h" #include +#include +#include +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 = 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 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 ignore; + ignore.push_back ("SubtitleID"); + check_xml (*j, private_data / "Ankoemmling.xml", ignore); + } + } + } + } +}