From 1aad2c33896ce6222f3c929c7af7fe4ff5fda0f2 Mon Sep 17 00:00:00 2001 From: Carl Hetherington Date: Tue, 16 Jan 2018 21:01:30 +0000 Subject: [PATCH] In general the player assumes that it won't receive out of order video. This clearly can happen with separate L/R sources. A pass in L might emit two frames which means the arrivals can't possibly be in order. This commit fixes this by introducing a Shuffler which all alternate-3D sources send their video to. The Shuffler re-orders things before they arrive at the player. It also fixes the code which inserts video frames before one that arrives after a gap. This didn't cope with 3D right before. The audio code solves a similar (perhaps the same?) problem with the AudioMerger; perhaps we should have a similar thing for video and make the player emit complete 3D frames. Should help with #976. --- src/lib/piece.h | 1 + src/lib/player.cc | 72 ++++++++++++++++++++----- src/lib/player.h | 8 ++- src/lib/shuffler.cc | 93 ++++++++++++++++++++++++++++++++ src/lib/shuffler.h | 42 +++++++++++++++ src/lib/util.cc | 10 ++++ src/lib/util.h | 1 + src/lib/wscript | 1 + test/shuffler_test.cc | 122 ++++++++++++++++++++++++++++++++++++++++++ test/wscript | 1 + 10 files changed, 336 insertions(+), 15 deletions(-) create mode 100644 src/lib/shuffler.cc create mode 100644 src/lib/shuffler.h create mode 100644 test/shuffler_test.cc diff --git a/src/lib/piece.h b/src/lib/piece.h index 711e292ee..440beecf4 100644 --- a/src/lib/piece.h +++ b/src/lib/piece.h @@ -22,6 +22,7 @@ #define DCPOMATIC_PIECE_H #include "types.h" +#include "frame_rate_change.h" class Content; class Decoder; diff --git a/src/lib/player.cc b/src/lib/player.cc index 553921726..522b8304f 100644 --- a/src/lib/player.cc +++ b/src/lib/player.cc @@ -1,5 +1,5 @@ /* - Copyright (C) 2013-2017 Carl Hetherington + Copyright (C) 2013-2018 Carl Hetherington This file is part of DCP-o-matic. @@ -48,6 +48,7 @@ #include "dcp_decoder.h" #include "image_decoder.h" #include "compose.hpp" +#include "shuffler.h" #include #include #include @@ -87,6 +88,7 @@ Player::Player (shared_ptr film, shared_ptr playlist , _fast (false) , _play_referenced (false) , _audio_merger (_film->audio_frame_rate()) + , _shuffler (0) { _film_changed_connection = _film->Changed.connect (bind (&Player::film_changed, this, _1)); _playlist_changed_connection = _playlist->Changed.connect (bind (&Player::playlist_changed, this)); @@ -98,11 +100,20 @@ Player::Player (shared_ptr film, shared_ptr playlist seek (DCPTime (), true); } +Player::~Player () +{ + delete _shuffler; +} + void Player::setup_pieces () { _pieces.clear (); + delete _shuffler; + _shuffler = new Shuffler(); + _shuffler->Video.connect(bind(&Player::video, this, _1, _2)); + BOOST_FOREACH (shared_ptr i, _playlist->content ()) { if (!i->paths_valid ()) { @@ -137,7 +148,11 @@ Player::setup_pieces () _pieces.push_back (piece); if (decoder->video) { - decoder->video->Data.connect (bind (&Player::video, this, weak_ptr (piece), _1)); + if (i->video->frame_type() == VIDEO_FRAME_TYPE_3D_LEFT || i->video->frame_type() == VIDEO_FRAME_TYPE_3D_RIGHT) { + decoder->video->Data.connect (bind (&Shuffler::video, _shuffler, weak_ptr(piece), _1)); + } else { + decoder->video->Data.connect (bind (&Player::video, this, weak_ptr(piece), _1)); + } } if (decoder->audio) { @@ -164,6 +179,7 @@ Player::setup_pieces () _silent = Empty (_film->content(), _film->length(), bind(&Content::audio, _1)); _last_video_time = DCPTime (); + _last_video_eyes = EYES_BOTH; _last_audio_time = DCPTime (); _have_valid_pieces = true; } @@ -309,7 +325,7 @@ Player::transform_image_subtitles (list subs) const } shared_ptr -Player::black_player_video_frame () const +Player::black_player_video_frame (Eyes eyes) const { return shared_ptr ( new PlayerVideo ( @@ -318,7 +334,7 @@ Player::black_player_video_frame () const optional (), _video_container_size, _video_container_size, - EYES_BOTH, + eyes, PART_WHOLE, PresetColourConversion::all().front().conversion ) @@ -516,7 +532,7 @@ Player::pass () if (_playlist->length() == DCPTime()) { /* Special case of an empty Film; just give one black frame */ - emit_video (black_player_video_frame(), DCPTime()); + emit_video (black_player_video_frame(EYES_BOTH), DCPTime()); return true; } @@ -534,6 +550,7 @@ Player::pass () if (t > i->content->end()) { i->done = true; } else { + /* Given two choices at the same time, pick the one with a subtitle so we see it before the video. */ @@ -572,7 +589,7 @@ Player::pass () earliest_content->done = earliest_content->decoder->pass (); break; case BLACK: - emit_video (black_player_video_frame(), _black.position()); + emit_video (black_player_video_frame(EYES_BOTH), _black.position()); _black.set_position (_black.position() + one_video_frame()); break; case SILENT: @@ -622,6 +639,9 @@ Player::pass () emit_audio (i->first, i->second); } + if (done) { + _shuffler->flush (); + } return done; } @@ -677,14 +697,33 @@ Player::video (weak_ptr wp, ContentVideo video) /* Fill gaps that we discover now that we have some video which needs to be emitted */ if (_last_video_time) { - /* XXX: this may not work for 3D */ DCPTime fill_from = max (*_last_video_time, piece->content->position()); - for (DCPTime j = fill_from; j < time; j += one_video_frame()) { - LastVideoMap::const_iterator k = _last_video.find (wp); - if (k != _last_video.end ()) { - emit_video (k->second, j); - } else { - emit_video (black_player_video_frame(), j); + LastVideoMap::const_iterator last = _last_video.find (wp); + if (_film->three_d()) { + DCPTime j = fill_from; + Eyes eyes = _last_video_eyes.get_value_or(EYES_LEFT); + if (eyes == EYES_BOTH) { + eyes = EYES_LEFT; + } + while (j < time || eyes != video.eyes) { + if (last != _last_video.end()) { + last->second->set_eyes (eyes); + emit_video (last->second, j); + } else { + emit_video (black_player_video_frame(eyes), j); + } + if (eyes == EYES_RIGHT) { + j += one_video_frame(); + } + eyes = increment_eyes (eyes); + } + } else { + for (DCPTime j = fill_from; j < time; j += one_video_frame()) { + if (last != _last_video.end()) { + emit_video (last->second, j); + } else { + emit_video (black_player_video_frame(EYES_BOTH), j); + } } } } @@ -872,6 +911,10 @@ Player::seek (DCPTime time, bool accurate) setup_pieces (); } + if (_shuffler) { + _shuffler->clear (); + } + if (_audio_processor) { _audio_processor->flush (); } @@ -896,9 +939,11 @@ Player::seek (DCPTime time, bool accurate) if (accurate) { _last_video_time = time; + _last_video_eyes = EYES_LEFT; _last_audio_time = time; } else { _last_video_time = optional(); + _last_video_eyes = optional(); _last_audio_time = optional(); } @@ -922,6 +967,7 @@ Player::emit_video (shared_ptr pv, DCPTime time) _last_video_time = time + one_video_frame(); _active_subtitles.clear_before (time); } + _last_video_eyes = increment_eyes (pv->eyes()); } void diff --git a/src/lib/player.h b/src/lib/player.h index 7cc3b0ef7..0b8540c15 100644 --- a/src/lib/player.h +++ b/src/lib/player.h @@ -1,5 +1,5 @@ /* - Copyright (C) 2013-2015 Carl Hetherington + Copyright (C) 2013-2018 Carl Hetherington This file is part of DCP-o-matic. @@ -46,6 +46,7 @@ class Playlist; class Font; class AudioBuffers; class ReferencedReelAsset; +class Shuffler; /** @class Player * @brief A class which can `play' a Playlist. @@ -54,6 +55,7 @@ class Player : public boost::enable_shared_from_this, public boost::nonc { public: Player (boost::shared_ptr, boost::shared_ptr playlist); + ~Player (); bool pass (); void seek (DCPTime time, bool accurate); @@ -105,7 +107,7 @@ private: DCPTime resampled_audio_to_dcp (boost::shared_ptr piece, Frame f) const; ContentTime dcp_to_content_time (boost::shared_ptr piece, DCPTime t) const; DCPTime content_time_to_dcp (boost::shared_ptr piece, ContentTime t) const; - boost::shared_ptr black_player_video_frame () const; + boost::shared_ptr black_player_video_frame (Eyes eyes) const; void video (boost::weak_ptr, ContentVideo); void audio (boost::weak_ptr, AudioStreamPtr, ContentAudio); void image_subtitle_start (boost::weak_ptr, ContentImageSubtitle); @@ -146,6 +148,7 @@ private: /** Time just after the last video frame we emitted, or the time of the last accurate seek */ boost::optional _last_video_time; + boost::optional _last_video_eyes; /** Time just after the last audio frame we emitted, or the time of the last accurate seek */ boost::optional _last_audio_time; @@ -155,6 +158,7 @@ private: LastVideoMap _last_video; AudioMerger _audio_merger; + Shuffler* _shuffler; class StreamState { diff --git a/src/lib/shuffler.cc b/src/lib/shuffler.cc new file mode 100644 index 000000000..997d91fb1 --- /dev/null +++ b/src/lib/shuffler.cc @@ -0,0 +1,93 @@ +/* + Copyright (C) 2018 Carl Hetherington + + This file is part of DCP-o-matic. + + DCP-o-matic 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. + + DCP-o-matic 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 DCP-o-matic. If not, see . + +*/ + +#include "shuffler.h" +#include "content_video.h" +#include "dcpomatic_assert.h" +#include +#include + +using std::make_pair; +using boost::weak_ptr; +using boost::shared_ptr; +using boost::optional; + +struct Comparator +{ + bool operator()(Shuffler::Store const & a, Shuffler::Store const & b) { + if (a.second.frame != b.second.frame) { + return a.second.frame < b.second.frame; + } + return a.second.eyes < b.second.eyes; + } +}; + +void +Shuffler::video (weak_ptr weak_piece, ContentVideo video) +{ + std::cout << "shuffler gets " << video.frame << " " << video.eyes << "\n"; + + /* Something has gong wrong if our store gets too big */ + DCPOMATIC_ASSERT (_store.size() != 8); + /* We should only ever see 3D_LEFT / 3D_RIGHT */ + DCPOMATIC_ASSERT (video.eyes == EYES_LEFT || video.eyes == EYES_RIGHT); + + shared_ptr piece = weak_piece.lock (); + DCPOMATIC_ASSERT (piece); + + if (!_last) { + /* We haven't seen anything since the last clear() so assume everything is OK */ + Video (weak_piece, video); + _last = video; + return; + } + + _store.push_back (make_pair (weak_piece, video)); + _store.sort (Comparator()); + + while ( + !_store.empty() && + ( + (_store.front().second.frame == _last->frame && _store.front().second.eyes == EYES_RIGHT && _last->eyes == EYES_LEFT) || + (_store.front().second.frame == (_last->frame + 1) && _store.front().second.eyes == EYES_LEFT && _last->eyes == EYES_RIGHT) || + _store.size() > 8 + ) + ) { + + std::cout << "shuffler emits " << _store.front().second.frame << " " << _store.front().second.eyes << "\n"; + Video (_store.front().first, _store.front().second); + _last = _store.front().second; + _store.pop_front (); + } +} + +void +Shuffler::clear () +{ + _last = optional(); +} + +void +Shuffler::flush () +{ + BOOST_FOREACH (Store i, _store) { + Video (i.first, i.second); + } +} diff --git a/src/lib/shuffler.h b/src/lib/shuffler.h new file mode 100644 index 000000000..54e13d486 --- /dev/null +++ b/src/lib/shuffler.h @@ -0,0 +1,42 @@ +/* + Copyright (C) 2018 Carl Hetherington + + This file is part of DCP-o-matic. + + DCP-o-matic 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. + + DCP-o-matic 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 DCP-o-matic. If not, see . + +*/ + +#include "types.h" +#include "content_video.h" +#include + +class Piece; + +class Shuffler +{ +public: + void clear (); + void flush (); + + void video (boost::weak_ptr, ContentVideo video); + boost::signals2::signal, ContentVideo)> Video; + + typedef std::pair, ContentVideo> Store; + +private: + + boost::optional _last; + std::list _store; +}; diff --git a/src/lib/util.cc b/src/lib/util.cc index 637030e86..8ed878561 100644 --- a/src/lib/util.cc +++ b/src/lib/util.cc @@ -774,3 +774,13 @@ remap (shared_ptr input, int output_channels, AudioMapping m return mapped; } + +Eyes +increment_eyes (Eyes e) +{ + if (e == EYES_LEFT) { + return EYES_RIGHT; + } + + return EYES_LEFT; +} diff --git a/src/lib/util.h b/src/lib/util.h index 1d4b01358..53a0a2d66 100644 --- a/src/lib/util.h +++ b/src/lib/util.h @@ -92,5 +92,6 @@ extern float relaxed_string_to_float (std::string); extern std::string careful_string_filter (std::string); extern std::pair audio_channel_types (std::list mapped, int channels); extern boost::shared_ptr remap (boost::shared_ptr input, int output_channels, AudioMapping map); +extern Eyes increment_eyes (Eyes e); #endif diff --git a/src/lib/wscript b/src/lib/wscript index 7fa32c7e7..ae8ab30cb 100644 --- a/src/lib/wscript +++ b/src/lib/wscript @@ -129,6 +129,7 @@ sources = """ send_kdm_email_job.cc send_problem_report_job.cc server.cc + shuffler.cc string_log_entry.cc subtitle_content.cc subtitle_decoder.cc diff --git a/test/shuffler_test.cc b/test/shuffler_test.cc new file mode 100644 index 000000000..879f2e079 --- /dev/null +++ b/test/shuffler_test.cc @@ -0,0 +1,122 @@ +#include "lib/shuffler.h" +#include "lib/piece.h" +#include "lib/content_video.h" +#include + +using std::list; +using boost::shared_ptr; +using boost::weak_ptr; +using boost::optional; + +static void +push (Shuffler& s, int frame, Eyes eyes) +{ + shared_ptr piece (new Piece (shared_ptr(), shared_ptr(), FrameRateChange(24, 24))); + ContentVideo cv; + cv.frame = frame; + cv.eyes = eyes; + s.video (piece, cv); +} + +list pending_cv; + +static void +receive (weak_ptr, ContentVideo cv) +{ + pending_cv.push_back (cv); +} + +static void +check (int frame, Eyes eyes, int line) +{ + BOOST_REQUIRE_MESSAGE (!pending_cv.empty(), "Check at " << line << " failed."); + BOOST_CHECK_MESSAGE (pending_cv.front().frame == frame, "Check at " << line << " failed."); + BOOST_CHECK_MESSAGE (pending_cv.front().eyes == eyes, "Check at " << line << " failed."); + pending_cv.pop_front(); +} + +/** A perfect sequence */ +BOOST_AUTO_TEST_CASE (shuffler_test1) +{ + Shuffler s; + s.Video.connect (boost::bind (&receive, _1, _2)); + + for (int i = 0; i < 10; ++i) { + push (s, i, EYES_LEFT); + push (s, i, EYES_RIGHT); + check (i, EYES_LEFT, __LINE__); + check (i, EYES_RIGHT, __LINE__); + } +} + +/** Everything present but some simple shuffling needed */ +BOOST_AUTO_TEST_CASE (shuffler_test2) +{ + Shuffler s; + s.Video.connect (boost::bind (&receive, _1, _2)); + + for (int i = 0; i < 10; i += 2) { + push (s, i, EYES_LEFT); + push (s, i + 1, EYES_LEFT); + push (s, i, EYES_RIGHT); + push (s, i + 1, EYES_RIGHT); + check (i, EYES_LEFT, __LINE__); + check (i, EYES_RIGHT, __LINE__); + check (i + 1, EYES_LEFT, __LINE__); + check (i + 1, EYES_RIGHT, __LINE__); + } +} + +/** One missing left eye image */ +BOOST_AUTO_TEST_CASE (shuffler_test3) +{ + Shuffler s; + s.Video.connect (boost::bind (&receive, _1, _2)); + + push (s, 0, EYES_LEFT); + check (0, EYES_LEFT, __LINE__); + push (s, 0, EYES_RIGHT); + check (0, EYES_RIGHT, __LINE__); + push (s, 1, EYES_LEFT); + check (1, EYES_LEFT, __LINE__); + push (s, 1, EYES_RIGHT); + check (1, EYES_RIGHT, __LINE__); + push (s, 2, EYES_RIGHT); + push (s, 3, EYES_LEFT); + push (s, 3, EYES_RIGHT); + push (s, 4, EYES_LEFT); + push (s, 4, EYES_RIGHT); + s.flush (); + check (2, EYES_RIGHT, __LINE__); + check (3, EYES_LEFT, __LINE__); + check (3, EYES_RIGHT, __LINE__); + check (4, EYES_LEFT, __LINE__); + check (4, EYES_RIGHT, __LINE__); +} + +/** One missing right eye image */ +BOOST_AUTO_TEST_CASE (shuffler_test4) +{ + Shuffler s; + s.Video.connect (boost::bind (&receive, _1, _2)); + + push (s, 0, EYES_LEFT); + check (0, EYES_LEFT, __LINE__); + push (s, 0, EYES_RIGHT); + check (0, EYES_RIGHT, __LINE__); + push (s, 1, EYES_LEFT); + check (1, EYES_LEFT, __LINE__); + push (s, 1, EYES_RIGHT); + check (1, EYES_RIGHT, __LINE__); + push (s, 2, EYES_LEFT); + push (s, 3, EYES_LEFT); + push (s, 3, EYES_RIGHT); + push (s, 4, EYES_LEFT); + push (s, 4, EYES_RIGHT); + s.flush (); + check (2, EYES_LEFT, __LINE__); + check (3, EYES_LEFT, __LINE__); + check (3, EYES_RIGHT, __LINE__); + check (4, EYES_LEFT, __LINE__); + check (4, EYES_RIGHT, __LINE__); +} diff --git a/test/wscript b/test/wscript index f0533cfa1..fef16dbed 100644 --- a/test/wscript +++ b/test/wscript @@ -93,6 +93,7 @@ def build(bld): render_subtitles_test.cc scaling_test.cc silence_padding_test.cc + shuffler_test.cc skip_frame_test.cc srt_subtitle_test.cc ssa_subtitle_test.cc -- 2.30.2