Refactor FFmpegAudioDecoder output timestamp logic.

media/filters/ffmpeg_audio_decoder.cc

Index: media/filters/ffmpeg_audio_decoder.cc
diff --git a/media/filters/ffmpeg_audio_decoder.cc b/media/filters/ffmpeg_audio_decoder.cc
index fb7b4a7609f704d9af3356da9d1c7e18505ae3c0..2a3faf4c7d6ffac521da0bbdd408f2d83aab6142 100644
--- a/media/filters/ffmpeg_audio_decoder.cc
+++ b/media/filters/ffmpeg_audio_decoder.cc
@@ -16,16 +16,6 @@
 
 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:
@@ -35,7 +25,6 @@ static inline bool IsEndOfStream(int result, int decoded_size, Buffer* input) {
   return result == 0 && decoded_size == 0 && input->IsEndOfStream();
 }
 
-
 FFmpegAudioDecoder::FFmpegAudioDecoder(
     const base::Callback<MessageLoop*()>& message_loop_cb)
     : message_loop_factory_cb_(message_loop_cb),
@@ -44,6 +33,10 @@ FFmpegAudioDecoder::FFmpegAudioDecoder(
       bits_per_channel_(0),
       channel_layout_(CHANNEL_LAYOUT_NONE),
       samples_per_second_(0),
+      bytes_per_frame_(0),
+      total_frames_base_(kNoTimestamp()),
+      total_frames_decoded_(0),
+      last_input_timestamp_(kNoTimestamp()),
       av_frame_(NULL) {
 }
 
@@ -146,13 +139,16 @@ void FFmpegAudioDecoder::DoInitialize(
   bits_per_channel_ = config.bits_per_channel();
   channel_layout_ = config.channel_layout();
   samples_per_second_ = config.samples_per_second();
-
+  bytes_per_frame_ =

[DaleCurtis, 2012/07/26 00:40:16]

codec_context_->channels is also available.

[acolwell GONE FROM CHROMIUM, 2012/07/26 01:21:34]

Done.

+      ChannelLayoutToChannelCount(channel_layout_) * 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();
+  total_frames_base_ = kNoTimestamp();
+  total_frames_decoded_ = 0;
+  last_input_timestamp_ = kNoTimestamp();
   closure.Run();
 }
 
@@ -181,14 +177,22 @@ void FFmpegAudioDecoder::DoDecodeBuffer(
     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() ||

[DaleCurtis, 2012/07/26 00:40:16]

DCHECK, CHECK is a bit heavy for a decoding loop, no?

[acolwell GONE FROM CHROMIUM, 2012/07/26 01:21:34]

Why? The old code produces incorrect behavior and/or require adding much more
logic for rebasing output buffer timestamps. The comments on 49709 seem to
indicate this is fixed, so I thought the CHECK() was a reasonable compromise to
smoke out content that triggers this bug again.

[DaleCurtis, 2012/07/26 01:49:16]

Fair enough, it happens on all content when it does AFAIK, thus it's likely only
to break when an FFmpeg roll happens, during which I run the tests in debug,
asan, tsan, valgrind, crazy mode which will find it then :)

We do see real world benefit in some cases by optimizing the decoder methods,
though possibly (probably?) not over one CHECK! Here's a neat graph which shows
Ronald's video memcpy removal patch and its effects on decoded fps:
http://build.chromium.org/f/chromium/perf/linux-release/media_tests_av_perf/r...

Up to you in any case.

+        total_frames_base_ != kNoTimestamp() ||
+        input->IsEndOfStream());
+
+  if (!input->IsEndOfStream()) {
+    if (last_input_timestamp_ != kNoTimestamp() &&
+        input->GetTimestamp() < last_input_timestamp_) {
+      DVLOG(1) << "Input timestamps are not monotonically increasing! "
+               << " ts " << input->GetTimestamp().InSecondsF()

[DaleCurtis, 2012/07/26 00:40:16]

Why not InMillisecondsF ? Are they generally O(seconds) different?

[acolwell GONE FROM CHROMIUM, 2012/07/26 01:21:34]

I just wanted the timestamp and difference to be in the same units.

[scherkus (not reviewing), 2012/07/26 01:43:08]

I usually do InMicroseconds() myself! (see FFmpegVideoDecoder)

[acolwell GONE FROM CHROMIUM, 2012/07/27 20:45:29]

ok. I'll use microseconds to match the FFmpegVideoDecoder code.

+               << " diff "
+               << (input->GetTimestamp() - last_input_timestamp_).InSecondsF();
+      base::ResetAndReturn(&read_cb_).Run(kDecodeError, NULL);

[scherkus (not reviewing), 2012/07/26 01:43:08]

I'd be interested in knowing if/when this happens

previously we would have gone back in time but continued playing where as now we
will error the whole player

[acolwell GONE FROM CHROMIUM, 2012/07/27 20:45:29]

This is intended to smoke out content that violates the assumption that input
timestamps always move forward. If we encounter such content then we can
determine whether it is a valid case or a bug in the demuxing code somewhere. I
just don't want to allow this situation to effect downstream code like the clock
anymore.

+      return;
+    }
+    last_input_timestamp_ = input->GetTimestamp();
   }
 
   AVPacket packet;
