From 97d39f46795af78b84d5f7bc9118a188f2864781 Mon Sep 17 00:00:00 2001 From: Carl Hetherington Date: Fri, 7 Oct 2016 16:22:38 +0100 Subject: [PATCH] A possibly-better approach to seeking. Before this commit, decoders try to guess whether they should request a seek based on what they have in their buffers. This seems reasonable for video and audio, which will always (I think) have some data lying around to give an indication of where their parent decoders are in the timeline. It doesn't work so well for subtitles, as the storage of subs is cleared out based on time (+/- 5s of "now") so there is a good chance that the storage will be empty. This gives the subtitle decoder no chance of knowing where its parent is, so it's very likely to seek. This commit asks the parent decoder to seek if it wants to, and it decides based on a knowledge of roughly where it is in the timeline. Hence the sub-decoders just see if they have got the data that is being requested, and if not they suggest to the parent that it might like to seek. They then start calling pass(). Hence the parent should only seek if some calls to pass() are not going to elicit the required data in a reasonable time. --- src/lib/audio_decoder_stream.cc | 30 ++++++++++++++---------- src/lib/dcp_decoder.cc | 1 + src/lib/dcp_subtitle_decoder.cc | 1 + src/lib/decoder.cc | 38 +++++++++++++++++++++++++++++++ src/lib/decoder.h | 22 +++++++++++------- src/lib/ffmpeg_decoder.cc | 17 ++++++++++++++ src/lib/ffmpeg_decoder.h | 1 + src/lib/ffmpeg_subtitle_stream.cc | 1 - src/lib/image_decoder.cc | 1 + src/lib/subtitle_decoder.cc | 18 ++++++++++++--- src/lib/text_subtitle_decoder.cc | 4 +++- src/lib/video_decoder.cc | 10 +++++--- src/lib/video_mxf_decoder.cc | 1 + src/lib/wscript | 1 + 14 files changed, 118 insertions(+), 28 deletions(-) create mode 100644 src/lib/decoder.cc diff --git a/src/lib/audio_decoder_stream.cc b/src/lib/audio_decoder_stream.cc index b7b96ddd4..c14dc654e 100644 --- a/src/lib/audio_decoder_stream.cc +++ b/src/lib/audio_decoder_stream.cc @@ -70,17 +70,23 @@ AudioDecoderStream::get (Frame frame, Frame length, bool accurate) _log->log (String::compose ("-> ADS has request for %1 %2", frame, length), LogEntry::TYPE_DEBUG_DECODE); - Frame const end = frame + length - 1; - - /* If we are less than (about) 5 seconds behind the data that we want we'll - run through it rather than seeking. - */ - Frame const slack = 5 * 48000; + Frame const from = frame; + Frame const to = from + length; + Frame const have_from = _decoded.frame; + Frame const have_to = _decoded.frame + _decoded.audio->frames(); + + optional missing; + if (have_from > from || have_to < to) { + /* We need something */ + if (have_from < from && from < have_to) { + missing = have_to; + } else { + missing = from; + } + } - if (frame < _decoded.frame || end > (_decoded.frame + _decoded.audio->frames() + slack)) { - /* Either we have no decoded data, all our data is after the time that we - want, or what we do have is a long way from what we want: seek */ - _decoder->seek (ContentTime::from_frames (frame, _content->resampled_frame_rate()), accurate); + if (missing) { + _decoder->maybe_seek (ContentTime::from_frames (*missing, _content->resampled_frame_rate()), accurate); } /* Offset of the data that we want from the start of _decoded.audio @@ -98,7 +104,7 @@ AudioDecoderStream::get (Frame frame, Frame length, bool accurate) if (accurate) { /* Keep stuffing data into _decoded until we have enough data, or the subclass does not want to give us any more */ while ( - (_decoded.frame > frame || (_decoded.frame + _decoded.audio->frames()) < end) && + (_decoded.frame > frame || (_decoded.frame + _decoded.audio->frames()) <= to) && !_decoder->pass (Decoder::PASS_REASON_AUDIO, accurate) ) {} @@ -106,7 +112,7 @@ AudioDecoderStream::get (Frame frame, Frame length, bool accurate) decoded_offset = frame - _decoded.frame; _log->log ( - String::compose ("Accurate ADS::get has offset %1 from request %2 and available %3", decoded_offset, frame, _decoded.frame), + String::compose ("Accurate ADS::get has offset %1 from request %2 and available %3", decoded_offset, frame, have_from), LogEntry::TYPE_DEBUG_DECODE ); } else { diff --git a/src/lib/dcp_decoder.cc b/src/lib/dcp_decoder.cc index 5d73a9bbf..38c2a7ccf 100644 --- a/src/lib/dcp_decoder.cc +++ b/src/lib/dcp_decoder.cc @@ -163,6 +163,7 @@ DCPDecoder::pass (PassReason reason, bool) } } + _position = _next; _next += ContentTime::from_frames (1, vfr); if ((*_reel)->main_picture ()) { diff --git a/src/lib/dcp_subtitle_decoder.cc b/src/lib/dcp_subtitle_decoder.cc index e3f724d93..d4e1a7fa2 100644 --- a/src/lib/dcp_subtitle_decoder.cc +++ b/src/lib/dcp_subtitle_decoder.cc @@ -79,6 +79,7 @@ DCPSubtitleDecoder::pass (PassReason, bool) } subtitle->give_text (p, s); + _position = p.from; return false; } diff --git a/src/lib/decoder.cc b/src/lib/decoder.cc new file mode 100644 index 000000000..cba674d04 --- /dev/null +++ b/src/lib/decoder.cc @@ -0,0 +1,38 @@ +/* + Copyright (C) 2012-2015 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 "decoder.h" + +void +Decoder::maybe_seek (ContentTime time, bool accurate) +{ + if (!_position) { + /* A seek has just happened */ + return; + } + + if (time >= *_position && time < (*_position + ContentTime::from_seconds(1))) { + /* No need to seek: caller should just pass() */ + return; + } + + _position.reset (); + seek (time, accurate); +} diff --git a/src/lib/decoder.h b/src/lib/decoder.h index 181fc6c2a..84661ee8c 100644 --- a/src/lib/decoder.h +++ b/src/lib/decoder.h @@ -52,6 +52,20 @@ public: PASS_REASON_SUBTITLE }; + void maybe_seek (ContentTime time, bool accurate); + + /** @return true if this decoder has already returned all its data and will give no more */ + virtual bool pass (PassReason, bool accurate) = 0; + + /** Ensure that any future get() calls return data that reflect + * changes in our content's settings. + */ + virtual void reset () {} + +protected: + boost::optional _position; + +private: /** Seek so that the next pass() will yield the next thing * (video/sound frame, subtitle etc.) at or after the requested * time. Pass accurate = true to try harder to ensure that, at worst, @@ -61,14 +75,6 @@ public: * it may seek to just the right spot. */ virtual void seek (ContentTime time, bool accurate) = 0; - - /** @return true if this decoder has already returned all its data and will give no more */ - virtual bool pass (PassReason, bool accurate) = 0; - - /** Ensure that any future get() calls return data that reflect - * changes in our content's settings. - */ - virtual void reset () {} }; #endif diff --git a/src/lib/ffmpeg_decoder.cc b/src/lib/ffmpeg_decoder.cc index f0500a629..e2c200d77 100644 --- a/src/lib/ffmpeg_decoder.cc +++ b/src/lib/ffmpeg_decoder.cc @@ -425,6 +425,8 @@ FFmpegDecoder::decode_audio_packet () LOG_WARNING ("Crazy timestamp %s", to_string (ct)); } + update_position (ct); + /* Give this data provided there is some, and its time is sane */ if (ct >= ContentTime() && data->frames() > 0) { audio->give (*stream, data, ct); @@ -476,6 +478,7 @@ FFmpegDecoder::decode_video_packet () shared_ptr (new RawImageProxy (image)), llrint (pts * _ffmpeg_content->active_video_frame_rate ()) ); + update_position (ContentTime::from_seconds (pts)); } else { LOG_WARNING_NC ("Dropping frame without PTS"); } @@ -506,6 +509,7 @@ FFmpegDecoder::decode_subtitle_packet () FFmpegSubtitlePeriod sub_period = subtitle_period (sub); ContentTimePeriod period; period.from = sub_period.from + _pts_offset; + update_position (period.from); if (sub_period.to) { /* We already know the subtitle period `to' time */ period.to = sub_period.to.get() + _pts_offset; @@ -639,3 +643,16 @@ FFmpegDecoder::decode_ass_subtitle (string ass, ContentTimePeriod period) subtitle->give_text (period, i); } } + +void +FFmpegDecoder::update_position (ContentTime p) +{ + /* _position should err on the side of being too big, as then there is less + chance that we will erroneously decide not to seek when _position > request. + */ + if (!_position) { + _position = p; + } else { + _position = max (*_position, p); + } +} diff --git a/src/lib/ffmpeg_decoder.h b/src/lib/ffmpeg_decoder.h index 76755c1fc..dfe1f2694 100644 --- a/src/lib/ffmpeg_decoder.h +++ b/src/lib/ffmpeg_decoder.h @@ -52,6 +52,7 @@ private: bool pass (PassReason, bool accurate); void seek (ContentTime time, bool); void flush (); + void update_position (ContentTime p); AVSampleFormat audio_sample_format (boost::shared_ptr stream) const; int bytes_per_audio_sample (boost::shared_ptr stream) const; diff --git a/src/lib/ffmpeg_subtitle_stream.cc b/src/lib/ffmpeg_subtitle_stream.cc index cf3a8c0f2..627b0fef1 100644 --- a/src/lib/ffmpeg_subtitle_stream.cc +++ b/src/lib/ffmpeg_subtitle_stream.cc @@ -128,7 +128,6 @@ FFmpegSubtitleStream::add_image_subtitle (string id, ContentTimePeriod period) void FFmpegSubtitleStream::add_text_subtitle (string id, ContentTimePeriod period) { - cout << id << " " << to_string(period.from) << " " << to_string(period.to) << "\n"; DCPOMATIC_ASSERT (_text_subtitles.find (id) == _text_subtitles.end ()); _text_subtitles[id] = period; } diff --git a/src/lib/image_decoder.cc b/src/lib/image_decoder.cc index f7afbc0a1..41a4949ba 100644 --- a/src/lib/image_decoder.cc +++ b/src/lib/image_decoder.cc @@ -72,6 +72,7 @@ ImageDecoder::pass (PassReason, bool) } } + _position = ContentTime::from_frames (_video_position, _image_content->active_video_frame_rate ()); video->give (_image, _video_position); ++_video_position; return false; diff --git a/src/lib/subtitle_decoder.cc b/src/lib/subtitle_decoder.cc index 307226da6..4be5c96b3 100644 --- a/src/lib/subtitle_decoder.cc +++ b/src/lib/subtitle_decoder.cc @@ -90,9 +90,21 @@ SubtitleDecoder::get (list const & subs, list const & sp, return list (); } - /* Seek if what we want is before what we have, or a more than a little bit after */ - if (subs.empty() || sp.back().to < subs.front().period().from || sp.front().from > (subs.back().period().to + ContentTime::from_seconds (1))) { - _parent->seek (sp.front().from, true); + /* Find the time of the first subtitle we don't have in subs */ + optional missing; + BOOST_FOREACH (ContentTimePeriod i, sp) { + typename list::const_iterator j = subs.begin(); + while (j != subs.end() && j->period() != i) { + ++j; + } + if (j == subs.end ()) { + missing = i.from; + } + } + + /* Suggest to our parent decoder that it might want to seek if we haven't got what we're being asked for */ + if (missing) { + _parent->maybe_seek (*missing, true); } /* Now enough pass() calls will either: diff --git a/src/lib/text_subtitle_decoder.cc b/src/lib/text_subtitle_decoder.cc index 05bde829c..e29bf5809 100644 --- a/src/lib/text_subtitle_decoder.cc +++ b/src/lib/text_subtitle_decoder.cc @@ -66,7 +66,9 @@ TextSubtitleDecoder::pass (PassReason, bool) return true; } - subtitle->give_text (content_time_period (_subtitles[_next]), _subtitles[_next]); + ContentTimePeriod const p = content_time_period (_subtitles[_next]); + subtitle->give_text (p, _subtitles[_next]); + _position = p.from; ++_next; return false; diff --git a/src/lib/video_decoder.cc b/src/lib/video_decoder.cc index 0d7cbfe2e..f240640d0 100644 --- a/src/lib/video_decoder.cc +++ b/src/lib/video_decoder.cc @@ -86,7 +86,12 @@ VideoDecoder::get (Frame frame, bool accurate) _log->log (String::compose ("VD has request for %1", frame), LogEntry::TYPE_DEBUG_DECODE); - if (_decoded.empty() || frame < _decoded.front().frame.index() || frame > (_decoded.back().frame.index() + 1)) { + /* See if we have frame, and suggest a seek if not */ + list::const_iterator i = _decoded.begin (); + while (i != _decoded.end() && i->frame.index() != frame) { + ++i; + } + if (i == _decoded.end()) { Frame seek_frame = frame; if (_content->video->frame_type() == VIDEO_FRAME_TYPE_3D_ALTERNATE) { /* 3D alternate is a special case as the frame index in the content is not the same @@ -94,8 +99,7 @@ VideoDecoder::get (Frame frame, bool accurate) */ seek_frame *= 2; } - _log->log (String::compose ("VD seeks to %1", seek_frame), LogEntry::TYPE_DEBUG_DECODE); - _parent->seek (ContentTime::from_frames (seek_frame, _content->active_video_frame_rate()), accurate); + _parent->maybe_seek (ContentTime::from_frames (seek_frame, _content->active_video_frame_rate()), accurate); } /* Work out the number of frames that we should return; we diff --git a/src/lib/video_mxf_decoder.cc b/src/lib/video_mxf_decoder.cc index dc4f8d60b..84aec869a 100644 --- a/src/lib/video_mxf_decoder.cc +++ b/src/lib/video_mxf_decoder.cc @@ -89,6 +89,7 @@ VideoMXFDecoder::pass (PassReason, bool) ); } + _position = _next; _next += ContentTime::from_frames (1, vfr); return false; } diff --git a/src/lib/wscript b/src/lib/wscript index b43443bd1..4d63b9be3 100644 --- a/src/lib/wscript +++ b/src/lib/wscript @@ -56,6 +56,7 @@ sources = """ dcp_video.cc dcpomatic_socket.cc dcpomatic_time.cc + decoder.cc decoder_factory.cc digester.cc dolby_cp750.cc -- 2.30.2