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.

diff --git a/src/lib/audio_decoder_stream.cc b/src/lib/audio_decoder_stream.cc
index b7b96ddd4ad88e17b2b03057559f283a04e617da..c14dc654e904cd611d961e4941f1356a1f8cdc17 100644 (file)
--- 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<Frame> 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 5d73a9bbfa32066832f41748bdc3de2beb423370..38c2a7ccfa2bdcab2bb96ecf850eed4de53ba5e4 100644 (file)
--- 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 e3f724d936ef782ae945911c16d9582ce112497b..d4e1a7fa29ce7d1cacf061c95256268f21ddfffb 100644 (file)
--- 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 (file)
index 0000000..cba674d
--- /dev/null
+++ b/src/lib/decoder.cc
@@ -0,0 +1,38 @@
+/*
+    Copyright (C) 2012-2015 Carl Hetherington <cth@carlh.net>
+
+    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 <http://www.gnu.org/licenses/>.
+
+*/
+
+#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 181fc6c2ab8869dc96f5269600f69017d362d631..84661ee8c4a1637b79a2796cce5ad1083f142e44 100644 (file)
--- 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<ContentTime> _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 f0500a6296965bec6a43816de589a49f999e0ed4..e2c200d7713224ae0c978ca5e8477f8ed4c318df 100644 (file)
--- 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<ImageProxy> (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 76755c1fcc2d6e1a3c75b202d5b2d5dc6c266d1e..dfe1f26940c59d6477f96bfb217dd0db01d3681c 100644 (file)
--- 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<FFmpegAudioStream> stream) const;
        int bytes_per_audio_sample (boost::shared_ptr<FFmpegAudioStream> stream) const;

diff --git a/src/lib/ffmpeg_subtitle_stream.cc b/src/lib/ffmpeg_subtitle_stream.cc
index cf3a8c0f2a1d2660ff8eec9a56a5acc8632b574c..627b0fef1c8b61b6890397c97282f4d42f7700af 100644 (file)
--- 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 f7afbc0a153b86cbabc77d88ba0c3f4d4bf1684d..41a4949ba34bc8397db266d746b7e08163e3b252 100644 (file)
--- 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 307226da6b184a25e12585dd14b2d83c4880c0a2..4be5c96b36c7b28b8009e1bb850d9dc271ffd5d3 100644 (file)
--- a/src/lib/subtitle_decoder.cc
+++ b/src/lib/subtitle_decoder.cc
@@ -90,9 +90,21 @@ SubtitleDecoder::get (list<T> const & subs, list<ContentTimePeriod> const & sp,
                return list<T> ();
        }
 
-       /* 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<ContentTime> missing;
+       BOOST_FOREACH (ContentTimePeriod i, sp) {
+               typename list<T>::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 05bde829cb6237dcb4a02d7d14ff4ab6372278bf..e29bf580992f534d4056e4780fa0b7008d86b915 100644 (file)
--- 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 0d7cbfe2e3c745cdfd45ebc1156f0bce924f8308..f240640d02ad4858e686c8ff55b88ab487e4c6fe 100644 (file)
--- 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<ContentVideo>::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 dc4f8d60b6882b23b255240cc4e23359bd2dcfbc..84aec869a2a2292c6883e97a9590622524072c5b 100644 (file)
--- 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 b43443bd13d5f87d1ed66e1154619706160a8419..4d63b9be3464a3e9acc4ee890f57ebfc44ed66a9 100644 (file)
--- 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