Fix incorrect opus seek preroll and flaky pre-skip removal.

media/filters/ffmpeg_demuxer.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/filters/ffmpeg_demuxer.h"
 
 #include <algorithm>
 
 #include "base/base64.h"
 #include "base/bind.h"
@@ skipping 76 matching lines @@
                                          AVStream* stream)
     : demuxer_(demuxer),
       task_runner_(base::ThreadTaskRunnerHandle::Get()),
       stream_(stream),
       type_(UNKNOWN),
       liveness_(LIVENESS_UNKNOWN),
       end_of_stream_(false),
       last_packet_timestamp_(kNoTimestamp()),
       last_packet_duration_(kNoTimestamp()),
       video_rotation_(VIDEO_ROTATION_0),
-      fixup_negative_ogg_timestamps_(false) {
+      fixup_negative_timestamps_(false) {
   DCHECK(demuxer_);
 
   bool is_encrypted = false;
   int rotation = 0;
   AVDictionaryEntry* rotation_entry = NULL;
 
   // Determine our media format.
   switch (stream->codec->codec_type) {
     case AVMEDIA_TYPE_AUDIO:
       type_ = AUDIO;
@@ skipping 179 matching lines @@
     buffer->set_duration(kNoTimestamp());
   }
 
   // Note: If pts is AV_NOPTS_VALUE, stream_timestamp will be kNoTimestamp().
   const base::TimeDelta stream_timestamp =
       ConvertStreamTimestamp(stream_->time_base, packet->pts);
 
   if (stream_timestamp != kNoTimestamp()) {
     const bool is_audio = type() == AUDIO;
 
     // If this is an OGG file with negative timestamps don't rebase any other

[wolenetz, 2015/08/06 22:21:27]

nit: s/an OGG/a/ ?

[DaleCurtis, 2015/08/06 23:45:27]

Already fixed in later patch set.

     // stream types against the negative starting time.
     base::TimeDelta start_time = demuxer_->start_time();
-    if (fixup_negative_ogg_timestamps_ && !is_audio &&
+    if (fixup_negative_timestamps_ && !is_audio &&
         start_time < base::TimeDelta()) {
       start_time = base::TimeDelta();
     }
 
     // Don't rebase timestamps for positive start times, the HTML Media Spec
     // details this in section "4.8.10.6 Offsets into the media resource." We
     // will still need to rebase timestamps before seeking with FFmpeg though.
     if (start_time > base::TimeDelta())
       start_time = base::TimeDelta();
 
     buffer->set_timestamp(stream_timestamp - start_time);
 
-    // If enabled, mark audio packets with negative timestamps for post-decode
-    // discard.
-    if (fixup_negative_ogg_timestamps_ && is_audio &&
+    // If enabled, and no codec delay is present, mark audio packets with
+    // negative timestamps for post-decode discard.

[chcunningham, 2015/07/31 19:48:44]

just to check my understanding....

1. we see negative timestamps here because ffmpeg is moving the timeline back
for preskip, so the skip porition is negative

2. we don't mark it for post-decode discard because we will handle that via
opus_decoder's discard helper, and marking it for discard_padding here is
redundant and error prone?

If I have the above right, are we confident in making this change for non-opus
codecs? Might there be other decoders which were relying on us to set discard
padding here for their codec delay?

You might add more to this comment about why the code path is different for
codec->delay.

[DaleCurtis, 2015/08/03 22:49:53]

Yes that is correct. We're confident because this code doesn't change anything
for codecs which don't specify codec delay, for those that do I've added a
DCHECK to verify we trim enough.

+    if (fixup_negative_timestamps_ && is_audio &&
         stream_timestamp < base::TimeDelta() &&
-        buffer->duration() != kNoTimestamp()) {
+        buffer->duration() != kNoTimestamp() && !stream_->codec->delay) {
+      DCHECK_EQ(buffer->discard_padding().first, base::TimeDelta());
+
       if (stream_timestamp + buffer->duration() < base::TimeDelta()) {
+        DCHECK_EQ(buffer->discard_padding().second, base::TimeDelta());
+
         // Discard the entire packet if it's entirely before zero.
         buffer->set_discard_padding(
             std::make_pair(kInfiniteDuration(), base::TimeDelta()));
       } else {
-        // Only discard part of the frame if it overlaps zero.

[chcunningham, 2015/07/31 19:48:44]

Did you mean to kill the comment?

[DaleCurtis, 2015/08/03 22:49:53]

Nope, reverted.

-        buffer->set_discard_padding(
-            std::make_pair(-stream_timestamp, base::TimeDelta()));
+        buffer->set_discard_padding(std::make_pair(
+            -stream_timestamp, buffer->discard_padding().second));

[chcunningham, 2015/07/31 19:48:44]

I can see why this is more correct, but I'm curious: did you hit this at some
point (what were the circumstances), or is it just drive-by fixing?

[DaleCurtis, 2015/08/03 22:49:53]

Just fixing this as wolenetz@ complained about it earlier. I haven't ever hit
this and am not sure it's possible, but the discard helper will allow it.

       }
     }
   } else {
     // If this happens on the first packet, decoders will throw an error.
     buffer->set_timestamp(kNoTimestamp());
   }
 
   if (last_packet_timestamp_ != kNoTimestamp()) {
     // FFmpeg doesn't support chained ogg correctly.  Instead of guaranteeing
     // continuity across links in the chain it uses the timestamp information
     // from each link directly.  Doing so can lead to timestamps which appear to
     // go backwards in time.
     //
     // If the new link starts with a negative timestamp or a timestamp less than
     // the original (positive) |start_time|, we will get a negative timestamp
     // here.  It's also possible FFmpeg returns kNoTimestamp() here if it's not
     // able to work out a timestamp using the previous link and the next.
     //
     // Fixing chained ogg is non-trivial, so for now just reuse the last good
     // timestamp.  The decoder will rewrite the timestamps to be sample accurate
     // later.  See http://crbug.com/396864.
-    if (fixup_negative_ogg_timestamps_ &&
+    if (fixup_negative_timestamps_ &&
         (buffer->timestamp() == kNoTimestamp() ||
          buffer->timestamp() < last_packet_timestamp_)) {
       buffer->set_timestamp(last_packet_timestamp_ +
                             (last_packet_duration_ != kNoTimestamp()
                                  ? last_packet_duration_
                                  : base::TimeDelta::FromMicroseconds(1)));
     }
 
     // The demuxer should always output positive timestamps.
     DCHECK(buffer->timestamp() >= base::TimeDelta());
@@ skipping 263 matching lines @@
   // otherwise we can end up waiting for a pre-seek read to complete even though
   // we know we're going to drop it on the floor.
 
   // FFmpeg requires seeks to be adjusted according to the lowest starting time.
   // Since EnqueuePacket() rebased negative timestamps by the start time, we
   // must correct the shift here.
   //
   // Additionally, to workaround limitations in how we expose seekable ranges to
   // Blink (http://crbug.com/137275), we also want to clamp seeks before the
   // start time to the start time.
-  const base::TimeDelta seek_time =
-      start_time_ < base::TimeDelta() ? time + start_time_
-                                      : time < start_time_ ? start_time_ : time;
+  base::TimeDelta seek_time = start_time_ < base::TimeDelta()
+                                  ? time + start_time_
+                                  : time < start_time_ ? start_time_ : time;

[chcunningham, 2015/07/31 19:48:44]

I don't follow why we only add start_time_ in the case where start_time < 0?

[DaleCurtis, 2015/08/03 22:49:53]

See l.305, the gist is that we don't rebase positive start times.

[chcunningham, 2015/08/04 20:38:09]

I see. This is a fragile contract between two methods several hundred lines
apart. Might be cool to have a module like FFmpegTimestampGlue that takes in a
configuration and has APIs like 

TimeDelta FfmpegSeekTimeFromPipelineTime(const DecoderBuffer*) - 
void FixPipelineTimestamp(DecoderBuffer*) - use in EnqueuePacket

eh... I don't know, just looking for ways to tame the spaghetti monster. 

+
+  // When seeking in an opus stream we need to ensure we deliver enough data to
+  // satisfy the seek preroll; otherwise the audio at the actual seek time will
+  // not be entirely accurate.
+  FFmpegDemuxerStream* audio_stream = GetFFmpegStream(DemuxerStream::AUDIO);
+  if (audio_stream) {
+    const AudioDecoderConfig& config = audio_stream->audio_decoder_config();
+    if (config.codec() == kCodecOpus)

[chcunningham, 2015/07/31 19:48:44]

How does this work for other codecs that need pre-roll?

[DaleCurtis, 2015/08/03 22:49:53]

It doesn't :) No other codecs specify this information in terms of an actual
seek_preroll(). For cases like mp3 and aac which would benefit from an explicit
seek_preroll() we aren't doing anything special -- though in many cases ffmpeg
seeks early enough that the decoder is effectively satisfied.

Thus far this hasn't been a noticeable problem, it may not be for opus either...
I expect at worst you might here a pop or click after the seek. I don't know if
skipping the opus preroll may have harsher effects or is more of a quality of
implementation thing.

We could probably start setting a seek_preroll() of 529 frames for MP3 and 1152
for AAC if we want.  I can take a look at that in a followup patch.  It
shouldn't be too hard for src=, not sure how you'd want to handle it for MSE,
but it'd be similar to whatever you want to do for OPUS.

[chcunningham, 2015/08/04 20:38:09]

Is this something we can extract into a utility used by both src= and mse? 

TimeDelta media::GetCodecSeekTime(TimeDelta, Codec) {
swtich(codec)
... various preroll adjustments
}

maintain it all in one place?

[DaleCurtis, 2015/08/04 21:38:34]

This is already encoded in the seek_preroll() and since it's just a subtraction,
I don't think it's worth abstracting further.

+      seek_time = std::max(start_time_, seek_time - config.seek_preroll());
+  }
 
   // Choose the seeking stream based on whether it contains the seek time, if no
   // match can be found prefer the preferred stream.
   //
   // TODO(dalecurtis): Currently FFmpeg does not ensure that all streams in a
   // given container will demux all packets after the seek point.  Instead it
   // only guarantees that all packets after the file position of the seek will
   // be demuxed.  It's an open question whether FFmpeg should fix this:
   // http://lists.ffmpeg.org/pipermail/ffmpeg-devel/2014-June/159212.html
   // Tracked by http://crbug.com/387996.
@@ skipping 309 matching lines @@
     // maximum between it and the duration from A/V streams.
     const AVRational av_time_base = {1, AV_TIME_BASE};
     max_duration =
         std::max(max_duration,
                  ConvertFromTimeBase(av_time_base, format_context->duration));
   } else {
     // The duration is unknown, in which case this is likely a live stream.
     max_duration = kInfiniteDuration();
   }
 
-  // Ogg has some peculiarities around negative timestamps, so use this flag to
-  // setup the FFmpegDemuxerStreams appropriately.
+  // FFmpeg represents audio data marked as before the beginning of stream as
+  // having negative timestamps.  This data must be discarded after it has been
+  // decoded, not before since it is used to warmup the decoder.  There are
+  // currently two known cases for this: vorbis in ogg and opus in ogg and webm.
+  //
+  // For API clarity, it was decided that the rest of the media pipeline should
+  // not be exposed to negative timestamps.  Which means we need to rebase these
+  // negative timestamps and mark them for discard post decoding.
   //
   // Post-decode frame dropping for packets with negative timestamps is outlined
   // in section A.2 in the Ogg Vorbis spec:
   // http://xiph.org/vorbis/doc/Vorbis_I_spec.html
-  if (strcmp(format_context->iformat->name, "ogg") == 0 && audio_stream &&
-      audio_stream->codec->codec_id == AV_CODEC_ID_VORBIS) {
+  //
+  // FFmpeg's use of negative timestamps for opus pre-skip is nonstandard, but
+  // for more information on pre-skip see section 4.2 of the Ogg Opus spec:
+  // https://tools.ietf.org/html/draft-ietf-codec-oggopus-08#section-4.2
+  if (audio_stream && (audio_stream->codec->codec_id == AV_CODEC_ID_OPUS ||
+                       (strcmp(format_context->iformat->name, "ogg") == 0 &&
+                        audio_stream->codec->codec_id == AV_CODEC_ID_VORBIS))) {
     for (size_t i = 0; i < streams_.size(); ++i) {
       if (streams_[i])
-        streams_[i]->enable_negative_timestamp_fixups_for_ogg();
+        streams_[i]->enable_negative_timestamp_fixups();

[chcunningham, 2015/07/31 19:48:44]

Why do we do this for all streams and not just the audio stream? 

[DaleCurtis, 2015/08/03 22:49:53]

Because we need to tell the video stream that it shouldn't rebase the timestamps
against the negative start time.  I've updated the function comments for
clarity.

[chcunningham, 2015/08/04 20:38:09]

I see. I feel like this should really be called 
enable_negative_*audio*_timestamp_fixups()

[DaleCurtis, 2015/08/04 21:38:34]

Well, it has different effects on audio and video, so that's misleading.

     }
 
     // Fixup the seeking information to avoid selecting the audio stream simply
     // because it has a lower starting time.
     if (fallback_stream_for_seeking_.first == audio_stream->index &&
         fallback_stream_for_seeking_.second < base::TimeDelta()) {
       fallback_stream_for_seeking_.second = base::TimeDelta();
     }
   }
 
@@ skipping 213 matching lines @@
     // when av_read_frame() returns success code. See bug comment for ideas:
     //
     // https://code.google.com/p/chromium/issues/detail?id=169133#c10
     if (!packet->data) {
       ScopedAVPacket new_packet(new AVPacket());
       av_new_packet(new_packet.get(), 0);
       av_packet_copy_props(new_packet.get(), packet.get());
       packet.swap(new_packet);
     }
 
-    // Special case for opus in ogg.  FFmpeg is pre-trimming the codec delay
-    // from the packet timestamp.  Chrome expects to handle this itself inside
-    // the decoder, so shift timestamps by the delay in this case.
-    // TODO(dalecurtis): Try to get fixed upstream.  See http://crbug.com/328207
-    if (strcmp(glue_->format_context()->iformat->name, "ogg") == 0) {
-      const AVCodecContext* codec_context =
-          glue_->format_context()->streams[packet->stream_index]->codec;
-      if (codec_context->codec_id == AV_CODEC_ID_OPUS &&
-          codec_context->delay > 0) {
-        packet->pts += codec_context->delay;
-      }
-    }
-
     FFmpegDemuxerStream* demuxer_stream = streams_[packet->stream_index];
     demuxer_stream->EnqueuePacket(packet.Pass());
   }
 
   // Keep reading until we've reached capacity.
   ReadFrameIfNeeded();
 }
 
 bool FFmpegDemuxer::StreamsHaveAvailableCapacity() {
   DCHECK(task_runner_->BelongsToCurrentThread());
@@ skipping 73 matching lines @@
 
 void FFmpegDemuxer::SetLiveness(DemuxerStream::Liveness liveness) {
   DCHECK(task_runner_->BelongsToCurrentThread());
   for (const auto& stream : streams_) {  // |stream| is a ref to a pointer.
     if (stream)
       stream->SetLiveness(liveness);
   }
 }
 
 }  // namespace media