Rewriting FFmpegAudioDecoder and eliminating DecoderBase.

media/filters/ffmpeg_audio_decoder.cc

 // Copyright (c) 2011 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 "media/base/data_buffer.h"
+#include "media/base/filter_host.h"
 #include "media/base/limits.h"
 #include "media/ffmpeg/ffmpeg_common.h"
-#include "media/filters/ffmpeg_demuxer.h"
-
-#if !defined(USE_SSE)
-#if defined(__SSE__) || defined(ARCH_CPU_X86_64) || _M_IX86_FP==1
-#define USE_SSE 1
-#else
-#define USE_SSE 0
-#endif
-#endif
-#if USE_SSE
-#include <xmmintrin.h>
-#endif
 
 namespace media {
 
-// Size of the decoded audio buffer.
-const size_t FFmpegAudioDecoder::kOutputBufferSize =
-    AVCODEC_MAX_AUDIO_FRAME_SIZE;
+// Returns true if the decode result was an error.
+static bool IsErrorResult(int result, int decoded_size) {
+  return result < 0 ||
+      decoded_size < 0 ||
+      decoded_size > AVCODEC_MAX_AUDIO_FRAME_SIZE;
+}
+
+// Returns true if the decode result produced audio samples.
+static bool ProducedAudioSamples(int decoded_size) {
+  return decoded_size > 0;
+}
+
+// Returns true if the decude result was a timestamp packet and not actual audio
+// data.
+static 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 decude result was end of stream.
+static 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(MessageLoop* message_loop)
-    : DecoderBase<AudioDecoder, Buffer>(message_loop),
+    : message_loop_(message_loop),
       codec_context_(NULL),
       config_(0, CHANNEL_LAYOUT_NONE, 0),
-      estimated_next_timestamp_(kNoTimestamp) {
+      estimated_next_timestamp_(kNoTimestamp),
+      decoded_audio_size_(AVCODEC_MAX_AUDIO_FRAME_SIZE),
+      decoded_audio_(new uint8[decoded_audio_size_]),
+      pending_reads_(0) {
 }
 
-FFmpegAudioDecoder::~FFmpegAudioDecoder() {
+FFmpegAudioDecoder::~FFmpegAudioDecoder() {}
+
+void FFmpegAudioDecoder::Flush(FilterCallback* callback) {
+  message_loop_->PostTask(
+      FROM_HERE,
+      NewRunnableMethod(this, &FFmpegAudioDecoder::DoFlush, callback));
 }
 
-void FFmpegAudioDecoder::DoInitialize(DemuxerStream* demuxer_stream,
-                                      bool* success,
-                                      Task* done_cb) {
-  base::ScopedTaskRunner done_runner(done_cb);
-  *success = false;
+void FFmpegAudioDecoder::Initialize(
+    DemuxerStream* stream,
+    FilterCallback* callback,
+    StatisticsCallback* stats_callback) {
+  // TODO(scherkus): change Initialize() signature to pass |stream| as a
+  // scoped_refptr<>.
+  scoped_refptr<DemuxerStream> ref_stream(stream);
+  message_loop_->PostTask(
+      FROM_HERE,
+      NewRunnableMethod(this, &FFmpegAudioDecoder::DoInitialize,
+                        ref_stream, callback, stats_callback));
+}
 
-  AVStream* av_stream = demuxer_stream->GetAVStream();
-  if (!av_stream) {
-    return;
-  }
+AudioDecoderConfig FFmpegAudioDecoder::config() {
+  return config_;
+}
+
+void FFmpegAudioDecoder::ProduceAudioSamples(scoped_refptr<Buffer> buffer) {

[acolwell GONE FROM CHROMIUM, 2011/09/13 22:09:28]

Remove buffer from method signature since it is never used & update
AudioRenderBase.

[scherkus (not reviewing), 2011/09/16 18:10:13]

Good suggestion! Deferring to later CL

+  message_loop_->PostTask(
+      FROM_HERE,
+      NewRunnableMethod(this, &FFmpegAudioDecoder::DoQueueOutput, buffer));
+}
+
+void FFmpegAudioDecoder::DoInitialize(
+    DemuxerStream* stream,
+    FilterCallback* callback,
+    StatisticsCallback* stats_callback) {
+  scoped_ptr<FilterCallback> c(callback);
+
+  demuxer_stream_ = stream;
+  AVStream* av_stream = demuxer_stream_->GetAVStream();
+  CHECK(av_stream);
+
+  stats_callback_.reset(stats_callback);
 
   // Grab the AVStream's codec context and make sure we have sensible values.
   codec_context_ = av_stream->codec;
   int bps = av_get_bits_per_sample_fmt(codec_context_->sample_fmt);
   if (codec_context_->channels <= 0 ||
       codec_context_->channels > Limits::kMaxChannels ||
       (codec_context_->channel_layout == 0 && codec_context_->channels > 2) ||
       bps <= 0 || bps > Limits::kMaxBitsPerSample ||
       codec_context_->sample_rate <= 0 ||
       codec_context_->sample_rate > Limits::kMaxSampleRate) {
-    DLOG(WARNING) << "Invalid audio stream -"
+    DLOG(ERROR) << "Invalid audio stream -"
         << " channels: " << codec_context_->channels

[acolwell GONE FROM CHROMIUM, 2011/09/13 22:09:28]

Indent

[scherkus (not reviewing), 2011/09/16 18:10:13]

Done.

         << " channel layout:" << codec_context_->channel_layout
         << " bps: " << bps
         << " sample rate: " << codec_context_->sample_rate;
+
+    host()->SetError(PIPELINE_ERROR_DECODE);
+    callback->Run();
     return;
   }
 
   // Serialize calls to avcodec_open().
   AVCodec* codec = avcodec_find_decoder(codec_context_->codec_id);
   if (!codec || avcodec_open(codec_context_, codec) < 0) {
+    DLOG(ERROR) << "Could not initialize audio decoder: "
+        << codec_context_->codec_id;

[acolwell GONE FROM CHROMIUM, 2011/09/13 22:09:28]

Indent

[scherkus (not reviewing), 2011/09/16 18:10:13]

Done.

+
+    host()->SetError(PIPELINE_ERROR_DECODE);
+    callback->Run();
     return;
   }
 
+  // Success!
   config_.bits_per_channel =
       av_get_bits_per_sample_fmt(codec_context_->sample_fmt);
   config_.channel_layout =
       ChannelLayoutToChromeChannelLayout(codec_context_->channel_layout,
                                          codec_context_->channels);
   config_.sample_rate = codec_context_->sample_rate;
 
-  // Prepare the output buffer.
-  output_buffer_.reset(static_cast<uint8*>(av_malloc(kOutputBufferSize)));
-  if (!output_buffer_.get()) {
-    host()->SetError(PIPELINE_ERROR_OUT_OF_MEMORY);
-    return;
-  }
-  *success = true;
+  callback->Run();
 }
 
-AudioDecoderConfig FFmpegAudioDecoder::config() {
-  return config_;
+void FFmpegAudioDecoder::DoFlush(FilterCallback* callback) {
+  avcodec_flush_buffers(codec_context_);
+  estimated_next_timestamp_ = kNoTimestamp;
+
+  callback->Run();
+  delete callback;
 }
 
-void FFmpegAudioDecoder::ProduceAudioSamples(scoped_refptr<Buffer> output) {
-  DecoderBase<AudioDecoder, Buffer>::PostReadTaskHack(output);
+void FFmpegAudioDecoder::DoQueueOutput(scoped_refptr<Buffer> output) {

[acolwell GONE FROM CHROMIUM, 2011/09/13 22:09:28]

const&

Rename to DoProduceAudioSamples().

+  output_buffers_.push_back(output);

[acolwell GONE FROM CHROMIUM, 2011/09/13 22:09:28]

Change this to a simple counter member variable since ProduceAudioSamples()
won't pass a buffer.

+  ReadFromDemuxerStream();

[acolwell GONE FROM CHROMIUM, 2011/09/13 22:09:28]

Do we want to do this if we have reached EOS?

 }
 
-void FFmpegAudioDecoder::DoSeek(base::TimeDelta time, Task* done_cb) {
-  avcodec_flush_buffers(codec_context_);
-  estimated_next_timestamp_ = kNoTimestamp;
-  done_cb->Run();
-  delete done_cb;
-}
-
-// ConvertAudioF32ToS32() converts float audio (F32) to int (S32) in place.
-// This is a temporary solution.
-// The purpose of this short term fix is to enable WMApro, which decodes to
-// float.
-// The audio driver has been tested by passing the float audio thru.
-// FFmpeg for ChromeOS only exposes U8, S16 and F32.
-// To properly expose new audio sample types at the audio driver layer, a enum
-// should be created to represent all suppported types, including types
-// for Pepper.  FFmpeg should be queried for type and passed along.
-
-// TODO(fbarchard): Remove this function.  Expose all FFmpeg types to driver.
-// TODO(fbarchard): If this function is kept, move it to audio_util.cc
-
-#if USE_SSE
-const __m128 kFloatScaler = _mm_set1_ps( 2147483648.0f );
-static void FloatToIntSaturate(float* p) {
-  __m128 a = _mm_set1_ps(*p);
-  a = _mm_mul_ss(a, kFloatScaler);
-  *reinterpret_cast<int32*>(p) = _mm_cvtss_si32(a);
-}
-#else
-const float kFloatScaler = 2147483648.0f;
-const int kMinSample = std::numeric_limits<int32>::min();
-const int kMaxSample = std::numeric_limits<int32>::max();
-const float kMinSampleFloat =
-    static_cast<float>(std::numeric_limits<int32>::min());
-const float kMaxSampleFloat =
-    static_cast<float>(std::numeric_limits<int32>::max());
-static void FloatToIntSaturate(float* p) {
-  float f = *p * kFloatScaler + 0.5f;
-  int sample;
-  if (f <= kMinSampleFloat) {
-    sample = kMinSample;
-  } else if (f >= kMaxSampleFloat) {
-    sample = kMaxSample;
-  } else {
-    sample = static_cast<int32>(f);
-  }
-  *reinterpret_cast<int32*>(p) = sample;
-}
-#endif
-static void ConvertAudioF32ToS32(void* buffer, int buffer_size) {
-  for (int i = 0; i < buffer_size / 4; ++i) {
-    FloatToIntSaturate(reinterpret_cast<float*>(buffer) + i);
-  }
-}
-
-void FFmpegAudioDecoder::DoDecode(Buffer* input) {
-  PipelineStatistics statistics;
+void FFmpegAudioDecoder::DoDecodeInput(scoped_refptr<Buffer> input) {

[acolwell GONE FROM CHROMIUM, 2011/09/13 22:09:28]

const&

Rename to DoOnReadComplete() to match Flush() -> DoFlush() pattern.

[scherkus (not reviewing), 2011/09/16 18:10:13]

Done.

+  DCHECK(!output_buffers_.empty());
+  DCHECK_GT(pending_reads_, 0);
+  pending_reads_--;
 
   // 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
-  // TODO(hclam): remove this once fixing the issue in FFmpeg.
   if (input->GetTimestamp() == kNoTimestamp &&
       estimated_next_timestamp_ == kNoTimestamp &&
       !input->IsEndOfStream()) {
-    DecoderBase<AudioDecoder, Buffer>::OnDecodeComplete(statistics);
+    ReadFromDemuxerStream();
     return;
   }
 
-  // Due to FFmpeg API changes we no longer have const read-only pointers.
   AVPacket packet;
   av_init_packet(&packet);
   packet.data = const_cast<uint8*>(input->GetData());

[acolwell GONE FROM CHROMIUM, 2011/09/13 22:09:28]

Mind if we make this a little more explicit for the EOS case?

if (input->IsEndOfStream()) {
    packet.data = NULL;
    packet.size = 0;
  } else {
    packet.data = const_cast<uint8*>(input->GetData());
    packet.size = input->GetDataSize();
  }

[scherkus (not reviewing), 2011/09/16 18:10:13]

Done.

   packet.size = input->GetDataSize();
 
+  PipelineStatistics statistics;
   statistics.audio_bytes_decoded = input->GetDataSize();
 
-  int16_t* output_buffer = reinterpret_cast<int16_t*>(output_buffer_.get());
-  int output_buffer_size = kOutputBufferSize;
-  int result = avcodec_decode_audio3(codec_context_,
-                                     output_buffer,
-                                     &output_buffer_size,
-                                     &packet);
+  int decoded_audio_size = decoded_audio_size_;
+  int result = avcodec_decode_audio3(
+      codec_context_, reinterpret_cast<int16_t*>(decoded_audio_.get()),
+      &decoded_audio_size, &packet);
 
-  if (codec_context_->sample_fmt == SAMPLE_FMT_FLT) {
-    ConvertAudioF32ToS32(output_buffer, output_buffer_size);
-  }
-
-  // TODO(ajwong): Consider if kOutputBufferSize should just be an int instead
-  // of a size_t.
-  if (result < 0 ||
-      output_buffer_size < 0 ||
-      static_cast<size_t>(output_buffer_size) > kOutputBufferSize) {
-    VLOG(1) << "Error decoding an audio frame with timestamp: "
-            << input->GetTimestamp().InMicroseconds() << " us, duration: "
-            << input->GetDuration().InMicroseconds() << " us, packet size: "
-            << input->GetDataSize() << " bytes";
-    DecoderBase<AudioDecoder, Buffer>::OnDecodeComplete(statistics);
+  if (IsErrorResult(result, decoded_audio_size)) {
+    DLOG(ERROR) << "Error decoding an audio frame with timestamp: "
+                << input->GetTimestamp().InMicroseconds() << " us, duration: "
+                << input->GetDuration().InMicroseconds() << " us, packet size: "
+                << input->GetDataSize() << " bytes";
+    ReadFromDemuxerStream();

[acolwell GONE FROM CHROMIUM, 2011/09/13 22:09:28]

Do we want to issue another read if this is IsEndOfStream() == true?

[scherkus (not reviewing), 2011/09/16 18:10:13]

Would that imply that the audio decoder in question is incorrectly reporting an
error given a flush packet of NULL/0 bytes?

I'll add a DCHECK

     return;
   }
 
-  // If we have decoded something, enqueue the result.
-  if (output_buffer_size) {
-    DataBuffer* result_buffer = new DataBuffer(output_buffer_size);
-    result_buffer->SetDataSize(output_buffer_size);
-    uint8* data = result_buffer->GetWritableData();
-    memcpy(data, output_buffer, output_buffer_size);
+  scoped_refptr<DataBuffer> output;
 
-    // We don't trust the demuxer, so always calculate the duration based on
-    // the actual number of samples decoded.
-    base::TimeDelta duration = CalculateDuration(output_buffer_size);
-    result_buffer->SetDuration(duration);
+  if (ProducedAudioSamples(decoded_audio_size)) {
+    // 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, decoded_audio_.get(), decoded_audio_size);
+  } else if (IsTimestampMarkerPacket(result, input)) {
+    // Nothing else to do here but update our estimation.
+    estimated_next_timestamp_ = input->GetTimestamp() + input->GetDuration();
+  } 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());
+  }
 
-    // Use an estimated timestamp unless the incoming buffer has a valid one.
-    if (input->GetTimestamp() == kNoTimestamp) {
-      result_buffer->SetTimestamp(estimated_next_timestamp_);
+  // Decoding finished successfully, update stats and execute callback.
+  stats_callback_->Run(statistics);
+  if (output) {
+    DCHECK_GT(output_buffers_.size(), 0u);

[acolwell GONE FROM CHROMIUM, 2011/09/13 22:09:28]

Replace w/ counter check and decrement.

[scherkus (not reviewing), 2011/09/16 18:10:13]

Future CL

+    output_buffers_.pop_front();
 
-      // Keep the estimated timestamp invalid until we get an incoming buffer
-      // with a valid timestamp.  This can happen during seeks, etc...
-      if (estimated_next_timestamp_ != kNoTimestamp) {
-        estimated_next_timestamp_ += duration;
-      }
-    } else {
-      result_buffer->SetTimestamp(input->GetTimestamp());
-      estimated_next_timestamp_ = input->GetTimestamp() + duration;
-    }
+    ConsumeAudioSamples(output);
+  }
+}
 
-    EnqueueResult(result_buffer);
-    DecoderBase<AudioDecoder, Buffer>::OnDecodeComplete(statistics);
+void FFmpegAudioDecoder::ReadFromDemuxerStream() {
+  DCHECK(!output_buffers_.empty())
+      << "Reads should only occur if there are output buffers.";
+
+  pending_reads_++;
+  demuxer_stream_->Read(base::Bind(&FFmpegAudioDecoder::OnReadComplete,
+                                   this));
+}
+
+void FFmpegAudioDecoder::OnReadComplete(Buffer* buffer) {
+  // TODO(scherkus): change DemuxerStream::Read() to use scoped_refptr<> for
+  // callback.
+  scoped_refptr<Buffer> ref_buffer(buffer);
+  message_loop_->PostTask(
+      FROM_HERE,
+      NewRunnableMethod(this, &FFmpegAudioDecoder::DoDecodeInput, ref_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());

[acolwell GONE FROM CHROMIUM, 2011/09/13 22:09:28]

Is this right given that there can be a codec delay where the first packet can
be passed in and not return any audio. When the second packet is passed in audio
comes out. Wouldn't this put the wrong timestamp on the data in this case?

[scherkus (not reviewing), 2011/09/16 18:10:13]

Good catch! This behaviour is now codified in a unit test but yeah let's fix
that :)

+    estimated_next_timestamp_ = input->GetTimestamp() + duration;
     return;

[acolwell GONE FROM CHROMIUM, 2011/09/13 22:09:28]

remove return.

Something still doesn't smell right in this function. If we hit the
IsResultError() if block in DoDecodeInput() won't this code start messing up the
timestamps? 

   }
 
-  // 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.  In this case we
-  // save the timestamp for later use.
-  if (result && !input->IsEndOfStream() &&
-      input->GetTimestamp() != kNoTimestamp &&
-      input->GetDuration() != kNoTimestamp) {
-    estimated_next_timestamp_ = input->GetTimestamp() + input->GetDuration();
-    DecoderBase<AudioDecoder, Buffer>::OnDecodeComplete(statistics);
-    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;
   }
-
-  // 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.
-  if (result == 0 && output_buffer_size == 0 && input->IsEndOfStream()) {
-    DataBuffer* result_buffer = new DataBuffer(0);
-    result_buffer->SetTimestamp(input->GetTimestamp());
-    result_buffer->SetDuration(input->GetDuration());
-    EnqueueResult(result_buffer);
-  }
-  DecoderBase<AudioDecoder, Buffer>::OnDecodeComplete(statistics);
 }
 
-base::TimeDelta FFmpegAudioDecoder::CalculateDuration(size_t size) {
-  int64 denominator = codec_context_->channels *
-      av_get_bits_per_sample_fmt(codec_context_->sample_fmt) / 8 *
-      codec_context_->sample_rate;
+base::TimeDelta FFmpegAudioDecoder::CalculateDuration(int size) {
+  int64 denominator = ChannelLayoutToChannelCount(config_.channel_layout) *
+      config_.bits_per_channel / 8 * config_.sample_rate;
   double microseconds = size /

[acolwell GONE FROM CHROMIUM, 2011/09/13 22:09:28]

why do we convert to a double and then back to int64 here? 

[scherkus (not reviewing), 2011/09/16 18:10:13]

I looked at the original code review but I can't recall why. I think it was a
cheesy way to work around my fears of integer overflow given the numbers we are
working with

http://codereview.chromium.org/113619/

       (denominator / static_cast<double>(base::Time::kMicrosecondsPerSecond));
   return base::TimeDelta::FromMicroseconds(static_cast<int64>(microseconds));
 }
 
-}  // namespace
+}  // namespace media