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

media/filters/opus_audio_decoder.cc

Index: media/filters/opus_audio_decoder.cc
diff --git a/media/filters/opus_audio_decoder.cc b/media/filters/opus_audio_decoder.cc
index 181f34014da4f0b9b011c1381b94a886440154e8..53d897175ad0fb42317c91a70ad2c1117519b175 100644
--- a/media/filters/opus_audio_decoder.cc
+++ b/media/filters/opus_audio_decoder.cc
@@ -19,6 +19,11 @@
 
 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);
@@ -245,9 +250,7 @@ static bool ParseOpusExtraData(const uint8* data, int data_size,
 
 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";
@@ -293,7 +296,6 @@ OpusAudioDecoder::~OpusAudioDecoder() {
     return;
 
   opus_multistream_decoder_ctl(opus_decoder_, OPUS_RESET_STATE);
-  ResetTimestampState();
   CloseDecoder();
 }
 
@@ -319,14 +321,6 @@ void OpusAudioDecoder::DecodeBuffer(
     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)) {
@@ -420,22 +414,22 @@ bool OpusAudioDecoder::ConfigureDecoder() {
     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,
@@ -477,9 +471,19 @@ bool OpusAudioDecoder::Decode(const scoped_refptr<DecoderBuffer>& input,
   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;
 }