Refactor FFmpegAudioDecoder output timestamp logic.

media/filters/ffmpeg_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/ffmpeg_audio_decoder.h"
 
 #include "base/bind.h"
 #include "base/callback_helpers.h"
 #include "media/base/audio_decoder_config.h"
 #include "media/base/data_buffer.h"
 #include "media/base/decoder_buffer.h"
 #include "media/base/demuxer.h"
 #include "media/base/pipeline.h"
 #include "media/ffmpeg/ffmpeg_common.h"
 #include "media/filters/ffmpeg_glue.h"
 
 namespace media {
 
-// Returns true if the decode result was a timestamp packet and not actual audio
-// data.
-static inline bool IsTimestampMarkerPacket(int result, Buffer* input) {
-  // We can get a positive result but no decoded data.  This is ok because this
-  // this can be a marker packet that only contains timestamp.
-  return result > 0 && !input->IsEndOfStream() &&
-      input->GetTimestamp() != kNoTimestamp() &&
-      input->GetDuration() != kNoTimestamp();
-}
-
 // Returns true if the decode result was end of stream.
 static inline bool IsEndOfStream(int result, int decoded_size, Buffer* input) {
   // Three conditions to meet to declare end of stream for this decoder:
   // 1. FFmpeg didn't read anything.
   // 2. FFmpeg didn't output anything.
   // 3. An end of stream buffer is received.
   return result == 0 && decoded_size == 0 && input->IsEndOfStream();
 }
 
-
 FFmpegAudioDecoder::FFmpegAudioDecoder(
     const base::Callback<MessageLoop*()>& message_loop_cb)
     : message_loop_factory_cb_(message_loop_cb),
       message_loop_(NULL),
       codec_context_(NULL),
       bits_per_channel_(0),
       channel_layout_(CHANNEL_LAYOUT_NONE),
       samples_per_second_(0),
+      bytes_per_frame_(0),
+      output_timestamp_base_(kNoTimestamp()),
+      total_frames_decoded_(0),
+      last_input_timestamp_(kNoTimestamp()),
+      output_bytes_to_drop_(0),
       av_frame_(NULL) {
 }
 
 void FFmpegAudioDecoder::Initialize(
     const scoped_refptr<DemuxerStream>& stream,
     const PipelineStatusCB& status_cb,
     const StatisticsCB& statistics_cb) {
   // Ensure FFmpeg has been initialized
   FFmpegGlue::GetInstance();
 
@@ skipping 82 matching lines @@
 
     status_cb.Run(DECODER_ERROR_NOT_SUPPORTED);
     return;
   }
 
   // Success!
   av_frame_ = avcodec_alloc_frame();
   bits_per_channel_ = config.bits_per_channel();
   channel_layout_ = config.channel_layout();
   samples_per_second_ = config.samples_per_second();
-
+  bytes_per_frame_ = codec_context_->channels * bits_per_channel_ / 8;
   status_cb.Run(PIPELINE_OK);
 }
 
 void FFmpegAudioDecoder::DoReset(const base::Closure& closure) {
   avcodec_flush_buffers(codec_context_);
-  estimated_next_timestamp_ = kNoTimestamp();
+  output_timestamp_base_ = kNoTimestamp();
+  total_frames_decoded_ = 0;
+  last_input_timestamp_ = kNoTimestamp();
+  output_bytes_to_drop_ = 0;
   closure.Run();
 }
 
 void FFmpegAudioDecoder::DoRead(const ReadCB& read_cb) {
   DCHECK_EQ(MessageLoop::current(), message_loop_);
   DCHECK(!read_cb.is_null());
   CHECK(read_cb_.is_null()) << "Overlapping decodes are not supported.";
 
   read_cb_ = read_cb;
   ReadFromDemuxerStream();
 }
 
 void FFmpegAudioDecoder::DoDecodeBuffer(
     DemuxerStream::Status status,
     const scoped_refptr<DecoderBuffer>& input) {
   DCHECK_EQ(MessageLoop::current(), message_loop_);
   DCHECK(!read_cb_.is_null());
 
   if (status != DemuxerStream::kOk) {
     DCHECK(!input);
     // TODO(acolwell): Add support for reinitializing the decoder when
     // |status| == kConfigChanged. For now we just trigger a decode error.
     AudioDecoder::Status decoder_status =
         (status == DemuxerStream::kAborted) ? kAborted : kDecodeError;
     base::ResetAndReturn(&read_cb_).Run(decoder_status, NULL);
     return;
   }
 
-  // FFmpeg tends to seek Ogg audio streams in the middle of nowhere, giving us
-  // a whole bunch of AV_NOPTS_VALUE packets.  Discard them until we find
-  // something valid.  Refer to http://crbug.com/49709
-  if (input->GetTimestamp() == kNoTimestamp() &&
-      estimated_next_timestamp_ == kNoTimestamp() &&
-      !input->IsEndOfStream()) {
-    ReadFromDemuxerStream();
-    return;
+  // Make sure we are notified if http://crbug.com/49709 returns.
+  CHECK(input->GetTimestamp() != kNoTimestamp() ||
+        output_timestamp_base_ != kNoTimestamp() ||
+        input->IsEndOfStream());

[scherkus (not reviewing), 2012/08/03 22:34:57]

want to << "some text" so folks know what's up instead of digging through the
source for the above comment?

[acolwell GONE FROM CHROMIUM, 2012/08/03 23:14:01]

Done.

+
+  bool is_vorbis =
+      demuxer_stream_->audio_decoder_config().codec() == kCodecVorbis;

[scherkus (not reviewing), 2012/08/03 22:34:57]

is the check done here in case the codec changes on the fly?

if so, will we handle the change correctly?
if not, should we cache the value in ::DoInitialize()?

[acolwell GONE FROM CHROMIUM, 2012/08/03 23:14:01]

There are no plans to allow the codec to change. Added codec_ member variable
and copy the value in DoInitialize().

[DaleCurtis, 2012/08/03 23:15:36]

codec_context_->codec_id == CODEC_ID_VORBIS seems much simpler?

[acolwell GONE FROM CHROMIUM, 2012/08/03 23:25:05]

Done.

+
+  if (!input->IsEndOfStream()) {
+    if (last_input_timestamp_ == kNoTimestamp()) {
+      if (is_vorbis) {

[scherkus (not reviewing), 2012/08/03 22:34:57]

if there is some online documentation anywhere that explains this craziness it
would be prudent to include it here :)

[acolwell GONE FROM CHROMIUM, 2012/08/03 23:14:01]

Added a reference to the Vorbis spec.

+        if (input->GetTimestamp() < base::TimeDelta()) {

[DaleCurtis, 2012/08/03 23:15:36]

Can avoid double else below by rolling this into if (is_vorbis).

[acolwell GONE FROM CHROMIUM, 2012/08/03 23:25:05]

Done.

+          int frames_to_drop = floor(0.5 +

[scherkus (not reviewing), 2012/08/03 22:34:57]

technially we'd wrap at the ( on next line, so what about dropping floor to next
line instead?

[acolwell GONE FROM CHROMIUM, 2012/08/03 23:14:01]

The floor doesn't fit. I've moved the 0.5 + down though.

+              -input->GetTimestamp().InSecondsF() * samples_per_second_);

[scherkus (not reviewing), 2012/08/03 22:34:57]

and we have a test that covers this business?

[acolwell GONE FROM CHROMIUM, 2012/08/03 23:14:01]

Yep. MediaTest.VideoBearTheora uncovered the need for this code.

+          output_bytes_to_drop_ = bytes_per_frame_ * frames_to_drop;
+        } else {
+          last_input_timestamp_ = input->GetTimestamp();
+        }
+      } else {
+        last_input_timestamp_ = input->GetTimestamp();
+      }
+    } else if (input->GetTimestamp() < last_input_timestamp_) {
+      base::TimeDelta diff = input->GetTimestamp() - last_input_timestamp_;
+      DVLOG(1) << "Input timestamps are not monotonically increasing! "
+               << " ts " << input->GetTimestamp().InMicroseconds() << " us"
+               << " diff " << diff.InMicroseconds() << " us";
+      base::ResetAndReturn(&read_cb_).Run(kDecodeError, NULL);
+      return;
+    }
   }
 
   AVPacket packet;
   av_init_packet(&packet);
   packet.data = const_cast<uint8*>(input->GetData());
   packet.size = input->GetDataSize();
 
   PipelineStatistics statistics;
   statistics.audio_bytes_decoded = input->GetDataSize();
 
@@ skipping 12 matching lines @@
 
     DLOG(ERROR) << "Error decoding an audio frame with timestamp: "
                 << input->GetTimestamp().InMicroseconds() << " us, duration: "
                 << input->GetDuration().InMicroseconds() << " us, packet size: "
                 << input->GetDataSize() << " bytes";
 
     ReadFromDemuxerStream();
     return;
   }
 
