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

media/filters/ffmpeg_demuxer.cc

Index: media/filters/ffmpeg_demuxer.cc
diff --git a/media/filters/ffmpeg_demuxer.cc b/media/filters/ffmpeg_demuxer.cc
index 80cb7adcbee7a0c13c7630b2d14d4b676db7af61..805517fe9293ab7de2fc9c025c64dc86f04da08c 100644
--- a/media/filters/ffmpeg_demuxer.cc
+++ b/media/filters/ffmpeg_demuxer.cc
@@ -94,7 +94,7 @@ FFmpegDemuxerStream::FFmpegDemuxerStream(FFmpegDemuxer* demuxer,
       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;
@@ -297,7 +297,7 @@ void FFmpegDemuxerStream::EnqueuePacket(ScopedAVPacket packet) {
     // 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();
     }
@@ -310,19 +310,22 @@ void FFmpegDemuxerStream::EnqueuePacket(ScopedAVPacket packet) {
 
     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 {
@@ -344,7 +347,7 @@ void FFmpegDemuxerStream::EnqueuePacket(ScopedAVPacket packet) {
     // 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_ +
@@ -628,9 +631,19 @@ void FFmpegDemuxer::Seek(base::TimeDelta time, const PipelineStatusCB& cb) {
   // 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.
@@ -960,17 +973,28 @@ void FFmpegDemuxer::OnFindStreamInfoDone(const PipelineStatusCB& status_cb,
     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
@@ -1204,19 +1228,6 @@ void FFmpegDemuxer::OnReadFrameDone(ScopedAVPacket packet, int result) {
       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());
   }