From 6e003ef110717dd3e4ecdb009d33671f7834e024 Mon Sep 17 00:00:00 2001 From: Carl Hetherington Date: Sat, 18 Apr 2020 20:42:58 +0200 Subject: [PATCH] Add _last_written to Writer, containing the last written frame and eyes to each reel. This is updated when things are popped off the queue, with _state_mutex_held, and used in preference to the ones in ReelWriter which were previously being updated during the time the _state_mutex lock is unlocked in the body of Writer::thread(). This was not thread safe (thanks, valgrind!) --- src/lib/reel_writer.cc | 10 +--------- src/lib/reel_writer.h | 13 +------------ src/lib/writer.cc | 35 +++++++++++++++++++++++++---------- src/lib/writer.h | 24 ++++++++++++++++++++++++ 4 files changed, 51 insertions(+), 31 deletions(-) diff --git a/src/lib/reel_writer.cc b/src/lib/reel_writer.cc index 49e09663f..06d20e7c8 100644 --- a/src/lib/reel_writer.cc +++ b/src/lib/reel_writer.cc @@ -71,8 +71,6 @@ ReelWriter::ReelWriter ( ) : _film (film) , _period (period) - , _last_written_video_frame (-1) - , _last_written_eyes (EYES_RIGHT) , _reel_index (reel_index) , _reel_count (reel_count) , _content_summary (content_summary) @@ -266,12 +264,10 @@ ReelWriter::write (optional encoded, Frame frame, Eyes eyes) dcp::FrameInfo fin = _picture_asset_writer->write (encoded->data().get (), encoded->size()); write_frame_info (frame, eyes, fin); _last_written[eyes] = encoded; - _last_written_video_frame = frame; - _last_written_eyes = eyes; } void -ReelWriter::fake_write (Frame frame, Eyes eyes, int size) +ReelWriter::fake_write (int size) { if (!_picture_asset_writer) { /* We're not writing any data */ @@ -279,8 +275,6 @@ ReelWriter::fake_write (Frame frame, Eyes eyes, int size) } _picture_asset_writer->fake_write (size); - _last_written_video_frame = frame; - _last_written_eyes = eyes; } void @@ -296,8 +290,6 @@ ReelWriter::repeat_write (Frame frame, Eyes eyes) _last_written[eyes]->size() ); write_frame_info (frame, eyes, fin); - _last_written_video_frame = frame; - _last_written_eyes = eyes; } void diff --git a/src/lib/reel_writer.h b/src/lib/reel_writer.h index d241c0fac..0b5a3dad5 100644 --- a/src/lib/reel_writer.h +++ b/src/lib/reel_writer.h @@ -64,7 +64,7 @@ public: ); void write (boost::optional encoded, Frame frame, Eyes eyes); - void fake_write (Frame frame, Eyes eyes, int size); + void fake_write (int size); void repeat_write (Frame frame, Eyes eyes); void write (boost::shared_ptr audio); void write (PlayerText text, TextType type, boost::optional track, dcpomatic::DCPTimePeriod period); @@ -79,14 +79,6 @@ public: return _period; } - int last_written_video_frame () const { - return _last_written_video_frame; - } - - Eyes last_written_eyes () const { - return _last_written_eyes; - } - int first_nonexistant_frame () const { return _first_nonexistant_frame; } @@ -109,9 +101,6 @@ private: int _first_nonexistant_frame; /** the data of the last written frame, if there is one */ boost::optional _last_written[EYES_COUNT]; - /** the index of the last written video frame within the reel */ - int _last_written_video_frame; - Eyes _last_written_eyes; /** index of this reel within the DCP (starting from 0) */ int _reel_index; /** number of reels in the DCP */ diff --git a/src/lib/writer.cc b/src/lib/writer.cc index 6d1886406..d346c4a4f 100644 --- a/src/lib/writer.cc +++ b/src/lib/writer.cc @@ -87,6 +87,8 @@ Writer::Writer (shared_ptr film, weak_ptr j) _reels.push_back (ReelWriter (film, p, job, reel_index++, reels.size(), _film->content_summary(p))); } + _last_written.resize (reels.size()); + /* We can keep track of the current audio, subtitle and closed caption reels easily because audio and captions arrive to the Writer in sequence. This is not so for video. */ @@ -300,7 +302,8 @@ Writer::write (shared_ptr audio, DCPTime const time) } } -/** This must be called from Writer::thread() with an appropriate lock held */ + +/** Caller must hold a lock on _state_mutex */ bool Writer::have_sequenced_image_at_queue_head () { @@ -309,30 +312,41 @@ Writer::have_sequenced_image_at_queue_head () } _queue.sort (); - QueueItem const & f = _queue.front(); - ReelWriter const & reel = _reels[f.reel]; + return _last_written[f.reel].next(f); +} - /* The queue should contain only EYES_LEFT/EYES_RIGHT pairs or EYES_BOTH */ - if (f.eyes == EYES_BOTH) { +bool +Writer::LastWritten::next (QueueItem qi) const +{ + if (qi.eyes == EYES_BOTH) { /* 2D */ - return f.frame == (reel.last_written_video_frame() + 1); + return qi.frame == (_frame + 1); } /* 3D */ - if (reel.last_written_eyes() == EYES_LEFT && f.frame == reel.last_written_video_frame() && f.eyes == EYES_RIGHT) { + if (_eyes == EYES_LEFT && qi.frame == _frame && qi.eyes == EYES_RIGHT) { return true; } - if (reel.last_written_eyes() == EYES_RIGHT && f.frame == (reel.last_written_video_frame() + 1) && f.eyes == EYES_LEFT) { + if (_eyes == EYES_RIGHT && qi.frame == (_frame + 1) && qi.eyes == EYES_LEFT) { return true; } return false; } + +void +Writer::LastWritten::update (QueueItem qi) +{ + _frame = qi.frame; + _eyes = qi.eyes; +} + + void Writer::thread () try @@ -381,6 +395,7 @@ try /* Write any frames that we can write; i.e. those that are in sequence. */ while (have_sequenced_image_at_queue_head ()) { QueueItem qi = _queue.front (); + _last_written[qi.reel].update (qi); _queue.pop_front (); if (qi.type == QueueItem::FULL && qi.encoded) { --_queued_full_in_memory; @@ -401,7 +416,7 @@ try break; case QueueItem::FAKE: LOG_DEBUG_ENCODE (N_("Writer FAKE-writes %1"), qi.frame); - reel.fake_write (qi.frame, qi.eyes, qi.size); + reel.fake_write (qi.size); ++_fake_written; break; case QueueItem::REPEAT: @@ -430,7 +445,7 @@ try DCPOMATIC_ASSERT (i != _queue.rend()); ++_pushed_to_disk; /* For the log message below */ - int const awaiting = _reels[_queue.front().reel].last_written_video_frame() + 1; + int const awaiting = _last_written[_queue.front().reel].frame() + 1; lock.unlock (); /* i is valid here, even though we don't hold a lock on the mutex, diff --git a/src/lib/writer.h b/src/lib/writer.h index d304133dc..d09b06264 100644 --- a/src/lib/writer.h +++ b/src/lib/writer.h @@ -151,6 +151,30 @@ private: int _maximum_frames_in_memory; unsigned int _maximum_queue_size; + class LastWritten + { + public: + LastWritten() + : _frame(-1) + , _eyes(EYES_RIGHT) + {} + + /** @return true if qi is the next item after this one */ + bool next (QueueItem qi) const; + void update (QueueItem qi); + + int frame () const { + return _frame; + } + + private: + int _frame; + Eyes _eyes; + }; + + /** The last frame written to each reel */ + std::vector _last_written; + /** number of FULL written frames */ int _full_written; /** number of FAKE written frames */ -- 2.30.2