+  if (result > 0)
+    DCHECK_EQ(result, input->GetDataSize());
+
+  if (output_timestamp_base_ == kNoTimestamp() && !input->IsEndOfStream()) {
+    DCHECK(input->GetTimestamp() != kNoTimestamp());
+    if (output_bytes_to_drop_ > 0) {
+      // Currently Vorbis is the only codec that causes us to drop samples.
+      // If we have to drop samples it always means the timeline starts at 0.
+      DCHECK(is_vorbis);
+      output_timestamp_base_ = base::TimeDelta();
+    } else {
+      output_timestamp_base_ = input->GetTimestamp();
+    }
+  }
+
+  const uint8* decoded_audio_data = NULL;
   int decoded_audio_size = 0;
   if (frame_decoded) {
     int output_sample_rate = av_frame_->sample_rate;
     if (output_sample_rate != samples_per_second_) {
       DLOG(ERROR) << "Output sample rate (" << output_sample_rate
                   << ") doesn't match expected rate " << samples_per_second_;
       base::ResetAndReturn(&read_cb_).Run(kDecodeError, NULL);
       return;
     }
 
+    decoded_audio_data = av_frame_->data[0];
     decoded_audio_size = av_samples_get_buffer_size(
         NULL, codec_context_->channels, av_frame_->nb_samples,
         codec_context_->sample_fmt, 1);
   }
 
   scoped_refptr<DataBuffer> output;
 
