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

media/filters/opus_audio_decoder.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/opus_audio_decoder.h"
 
 #include <cmath>
 
 #include "base/single_thread_task_runner.h"
 #include "base/sys_byteorder.h"
 #include "media/base/audio_buffer.h"
 #include "media/base/audio_decoder_config.h"
 #include "media/base/audio_discard_helper.h"
 #include "media/base/bind_to_current_loop.h"
 #include "media/base/buffers.h"
 #include "media/base/decoder_buffer.h"
 #include "third_party/opus/src/include/opus.h"
 #include "third_party/opus/src/include/opus_multistream.h"
 
 namespace media {
 
+static base::TimeDelta FramesToTimeDelta(int frames, double sample_rate) {
+  return base::TimeDelta::FromMicroseconds(
+      frames * base::Time::kMicrosecondsPerSecond / sample_rate);
+}
+
 static uint16 ReadLE16(const uint8* data, size_t data_size, int read_offset) {
   uint16 value = 0;
   DCHECK_LE(read_offset + sizeof(value), data_size);
   memcpy(&value, data + read_offset, sizeof(value));
   return base::ByteSwapToLE16(value);
 }
 
 // The Opus specification is part of IETF RFC 6716:
 // http://tools.ietf.org/html/rfc6716
 
@@ skipping 206 matching lines @@
   if (extra_data->num_streams + extra_data->num_coupled != extra_data->channels)
     DVLOG(1) << "Inconsistent channel mapping.";
 
   for (int i = 0; i < extra_data->channels; ++i)
     extra_data->stream_map[i] = *(data + kOpusExtraDataStreamMapOffset + i);
   return true;
 }
 
 OpusAudioDecoder::OpusAudioDecoder(
     const scoped_refptr<base::SingleThreadTaskRunner>& task_runner)
-    : task_runner_(task_runner),
-      opus_decoder_(NULL),
-      start_input_timestamp_(kNoTimestamp()) {}
+    : task_runner_(task_runner), opus_decoder_(nullptr) {}
 
 std::string OpusAudioDecoder::GetDisplayName() const {
   return "OpusAudioDecoder";
 }
 
 void OpusAudioDecoder::Initialize(const AudioDecoderConfig& config,
                                   const InitCB& init_cb,
                                   const OutputCB& output_cb) {
   DCHECK(task_runner_->BelongsToCurrentThread());
   InitCB bound_init_cb = BindToCurrentLoop(init_cb);
@@ skipping 25 matching lines @@
   task_runner_->PostTask(FROM_HERE, closure);
 }
 
 OpusAudioDecoder::~OpusAudioDecoder() {
   DCHECK(task_runner_->BelongsToCurrentThread());
 
   if (!opus_decoder_)
     return;
 
   opus_multistream_decoder_ctl(opus_decoder_, OPUS_RESET_STATE);
-  ResetTimestampState();
   CloseDecoder();
 }
 
 void OpusAudioDecoder::DecodeBuffer(
     const scoped_refptr<DecoderBuffer>& input,
     const DecodeCB& decode_cb) {
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK(!decode_cb.is_null());
   DCHECK(input.get());
 
   // Libopus does not buffer output. Decoding is complete when an end of stream
   // input buffer is received.
   if (input->end_of_stream()) {
     decode_cb.Run(kOk);
     return;
   }
 
   // Make sure we are notified if http://crbug.com/49709 returns.  Issue also
   // occurs with some damaged files.
   if (input->timestamp() == kNoTimestamp()) {
     DLOG(ERROR) << "Received a buffer without timestamps!";
     decode_cb.Run(kDecodeError);
     return;
   }
 
-  // Apply the necessary codec delay.
-  if (start_input_timestamp_ == kNoTimestamp())
-    start_input_timestamp_ = input->timestamp();
-  if (!discard_helper_->initialized() &&
-      input->timestamp() == start_input_timestamp_) {
-    discard_helper_->Reset(config_.codec_delay());
-  }
-
   scoped_refptr<AudioBuffer> output_buffer;
 
   if (!Decode(input, &output_buffer)) {
     decode_cb.Run(kDecodeError);
     return;
   }
 
   if (output_buffer.get()) {
     output_cb_.Run(output_buffer);
   }
@@ skipping 73 matching lines @@
   }
 
   status = opus_multistream_decoder_ctl(
       opus_decoder_, OPUS_SET_GAIN(opus_extra_data.gain_db));
   if (status != OPUS_OK) {
     DLOG(ERROR) << "Failed to set OPUS header gain; status="
                 << opus_strerror(status);
     return false;
   }
 
