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 236 matching lines @@
                                        packet.get()->size - data_offset);
     }
 
     int skip_samples_size = 0;
     const uint32* skip_samples_ptr =
         reinterpret_cast<const uint32*>(av_packet_get_side_data(
             packet.get(), AV_PKT_DATA_SKIP_SAMPLES, &skip_samples_size));
     const int kSkipSamplesValidSize = 10;
     const int kSkipEndSamplesOffset = 1;
     if (skip_samples_size >= kSkipSamplesValidSize) {
       // Because FFmpeg rolls codec delay and skip samples into one we can only

[wolenetz, 2015/07/29 21:57:27]

Is there any AV_PKT_DATA_SKIP_SAMPLES (front skip) ever populated for
AV_CODEC_ID_OPUS? Perhaps add a DCHECK to help catch regression?

[DaleCurtis, 2015/07/30 01:28:14]

Added a DCHECK and reflowed the conditional for support of end-trimming, though
I'm not sure that's actually possible.

       // allow front discard padding on the first buffer.  Otherwise the discard
       // helper can't figure out which data to discard.  See AudioDiscardHelper.
       int discard_front_samples = base::ByteSwapToLE32(*skip_samples_ptr);
       if (last_packet_timestamp_ != kNoTimestamp() && discard_front_samples) {
         DLOG(ERROR) << "Skip samples are only allowed for the first packet.";
         discard_front_samples = 0;
       }
 
       const int discard_end_samples =
           base::ByteSwapToLE32(*(skip_samples_ptr + kSkipEndSamplesOffset));
       const int samples_per_second =
           audio_decoder_config().samples_per_second();
       buffer->set_discard_padding(std::make_pair(
           FramesToTimeDelta(discard_front_samples, samples_per_second),
           FramesToTimeDelta(discard_end_samples, samples_per_second)));
+    } else if (type() == AUDIO && stream_->codec->delay > 0 &&
+               packet->pts <= stream_->start_time &&
+               stream_->codec->codec_id == AV_CODEC_ID_OPUS) {
+      // FFmpeg does not encode the opus pre-skip value within the packet side
+      // data, so when we encounter the first packet in a stream ensure the
+      // codec delay is applied per spec.
+      //
+      // |stream_->codec->delay| has been added to packet->pts automatically by
+      // ffmpeg, so the delta between the start time and the packet pts is the
+      // amount of delay remaining to trim.
+      buffer->set_discard_padding(std::make_pair(
+          FramesToTimeDelta(std::min(static_cast<int64_t>(packet->duration),
+                                     stream_->start_time - packet->pts),
+                            audio_decoder_config().samples_per_second()),
+          base::TimeDelta()));

[wolenetz, 2015/07/29 21:57:27]

Could there be a single packet that is both the start and end of an Opus stream?
If so, then if there is any end sample skipping, this code doesn't work properly
IIUC.

[DaleCurtis, 2015/07/30 01:28:14]

Done.

     }
 
     if (decrypt_config)
       buffer->set_decrypt_config(decrypt_config.Pass());
   }
 
   if (packet->duration >= 0) {
     buffer->set_duration(
         ConvertStreamTimestamp(stream_->time_base, packet->duration));
   } else {
@@ skipping 338 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;
+
+  // 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)
+      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 553 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