+  if (decoded_audio_size > 0 && output_bytes_to_drop_ > 0) {
+    int dropped_size = std::min(decoded_audio_size, output_bytes_to_drop_);
+    decoded_audio_data += dropped_size;
+    decoded_audio_size -= dropped_size;
+    output_bytes_to_drop_ -= dropped_size;
+  }
+
   if (decoded_audio_size > 0) {
+    DCHECK_EQ(decoded_audio_size % bytes_per_frame_, 0)
+        << "Decoder didn't output full frames";
+
     // Copy the audio samples into an output buffer.
     output = new DataBuffer(decoded_audio_size);
     output->SetDataSize(decoded_audio_size);
     uint8* data = output->GetWritableData();
-    memcpy(data, av_frame_->data[0], decoded_audio_size);
+    memcpy(data, decoded_audio_data, decoded_audio_size);
 
-    UpdateDurationAndTimestamp(input, output);
-  } else if (IsTimestampMarkerPacket(result, input)) {
-    // Nothing else to do here but update our estimation.
-    estimated_next_timestamp_ = input->GetTimestamp() + input->GetDuration();
+    base::TimeDelta timestamp = GetNextOutputTimestamp();
+    total_frames_decoded_ += decoded_audio_size / bytes_per_frame_;
+
+    output->SetTimestamp(timestamp);
+    output->SetDuration(GetNextOutputTimestamp() - timestamp);
   } else if (IsEndOfStream(result, decoded_audio_size, input)) {

[scherkus (not reviewing), 2012/08/03 22:34:57]

sanity q: if line 279 caused decoded_audio_size -> 0 is it possible in any way
to mistakenly cause this condition to go true when perhaps it shouldn't have?

[acolwell GONE FROM CHROMIUM, 2012/08/03 23:14:01]

I don't believe so. Even if the decoder outputted samples and line 279 made
decoded_audio_size ->0 we are still at the end of the stream. Those samples
weren't supposed to be played anyway and input->IsEndOfStream() signals that no
more data is coming.

     // Create an end of stream output buffer.
     output = new DataBuffer(0);
-    output->SetTimestamp(input->GetTimestamp());
-    output->SetDuration(input->GetDuration());
   }
 
   // Decoding finished successfully, update stats and execute callback.
   statistics_cb_.Run(statistics);