@@ -221,6 +225,14 @@ void FFmpegAudioDecoder::DoDecodeBuffer(
     return;
   }
 
+  if (result > 0)
+    DCHECK_EQ(result, input->GetDataSize());
+
+  if (total_frames_base_ == kNoTimestamp()) {
+    DCHECK(input->GetTimestamp() != kNoTimestamp());

[scherkus (not reviewing), 2012/07/26 01:43:08]

somewhat of a longshot, but what if we seeked to the verrrrrrrrrrrrrrrrrrry edge
of the file?

if Reset() was called here (clearing total_frames_base_) and our first buffer
was EOS, this would trigger as would the DCHECK inside GetNextOutputTimestamp()
as well

[acolwell GONE FROM CHROMIUM, 2012/07/27 20:45:29]

Good catch! Added IsEndOfStream() check.

+    total_frames_base_ = input->GetTimestamp();
+  }
+
   int decoded_audio_size = 0;
   if (frame_decoded) {
     int output_sample_rate = av_frame_->sample_rate;
@@ -239,21 +251,24 @@ void FFmpegAudioDecoder::DoDecodeBuffer(
   scoped_refptr<DataBuffer> output;
 
   if (decoded_audio_size > 0) {
+    DCHECK_EQ(decoded_audio_size % bytes_per_frame_, 0)

[DaleCurtis, 2012/07/26 00:40:16]

Is this true for all the codecs we support? I saw some comments on the DV codec
recently indicating that it didn't always output full frames. Granted that was
video and not audio.

[acolwell GONE FROM CHROMIUM, 2012/07/26 01:21:34]

If this isn't true then I'm pretty sure other code would break since it means
that we'd output only some of the channels. I'm just trying to make this
expectation explicit so we can catch any unexpected behavior.

[scherkus (not reviewing), 2012/07/26 01:43:08]

if it isn't I'd like to know about it :)

+        << "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);
 
-    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)) {
     // Create an end of stream output buffer.
     output = new DataBuffer(0);
-    output->SetTimestamp(input->GetTimestamp());
-    output->SetDuration(input->GetDuration());
+    output->SetTimestamp(GetNextOutputTimestamp());
   }
 
   // Decoding finished successfully, update stats and execute callback.
@@ -281,34 +296,13 @@ void FFmpegAudioDecoder::DecodeBuffer(
       &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::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));
+base::TimeDelta FFmpegAudioDecoder::GetNextOutputTimestamp() const {
+  DCHECK(total_frames_base_ != kNoTimestamp());

[DaleCurtis, 2012/07/26 00:40:16]

DCHECK_NE ?

[acolwell GONE FROM CHROMIUM, 2012/07/26 01:21:34]

Compiler gets angry because there isn't a << operator for TimeDelta.

+  int64 decoded_seconds = total_frames_decoded_ / samples_per_second_;
+  int64 fraction_of_a_second = total_frames_decoded_ % samples_per_second_;
+  int64 decoded_microseconds =

[DaleCurtis, 2012/07/26 00:40:16]

Is int64 precise enough for this? Why not double?

[acolwell GONE FROM CHROMIUM, 2012/07/26 01:21:34]

This is precise for microsecond resolution which is what I'm going for. I'll
change the code to use a double though since it is unlikely that
total_frames_decoded_ would ever get high enough to cause the double to start
loosing precision and because that is more readable. I'm showing my age with
these overflow avoidance tricks from the 32-bit & slow float days.

+      1000000 * fraction_of_a_second / samples_per_second_;
+  return total_frames_base_ + base::TimeDelta::FromSeconds(decoded_seconds)
+      + base::TimeDelta::FromMicroseconds(decoded_microseconds);
 }
-
 }  // namespace media