Introduce AudioDiscardHelper. Refactor audio decoders to use it.

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/callback_helpers.h"
 #include "base/single_thread_task_runner.h"
 #include "media/base/audio_buffer.h"
 #include "media/base/audio_bus.h"
 #include "media/base/audio_decoder_config.h"
-#include "media/base/audio_timestamp_helper.h"
+#include "media/base/audio_discard_helper.h"
 #include "media/base/bind_to_current_loop.h"
 #include "media/base/decoder_buffer.h"
 #include "media/base/limits.h"
 #include "media/base/sample_format.h"
 #include "media/ffmpeg/ffmpeg_common.h"
 #include "media/filters/ffmpeg_glue.h"
 
 namespace media {
 
 // Returns true if the decode result was end of stream.
@@ skipping 99 matching lines @@
   buffer.swap(reinterpret_cast<AudioBuffer**>(&opaque));
   frame->buf[0] = av_buffer_create(
       frame->data[0], buffer_size_in_bytes, ReleaseAudioBufferImpl, opaque, 0);
   return 0;
 }
 
 FFmpegAudioDecoder::FFmpegAudioDecoder(
     const scoped_refptr<base::SingleThreadTaskRunner>& task_runner)
     : task_runner_(task_runner),
       state_(kUninitialized),
-      av_sample_format_(0),
-      last_input_timestamp_(kNoTimestamp()),
-      output_frames_to_drop_(0) {}
+      av_sample_format_(0) {}
 
 FFmpegAudioDecoder::~FFmpegAudioDecoder() {
   DCHECK_EQ(state_, kUninitialized);
   DCHECK(!codec_context_);
   DCHECK(!av_frame_);
 }
 
 void FFmpegAudioDecoder::Initialize(const AudioDecoderConfig& config,
                                     const PipelineStatusCB& status_cb) {
   DCHECK(task_runner_->BelongsToCurrentThread());
@@ skipping 102 matching lines @@
   //     A decoding error occurs and decoding needs to stop.
   // kFlushCodec -> kDecodeFinished:
   //     When avcodec_decode_audio4() returns 0 data.
   // kFlushCodec -> kError:
   //     When avcodec_decode_audio4() errors out.
   // (any state) -> kNormal:
   //     Any time Reset() is called.
 
   // Make sure we are notified if http://crbug.com/49709 returns.  Issue also
   // occurs with some damaged files.
-  if (!buffer->end_of_stream() && buffer->timestamp() == kNoTimestamp() &&
-      output_timestamp_helper_->base_timestamp() == kNoTimestamp()) {
+  if (!buffer->end_of_stream() && buffer->timestamp() == kNoTimestamp()) {

[wolenetz, 2014/04/28 21:52:07]

Was it incorrect previously to also condition this error on what looks to me to
be equivalent to !discard_helper_->initialized()?

[DaleCurtis, 2014/04/28 22:03:51]

I've removed this exception as the only time this should happen is when the file
is broken.

     DVLOG(1) << "Received a buffer without timestamps!";
     decode_cb.Run(kDecodeError, NULL);
     return;
   }
 
-  if (!buffer->end_of_stream()) {
-    DCHECK(buffer->timestamp() != kNoTimestamp());
-    const bool first_buffer =
-        last_input_timestamp_ == kNoTimestamp() &&
-        output_timestamp_helper_->base_timestamp() == kNoTimestamp();
-    if (first_buffer && codec_context_->codec_id == AV_CODEC_ID_VORBIS &&
-        buffer->timestamp() < base::TimeDelta()) {
-      // Dropping frames for negative timestamps as outlined in section A.2
-      // in the Vorbis spec. http://xiph.org/vorbis/doc/Vorbis_I_spec.html
-      DCHECK_EQ(output_frames_to_drop_, 0);
-      output_frames_to_drop_ =
-          0.5 +
-          -buffer->timestamp().InSecondsF() * config_.samples_per_second();
-
-      // If we are dropping samples for Vorbis, the timeline always starts at 0.
-      output_timestamp_helper_->SetBaseTimestamp(base::TimeDelta());
-    } else {
-      if (first_buffer) {
-        output_timestamp_helper_->SetBaseTimestamp(buffer->timestamp());
-      } else if (buffer->timestamp() < last_input_timestamp_) {
-        const base::TimeDelta diff =
-            buffer->timestamp() - last_input_timestamp_;
-        DLOG(WARNING) << "Input timestamps are not monotonically increasing! "
-                      << " ts " << buffer->timestamp().InMicroseconds() << " us"
-                      << " diff " << diff.InMicroseconds() << " us";
-      }
-
-      last_input_timestamp_ = buffer->timestamp();
-    }
+  if (!buffer->end_of_stream() &&
+      !discard_helper_->initialized() &&
+      codec_context_->codec_id == AV_CODEC_ID_VORBIS &&
+      buffer->timestamp() < base::TimeDelta()) {
+    // Dropping frames for negative timestamps as outlined in section A.2
+    // in the Vorbis spec. http://xiph.org/vorbis/doc/Vorbis_I_spec.html
+    const int discard_frames =

[wolenetz, 2014/04/28 21:52:07]

If a stream has both a codec_delay and a negative initial timestamp, what should
behavior be? IIUC, this code would ignore the codec delay and just use the
negative timestamp's conversion to a discard framecount. Is that really correct?

[DaleCurtis, 2014/04/28 22:03:51]

For now this code is sufficient since it just implements existing behavior.  See
part II of this CL for how I resolve this.

+        discard_helper_->TimeDeltaToFrames(-buffer->timestamp());

[wolenetz, 2014/04/28 21:52:07]

Previously, we rounded vs currently using ceil. Which version is correct?

[DaleCurtis, 2014/04/28 22:03:51]

ceil().  It doesn't make sense to discard 1.5 frames. 

[wolenetz, 2014/04/28 22:37:56]

Suppose we have frame rate of 48000 frames/sec and encoder wishes to discard 1
frame:
1 frame = 20.833333us: would buffer->timestamp() in this case (limited to
microsecond granularity) be -21 or -20?
If -21, then discard_helper_->TimeDeltaToFrames(21) would be std::ceil(0.000021
* 48000) == std::ceil(1.008) == 2 frames. If -20, then it would be 1 frame. I'm
unfamiliar with the bitstream and the parsers, but this seems fragile to
off-by-one if not rounding.

[DaleCurtis, 2014/04/28 23:09:03]

You can see how the values are converted in ffmpeg_demuxer:

base::TimeDelta::FromMicroseconds(
    discard_padding_samples * 1000000.0 /
    audio_decoder_config().samples_per_second()));

Which gives a timestamp of 20us with your example data.  With vorbis and
negative timestamps, an example value from bear.ogv is -2902us => 127.9782
frames.

I don't have evidence that we always need ceil and the cases you point out do
seem a bit fragile, so I'll change this to a rounding solution.  I also found a
bug with my output time-stamping while evaluating this.

+    discard_helper_->Reset(discard_frames);

[wolenetz, 2014/04/28 21:52:07]

Is the following piece of that section A.2 Vorbis spec verified somewhere here?
::
"The number of samples to be discarded must not exceed the overlap-add span of
the first two audio packets."

[DaleCurtis, 2014/04/28 22:03:51]

This is not verified.  I'd fix that in a separate CL if that's something we need
to do.

[wolenetz, 2014/04/29 00:22:44]

Seems low-pri (we might reject streams that are slightly out-of-spec that we
might otherwise allow). File a bug and reference it if you think this is
something we need to do please.

[DaleCurtis, 2014/04/29 00:28:59]

I don't think it's necessary to worry about.

   }
 
   // Transition to kFlushCodec on the first end of stream buffer.
   if (state_ == kNormal && buffer->end_of_stream()) {
     state_ = kFlushCodec;
   }
 
   if (!FFmpegDecode(buffer)) {
     state_ = kError;
     decode_cb.Run(kDecodeError, NULL);
@@ skipping 11 matching lines @@
     decode_cb.Run(kNotEnoughData, NULL);
     return;
   }
 
   decode_cb.Run(kOk, queued_audio_.front());
   queued_audio_.pop_front();
 }
 
 bool FFmpegAudioDecoder::FFmpegDecode(
     const scoped_refptr<DecoderBuffer>& buffer) {
-
   DCHECK(queued_audio_.empty());
 
   AVPacket packet;
   av_init_packet(&packet);
   if (buffer->end_of_stream()) {
     packet.data = NULL;
     packet.size = 0;
   } else {
     packet.data = const_cast<uint8*>(buffer->data());
     packet.size = buffer->data_size();
   }
 
   // Each audio packet may contain several frames, so we must call the decoder
   // until we've exhausted the packet.  Regardless of the packet size we always
   // want to hand it to the decoder at least once, otherwise we would end up
   // skipping end of stream packets since they have a size of zero.
   do {
     int frame_decoded = 0;
-    int result = avcodec_decode_audio4(
+    const int result = avcodec_decode_audio4(
         codec_context_.get(), av_frame_.get(), &frame_decoded, &packet);
 
     if (result < 0) {
       DCHECK(!buffer->end_of_stream())
           << "End of stream buffer produced an error! "
           << "This is quite possibly a bug in the audio decoder not handling "
           << "end of stream AVPackets correctly.";
 
       DLOG(WARNING)
           << "Failed to decode an audio frame with timestamp: "
           << buffer->timestamp().InMicroseconds() << " us, duration: "
           << buffer->duration().InMicroseconds() << " us, packet size: "
           << buffer->data_size() << " bytes";
 
       break;
     }
 
     // Update packet size and data pointer in case we need to call the decoder
     // with the remaining bytes from this packet.
     packet.size -= result;
     packet.data += result;
 
     scoped_refptr<AudioBuffer> output;
-    int decoded_frames = 0;
-    int original_frames = 0;
-    int channels = DetermineChannels(av_frame_.get());
+    const int channels = DetermineChannels(av_frame_.get());
     if (frame_decoded) {
       if (av_frame_->sample_rate != config_.samples_per_second() ||
           channels != ChannelLayoutToChannelCount(config_.channel_layout()) ||
           av_frame_->format != av_sample_format_) {
         DLOG(ERROR) << "Unsupported midstream configuration change!"
                     << " Sample Rate: " << av_frame_->sample_rate << " vs "
                     << config_.samples_per_second()
                     << ", Channels: " << channels << " vs "
                     << ChannelLayoutToChannelCount(config_.channel_layout())
                     << ", Sample Format: " << av_frame_->format << " vs "
                     << av_sample_format_;
 
         // This is an unrecoverable error, so bail out.
         queued_audio_.clear();
         av_frame_unref(av_frame_.get());
         return false;
       }
 
       // Get the AudioBuffer that the data was decoded into. Adjust the number
       // of frames, in case fewer than requested were actually decoded.
       output = reinterpret_cast<AudioBuffer*>(
           av_buffer_get_opaque(av_frame_->buf[0]));
 
       DCHECK_EQ(ChannelLayoutToChannelCount(config_.channel_layout()),
                 output->channel_count());
-      original_frames = av_frame_->nb_samples;
-      int unread_frames = output->frame_count() - original_frames;
+      const int unread_frames = output->frame_count() - av_frame_->nb_samples;
       DCHECK_GE(unread_frames, 0);
       if (unread_frames > 0)
         output->TrimEnd(unread_frames);
 
-      // If there are frames to drop, get rid of as many as we can.
-      if (output_frames_to_drop_ > 0) {
-        int drop = std::min(output->frame_count(), output_frames_to_drop_);
-        output->TrimStart(drop);
-        output_frames_to_drop_ -= drop;
-      }
-
-      decoded_frames = output->frame_count();
       av_frame_unref(av_frame_.get());
     }
 
     // WARNING: |av_frame_| no longer has valid data at this point.
-
-    if (decoded_frames > 0) {
-      // Set the timestamp/duration once all the extra frames have been
-      // discarded.
-      output->set_timestamp(output_timestamp_helper_->GetTimestamp());
-      output->set_duration(
-          output_timestamp_helper_->GetFrameDuration(decoded_frames));
-      output_timestamp_helper_->AddFrames(decoded_frames);
-    } else if (IsEndOfStream(result, original_frames, buffer)) {
+    const int decoded_frames = frame_decoded ? output->frame_count() : 0;
+    if (IsEndOfStream(result, decoded_frames, buffer)) {
       DCHECK_EQ(packet.size, 0);
-      output = AudioBuffer::CreateEOSBuffer();
-    } else {
-      // In case all the frames in the buffer were dropped.
-      output = NULL;
+      queued_audio_.push_back(AudioBuffer::CreateEOSBuffer());
+    } else if (discard_helper_->ProcessBuffers(buffer, output)) {

[wolenetz, 2014/04/28 21:52:07]

Should any encoded buffer originating from ffmpeg_audio_decoder.cc *not* have
any discard_padding()? In previous code, it looks like any such padding was not
discarded. In current code, it will be discarded. Which is correct? If the
previous code was correct, include a DCHECK (or CHECK if that important) to flag
any occurrences of nonzero discard_padding() processed in this codepath?

[DaleCurtis, 2014/04/28 22:03:51]

It's possible for buffers originating from ffmpeg to have discard padding,
previously these were not handled correctly.  See part II of this CL
(specifically ffmpeg_demuxer) to see where this happens.

+      queued_audio_.push_back(output);
     }
-
-    if (output.get())
-      queued_audio_.push_back(output);
-
   } while (packet.size > 0);
 
   return true;
 }
 
 void FFmpegAudioDecoder::ReleaseFFmpegResources() {
   codec_context_.reset();
   av_frame_.reset();
 }
 
@@ skipping 27 matching lines @@
   if (!codec || avcodec_open2(codec_context_.get(), codec, NULL) < 0) {
     DLOG(ERROR) << "Could not initialize audio decoder: "
                 << codec_context_->codec_id;
     ReleaseFFmpegResources();
     state_ = kUninitialized;
     return false;
   }
 
   // Success!
   av_frame_.reset(av_frame_alloc());
-  output_timestamp_helper_.reset(
-      new AudioTimestampHelper(config_.samples_per_second()));
-  ResetTimestampState();
-
+  discard_helper_.reset(new AudioDiscardHelper(config_.samples_per_second()));
   av_sample_format_ = codec_context_->sample_fmt;
 
   if (codec_context_->channels !=
       ChannelLayoutToChannelCount(config_.channel_layout())) {
     DLOG(ERROR) << "Audio configuration specified "
                 << ChannelLayoutToChannelCount(config_.channel_layout())
                 << " channels, but FFmpeg thinks the file contains "
                 << codec_context_->channels << " channels";
     ReleaseFFmpegResources();
     state_ = kUninitialized;
     return false;
   }
 
-  output_frames_to_drop_ = config_.codec_delay();
+  ResetTimestampState();
   return true;
 }
 
 void FFmpegAudioDecoder::ResetTimestampState() {
-  output_timestamp_helper_->SetBaseTimestamp(kNoTimestamp());
-  last_input_timestamp_ = kNoTimestamp();
-  output_frames_to_drop_ = 0;
+  discard_helper_->Reset(config_.codec_delay());
 }
 
 }  // namespace media