From 67775a6d0d28131b98ae284c7be23d79ccdab685 Mon Sep 17 00:00:00 2001 From: Carl Hetherington Date: Tue, 14 Apr 2020 23:11:08 +0200 Subject: [PATCH] Fix Empty/Player behaviour when using a playlist that is not the same as the Film's. Previously Empty would use the length of the film for its end point. Now it takes a Playlist (rather than a list of Pieces) and uses the length of that playlist for its end point. This fixes #1543, in which single-content audio analysis jobs would run for the whole length of the film, rather than the length of the content, producing strange graphs and incorrect progress reports. --- src/lib/empty.cc | 8 ++--- src/lib/empty.h | 5 +-- src/lib/player.cc | 12 +++---- src/wx/audio_dialog.h | 1 + test/empty_test.cc | 77 +++++++++++++++++++++++++++++-------------- 5 files changed, 67 insertions(+), 36 deletions(-) diff --git a/src/lib/empty.cc b/src/lib/empty.cc index 206acfdb0..71bf3aa95 100644 --- a/src/lib/empty.cc +++ b/src/lib/empty.cc @@ -36,16 +36,16 @@ using boost::dynamic_pointer_cast; using boost::function; using namespace dcpomatic; -Empty::Empty (shared_ptr film, list > pieces, function)> part) +Empty::Empty (shared_ptr film, shared_ptr playlist, function)> part) { list full; - BOOST_FOREACH (shared_ptr i, pieces) { + BOOST_FOREACH (shared_ptr i, playlist->content()) { if (part(i)) { - full.push_back (DCPTimePeriod (i->content->position(), i->content->end(film))); + full.push_back (DCPTimePeriod (i->position(), i->end(film))); } } - _periods = subtract (DCPTimePeriod(DCPTime(), film->length()), coalesce(full)); + _periods = subtract (DCPTimePeriod(DCPTime(), playlist->length(film)), coalesce(full)); if (!_periods.empty ()) { _position = _periods.front().from; diff --git a/src/lib/empty.h b/src/lib/empty.h index bd295206e..abd6fdef0 100644 --- a/src/lib/empty.h +++ b/src/lib/empty.h @@ -28,14 +28,14 @@ struct empty_test1; struct empty_test2; +struct empty_test3; struct player_subframe_test; -class Piece; class Empty { public: Empty () {} - Empty (boost::shared_ptr film, std::list > pieces, boost::function)> part); + Empty (boost::shared_ptr film, boost::shared_ptr playlist, boost::function)> part); dcpomatic::DCPTime position () const { return _position; @@ -50,6 +50,7 @@ public: private: friend struct ::empty_test1; friend struct ::empty_test2; + friend struct ::empty_test3; friend struct ::player_subframe_test; std::list _periods; diff --git a/src/lib/player.cc b/src/lib/player.cc index d5a558184..3202c1b85 100644 --- a/src/lib/player.cc +++ b/src/lib/player.cc @@ -125,15 +125,15 @@ Player::setup_pieces () } bool -have_video (shared_ptr piece) +have_video (shared_ptr content) { - return piece->decoder && piece->decoder->video; + return static_cast(content->video); } bool -have_audio (shared_ptr piece) +have_audio (shared_ptr content) { - return piece->decoder && piece->decoder->audio; + return static_cast(content->audio); } void @@ -237,8 +237,8 @@ Player::setup_pieces_unlocked () } } - _black = Empty (_film, _pieces, bind(&have_video, _1)); - _silent = Empty (_film, _pieces, bind(&have_audio, _1)); + _black = Empty (_film, _playlist, bind(&have_video, _1)); + _silent = Empty (_film, _playlist, bind(&have_audio, _1)); _last_video_time = DCPTime (); _last_video_eyes = EYES_BOTH; diff --git a/src/wx/audio_dialog.h b/src/wx/audio_dialog.h index e866aca3f..3a02fd87f 100644 --- a/src/wx/audio_dialog.h +++ b/src/wx/audio_dialog.h @@ -49,6 +49,7 @@ private: boost::shared_ptr _analysis; boost::weak_ptr _film; + /** content to analyse, or 0 to analyse all the film's content */ boost::weak_ptr _content; int _channels; boost::shared_ptr _playlist; diff --git a/test/empty_test.cc b/test/empty_test.cc index f0fa7138f..1a4d03300 100644 --- a/test/empty_test.cc +++ b/test/empty_test.cc @@ -34,17 +34,14 @@ using boost::shared_ptr; using namespace dcpomatic; bool -has_video (shared_ptr piece) +has_video (shared_ptr content) { - return piece->decoder && piece->decoder->video; + return static_cast(content->video); } BOOST_AUTO_TEST_CASE (empty_test1) { - shared_ptr film = new_test_film ("empty_test1"); - film->set_dcp_content_type (DCPContentType::from_isdcf_name ("FTR")); - film->set_name ("empty_test1"); - film->set_container (Ratio::from_id ("185")); + shared_ptr film = new_test_film2 ("empty_test1"); film->set_sequence (false); shared_ptr contentA (new ImageContent("test/data/simple_testcard_640x480.png")); shared_ptr contentB (new ImageContent("test/data/simple_testcard_640x480.png")); @@ -55,6 +52,9 @@ BOOST_AUTO_TEST_CASE (empty_test1) int const vfr = film->video_frame_rate (); + /* 0 1 2 3 4 5 6 7 + * A A A B + */ contentA->video->set_scale (VideoContentScale (Ratio::from_id ("185"))); contentA->video->set_length (3); contentA->set_position (film, DCPTime::from_frames (2, vfr)); @@ -62,27 +62,20 @@ BOOST_AUTO_TEST_CASE (empty_test1) contentB->video->set_length (1); contentB->set_position (film, DCPTime::from_frames (7, vfr)); - shared_ptr player (new Player(film, film->playlist())); - Empty black (film, player->_pieces, bind(&has_video, _1)); - BOOST_REQUIRE_EQUAL (black._periods.size(), 3); + Empty black (film, film->playlist(), bind(&has_video, _1)); + BOOST_REQUIRE_EQUAL (black._periods.size(), 2); list::const_iterator i = black._periods.begin(); - BOOST_CHECK (i->from == DCPTime()); - BOOST_CHECK (i->to == DCPTime::from_frames(2, vfr)); + BOOST_CHECK (i->from == DCPTime::from_frames(0, vfr)); + BOOST_CHECK (i->to == DCPTime::from_frames(2, vfr)); ++i; BOOST_CHECK (i->from == DCPTime::from_frames(5, vfr)); - BOOST_CHECK (i->to == DCPTime::from_frames(7, vfr)); - ++i; - BOOST_CHECK (i->from == DCPTime::from_frames(8, vfr)); - BOOST_CHECK (i->to == DCPTime::from_frames(24, vfr)); + BOOST_CHECK (i->to == DCPTime::from_frames(7, vfr)); } /** Some tests where the first empty period is not at time 0 */ BOOST_AUTO_TEST_CASE (empty_test2) { - shared_ptr film = new_test_film ("empty_test1"); - film->set_dcp_content_type (DCPContentType::from_isdcf_name ("FTR")); - film->set_name ("empty_test1"); - film->set_container (Ratio::from_id ("185")); + shared_ptr film = new_test_film2 ("empty_test2"); film->set_sequence (false); shared_ptr contentA (new ImageContent("test/data/simple_testcard_640x480.png")); shared_ptr contentB (new ImageContent("test/data/simple_testcard_640x480.png")); @@ -93,6 +86,9 @@ BOOST_AUTO_TEST_CASE (empty_test2) int const vfr = film->video_frame_rate (); + /* 0 1 2 3 4 5 6 7 + * A A A B + */ contentA->video->set_scale (VideoContentScale (Ratio::from_id ("185"))); contentA->video->set_length (3); contentA->set_position (film, DCPTime(0)); @@ -100,9 +96,8 @@ BOOST_AUTO_TEST_CASE (empty_test2) contentB->video->set_length (1); contentB->set_position (film, DCPTime::from_frames(7, vfr)); - shared_ptr player (new Player(film, film->playlist())); - Empty black (film, player->_pieces, bind(&has_video, _1)); - BOOST_REQUIRE_EQUAL (black._periods.size(), 2); + Empty black (film, film->playlist(), bind(&has_video, _1)); + BOOST_REQUIRE_EQUAL (black._periods.size(), 1); BOOST_CHECK (black._periods.front().from == DCPTime::from_frames(3, vfr)); BOOST_CHECK (black._periods.front().to == DCPTime::from_frames(7, vfr)); @@ -114,7 +109,41 @@ BOOST_AUTO_TEST_CASE (empty_test2) black.set_position (DCPTime::from_frames (4, vfr)); BOOST_CHECK (!black.done ()); black.set_position (DCPTime::from_frames (7, vfr)); - BOOST_CHECK (!black.done ()); - black.set_position (DCPTime::from_frames (24, vfr)); BOOST_CHECK (black.done ()); } + +/** Test for when the film's playlist is not the same as the one passed into Empty */ +BOOST_AUTO_TEST_CASE (empty_test3) +{ + shared_ptr film = new_test_film2 ("empty_test3"); + film->set_sequence (false); + shared_ptr contentA (new ImageContent("test/data/simple_testcard_640x480.png")); + shared_ptr contentB (new ImageContent("test/data/simple_testcard_640x480.png")); + + film->examine_and_add_content (contentA); + film->examine_and_add_content (contentB); + BOOST_REQUIRE (!wait_for_jobs()); + + int const vfr = film->video_frame_rate (); + + /* 0 1 2 3 4 5 6 7 + * A A A B + */ + contentA->video->set_scale (VideoContentScale (Ratio::from_id ("185"))); + contentA->video->set_length (3); + contentA->set_position (film, DCPTime(0)); + contentB->video->set_scale (VideoContentScale (Ratio::from_id ("185"))); + contentB->video->set_length (1); + contentB->set_position (film, DCPTime::from_frames(7, vfr)); + + shared_ptr playlist (new Playlist); + playlist->add (film, contentB); + Empty black (film, playlist, bind(&has_video, _1)); + BOOST_REQUIRE_EQUAL (black._periods.size(), 1); + BOOST_CHECK (black._periods.front().from == DCPTime::from_frames(0, vfr)); + BOOST_CHECK (black._periods.front().to == DCPTime::from_frames(7, vfr)); + + /* position should initially be the start of the first empty period */ + BOOST_CHECK (black.position() == DCPTime::from_frames(0, vfr)); +} + -- 2.30.2