-  discard_helper_.reset(
-      new AudioDiscardHelper(config_.samples_per_second(), 0));
-  start_input_timestamp_ = kNoTimestamp();
+  ResetTimestampState();
   return true;
 }
 
 void OpusAudioDecoder::CloseDecoder() {
   if (opus_decoder_) {
     opus_multistream_decoder_destroy(opus_decoder_);
-    opus_decoder_ = NULL;
+    opus_decoder_ = nullptr;
   }
 }
 
 void OpusAudioDecoder::ResetTimestampState() {
-  discard_helper_->Reset(
-      discard_helper_->TimeDeltaToFrames(config_.seek_preroll()));
+  start_timestamp_ =
+      FramesToTimeDelta(config_.codec_delay(), config_.samples_per_second());
+  discard_helper_.reset(
+      new AudioDiscardHelper(config_.samples_per_second(), 0));
 }
 
 bool OpusAudioDecoder::Decode(const scoped_refptr<DecoderBuffer>& input,
                               scoped_refptr<AudioBuffer>* output_buffer) {
   // Allocate a buffer for the output samples.
   *output_buffer = AudioBuffer::CreateBuffer(
       config_.sample_format(),
       config_.channel_layout(),
       ChannelLayoutToChannelCount(config_.channel_layout()),
       config_.samples_per_second(),
@@ skipping 21 matching lines @@
                 << " status: " << opus_strerror(frames_decoded);
     return false;
   }
 
   // Trim off any extraneous allocation.
   DCHECK_LE(frames_decoded, output_buffer->get()->frame_count());
   const int trim_frames = output_buffer->get()->frame_count() - frames_decoded;
   if (trim_frames > 0)
     output_buffer->get()->TrimEnd(trim_frames);
 
+  // Workaround for the lack of proper pre-skip support in MediaSource; if this
+  // is the first packet and the timestamp is zero, strip codec delay.

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

Hmm. In MSE, the fix could be simpler: on config change, a new Opus decoder is
initialized. The Opus decoder output following that config change will need the
first config->codec_delay samples discarded no matter what IIUC.

For example, append init segment containing Opus codec delay of 312
frames@48Khz=6.5ms, and then time [10s,20s) of an Opus stream that begins at
time 0. Seek to time 10s. Ignoring the lack of seek preroll support for Opus in
MSE, the first 6.5ms of decode output from the encoded media @ 10s would need to
be discarded.  This is because the PTS of 10s in webm is for a block containing
codec_delay frames (from previous packet spilling over transitively), followed
by the frames that are really meant to be presented beginning at 10s.

So, instead of messing with start_timestamp_, couldn't we just apply Opus codec
delay discard padding to the first buffer passed to a new opus decoder instance?
Would this also work for ffmpeg demuxed buffers?

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

That understanding is not correct, only the first buffer has data which needs
trimming. Only timestamps should be shifted by the pre-skip value, the actual
decoded audio data should not be trimmed unless we're dealing with the
beginning-of-stream packet(s).

The timestamp shifting is done by ffmpeg for opus and webm  (see the code I
removed in FFmpegDemuxer), I'm not sure where or if it needs to happen for webm
in mse, though the file plays correctly from start, and a seek within the
pre-skip appears to work correctly -- however the midpoint seek does not match
the results returned by ffmpeg for ogg/webm.

[DaleCurtis, 2015/07/31 02:41:00]

Turns out my understanding was totally wrong. It is correct that we always want
to drop pre-skip worth of samples since timestamp zero represents the beginning
of the pre-skip. The hash tests I ran previously were in fact forcing ffmpeg to
do the wrong thing. Sorry for all the confusion on my part.

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

No worries. Thanks for following-up on this point of confusion!

+  // TODO(chcunningham): Remove this hack, http://crbug.com/509894
+  if (input->timestamp() < start_timestamp_ &&
+      input->discard_padding().first == base::TimeDelta()) {
+    input->set_discard_padding(std::make_pair(
+        std::min(input->duration(), start_timestamp_ - input->timestamp()),
+        base::TimeDelta()));
+  }
+
   // Handles discards and timestamping.  Discard the buffer if more data needed.
   if (!discard_helper_->ProcessBuffers(input, *output_buffer))
-    *output_buffer = NULL;
+    *output_buffer = nullptr;
 
   return true;
 }
 
 }  // namespace media