Make separate reels for parts of the timeline with no video when

we are in REEL_TYPE_BY_VIDEO_CONTENT mode.  This fixes VF creation
with gaps.  Also the implementation of Film::reels() is cleaner now.

Backport of 122bea7f0e08e07dcdaccd51751a9c83504f4c04 from master.

4 files changed, 95 insertions, 16 deletions

diff --git a/src/lib/film.cc b/src/lib/film.cc
index d311c76cb..9f909633a 100644
--- a/src/lib/film.cc
+++ b/src/lib/film.cc
@@ -1563,29 +1563,29 @@ Film::reels () const
 		break;
 	case REELTYPE_BY_VIDEO_CONTENT:
 	{
-		DCPTime last_split;
-		shared_ptr<Content> last_video;
-		BOOST_FOREACH (shared_ptr<Content> c, content ()) {
+		/* Collect all reel boundaries */
+		list<DCPTime> split_points;
+		split_points.push_back (DCPTime());
+		split_points.push_back (len);
+		BOOST_FOREACH (shared_ptr<Content> c, content()) {
 			if (c->video) {
 				BOOST_FOREACH (DCPTime t, c->reel_split_points(shared_from_this())) {
-					if (last_split != t) {
-						p.push_back (DCPTimePeriod(last_split, t));
-						last_split = t;
-					}
+					split_points.push_back (t);
 				}
-				last_video = c;
+				split_points.push_back (c->end(shared_from_this()));
 			}
 		}
 
-		DCPTime video_end = last_video ? last_video->end(shared_from_this()) : DCPTime(0);
-		if (last_split != video_end) {
-			/* Definitely go from the last split to the end of the video content */
-			p.push_back (DCPTimePeriod(last_split, video_end));
-		}
+		split_points.sort ();
+		split_points.unique ();
 
-		if (video_end < len) {
-			/* And maybe go after that as well if there is any non-video hanging over the end */
-			p.push_back (DCPTimePeriod (video_end, len));
+		/* Make them into periods */
+		optional<DCPTime> last;
+		BOOST_FOREACH (DCPTime t, split_points) {
+			if (last) {
+				p.push_back (DCPTimePeriod(*last, t));
+			}
+			last = t;
 		}
 		break;
 	}
diff --git a/src/lib/reel_writer.cc b/src/lib/reel_writer.cc
index 67606cee2..8044973ec 100644
--- a/src/lib/reel_writer.cc
+++ b/src/lib/reel_writer.cc
@@ -439,6 +439,8 @@ maybe_add_text (
 shared_ptr<dcp::Reel>
 ReelWriter::create_reel (list<ReferencedReelAsset> const & refs, list<shared_ptr<Font> > const & fonts)
 {
+	LOG_GENERAL ("create_reel for %1-%2; %3 of %4", _period.from.get(), _period.to.get(), _reel_index, _reel_count);
+
 	shared_ptr<dcp::Reel> reel (new dcp::Reel ());
 
 	shared_ptr<dcp::ReelPictureAsset> reel_picture_asset;
diff --git a/test/reels_test.cc b/test/reels_test.cc
index e26dea5b8..45c87654d 100644
--- a/test/reels_test.cc
+++ b/test/reels_test.cc
@@ -440,3 +440,44 @@ BOOST_AUTO_TEST_CASE (reels_test11)
 	BOOST_CHECK_EQUAL (r.back().from.get(), DCPTime::from_seconds(1).get());
 	BOOST_CHECK_EQUAL (r.back().to.get(), DCPTime::from_seconds(1 + 10).get());
 }
+
+/** For VFs to work right we have to make separate reels for empty bits between
+ *  video content.
+ */
+BOOST_AUTO_TEST_CASE (reels_test12)
+{
+	shared_ptr<Film> film = new_test_film2 ("reels_test12");
+	film->set_video_frame_rate (24);
+	film->set_reel_type (REELTYPE_BY_VIDEO_CONTENT);
+	film->set_sequence (false);
+
+	shared_ptr<FFmpegContent> A(new FFmpegContent("test/data/flat_red.png"));
+	film->examine_and_add_content (A);
+	BOOST_REQUIRE (!wait_for_jobs());
+	A->video->set_length (240);
+	A->set_video_frame_rate (24);
+	A->set_position (film, DCPTime::from_seconds(1));
+
+	shared_ptr<FFmpegContent> B(new FFmpegContent("test/data/flat_red.png"));
+	film->examine_and_add_content (B);
+	BOOST_REQUIRE (!wait_for_jobs());
+	B->video->set_length (120);
+	B->set_video_frame_rate (24);
+	B->set_position (film, DCPTime::from_seconds(14));
+
+	list<DCPTimePeriod> r = film->reels ();
+	BOOST_REQUIRE_EQUAL (r.size(), 4);
+	list<DCPTimePeriod>::const_iterator i = r.begin ();
+
+	BOOST_CHECK_EQUAL (i->from.get(), 0);
+	BOOST_CHECK_EQUAL (i->to.get(),   DCPTime::from_seconds(1).get());
+	++i;
+	BOOST_CHECK_EQUAL (i->from.get(), DCPTime::from_seconds(1).get());
+	BOOST_CHECK_EQUAL (i->to.get(),   DCPTime::from_seconds(11).get());
+	++i;
+	BOOST_CHECK_EQUAL (i->from.get(), DCPTime::from_seconds(11).get());
+	BOOST_CHECK_EQUAL (i->to.get(),   DCPTime::from_seconds(14).get());
+	++i;
+	BOOST_CHECK_EQUAL (i->from.get(), DCPTime::from_seconds(14).get());
+	BOOST_CHECK_EQUAL (i->to.get(),   DCPTime::from_seconds(19).get());
+}
diff --git a/test/vf_test.cc b/test/vf_test.cc
index b82c3c889..d1879f4e3 100644
--- a/test/vf_test.cc
+++ b/test/vf_test.cc
@@ -336,3 +336,39 @@ BOOST_AUTO_TEST_CASE (vf_test6)
 	vf->make_dcp ();
 	BOOST_REQUIRE (!wait_for_jobs());
 }
+
+/** Test bug #1643 (the second part; referring fails if there are gaps) */
+BOOST_AUTO_TEST_CASE (vf_test7)
+{
+	/* First OV */
+	shared_ptr<Film> ov1 = new_test_film2 ("vf_test7_ov1");
+	ov1->set_video_frame_rate (24);
+	ov1->examine_and_add_content (content_factory("test/data/flat_red.png").front());
+	BOOST_REQUIRE (!wait_for_jobs());
+	ov1->make_dcp ();
+	BOOST_REQUIRE (!wait_for_jobs());
+
+	/* Second OV */
+	shared_ptr<Film> ov2 = new_test_film2 ("vf_test7_ov2");
+	ov2->set_video_frame_rate (24);
+	ov2->examine_and_add_content (content_factory("test/data/flat_red.png").front());
+	BOOST_REQUIRE (!wait_for_jobs());
+	ov2->make_dcp ();
+	BOOST_REQUIRE (!wait_for_jobs());
+
+	/* VF */
+	shared_ptr<Film> vf = new_test_film2 ("vf_test7_vf");
+	shared_ptr<DCPContent> ov1_dcp (new DCPContent(ov1->dir(ov1->dcp_name())));
+	vf->examine_and_add_content (ov1_dcp);
+	shared_ptr<DCPContent> ov2_dcp (new DCPContent(ov1->dir(ov1->dcp_name())));
+	vf->examine_and_add_content (ov2_dcp);
+	BOOST_REQUIRE (!wait_for_jobs());
+	vf->set_reel_type (REELTYPE_BY_VIDEO_CONTENT);
+	ov1_dcp->set_reference_video (true);
+	ov2_dcp->set_reference_video (true);
+	ov1_dcp->set_position (vf, DCPTime::from_seconds(1));
+	ov2_dcp->set_position (vf, DCPTime::from_seconds(20));
+	vf->write_metadata ();
+	vf->make_dcp ();
+	BOOST_REQUIRE (!wait_for_jobs());
+}