-  if (output)
-    base::ResetAndReturn(&read_cb_).Run(kOk, output);
-  else
+
+  if (!output) {
     ReadFromDemuxerStream();
+    return;
+  }
+  base::ResetAndReturn(&read_cb_).Run(kOk, output);
 }
 
 void FFmpegAudioDecoder::ReadFromDemuxerStream() {
   DCHECK(!read_cb_.is_null());
 
   demuxer_stream_->Read(base::Bind(&FFmpegAudioDecoder::DecodeBuffer, this));
 }
 
 void FFmpegAudioDecoder::DecodeBuffer(
     DemuxerStream::Status status,
     const scoped_refptr<DecoderBuffer>& buffer) {
   DCHECK_EQ(status != DemuxerStream::kOk, !buffer) << status;
 
   // TODO(scherkus): fix FFmpegDemuxerStream::Read() to not execute our read
   // callback on the same execution stack so we can get rid of forced task post.
   message_loop_->PostTask(FROM_HERE, base::Bind(
       &FFmpegAudioDecoder::DoDecodeBuffer, this, status, buffer));
 }
 
-void FFmpegAudioDecoder::UpdateDurationAndTimestamp(
-    const Buffer* input,
-    DataBuffer* output) {
-  // Always calculate duration based on the actual number of samples decoded.
-  base::TimeDelta duration = CalculateDuration(output->GetDataSize());
-  output->SetDuration(duration);
-
-  // Use the incoming timestamp if it's valid.
-  if (input->GetTimestamp() != kNoTimestamp()) {
-    output->SetTimestamp(input->GetTimestamp());
-    estimated_next_timestamp_ = input->GetTimestamp() + duration;
-    return;
-  }
-
-  // Otherwise use an estimated timestamp and attempt to update the estimation
-  // as long as it's valid.
-  output->SetTimestamp(estimated_next_timestamp_);
-  if (estimated_next_timestamp_ != kNoTimestamp()) {
-    estimated_next_timestamp_ += duration;
-  }
+base::TimeDelta FFmpegAudioDecoder::GetNextOutputTimestamp() const {
+  DCHECK(output_timestamp_base_ != kNoTimestamp());
+  double decoded_us = (total_frames_decoded_ / samples_per_second_) *
+      base::Time::kMicrosecondsPerSecond;
+  return output_timestamp_base_ + base::TimeDelta::FromMicroseconds(decoded_us);
 }
-
-base::TimeDelta FFmpegAudioDecoder::CalculateDuration(int size) {
-  int64 denominator = ChannelLayoutToChannelCount(channel_layout_) *
-      bits_per_channel_ / 8 * samples_per_second_;
-  double microseconds = size /
-      (denominator / static_cast<double>(base::Time::kMicrosecondsPerSecond));
-  return base::TimeDelta::FromMicroseconds(static_cast<int64>(microseconds));
-}
-
 }  // namespace media