Add MediaCodecAudioDecoder implementation

media/filters/android/media_codec_audio_decoder.cc

Index: media/filters/android/media_codec_audio_decoder.cc
diff --git a/media/filters/android/media_codec_audio_decoder.cc b/media/filters/android/media_codec_audio_decoder.cc
index 0f4d35296fd9902ee0d0d991e463c53f9fd96ac8..4b9fd2da8de9b1d378ed1547ce84a36d20c54576 100644
--- a/media/filters/android/media_codec_audio_decoder.cc
+++ b/media/filters/android/media_codec_audio_decoder.cc
@@ -4,11 +4,69 @@
 
 #include "media/filters/android/media_codec_audio_decoder.h"
 
+#include "base/android/build_info.h"
+#include "base/bind.h"
+#include "base/callback_helpers.h"
 #include "base/logging.h"
 #include "base/thread_task_runner_handle.h"
 
+#include "media/base/android/sdk_media_codec_bridge.h"
+#include "media/base/audio_buffer.h"
+#include "media/base/bind_to_current_loop.h"
+
 namespace media {
 
+namespace {
+
+// Android MediaCodec can only output 16bit PCM audio.
+const int kBytesPerOutputSample = 2;
+
+inline const base::TimeDelta DecodePollDelay() {
+  return base::TimeDelta::FromMilliseconds(10);
+}
+
+inline const base::TimeDelta NoWaitTimeOut() {
+  return base::TimeDelta::FromMicroseconds(0);
+}
+
+inline const base::TimeDelta IdleTimerTimeOut() {
+  return base::TimeDelta::FromSeconds(1);
+}
+
+inline int GetChannelCount(const AudioDecoderConfig& config) {
+  return ChannelLayoutToChannelCount(config.channel_layout());
+}
+
+scoped_ptr<MediaCodecBridge> CreateMediaCodec(
+    const AudioDecoderConfig& config) {
+  DVLOG(1) << __FUNCTION__ << ": config:" << config.AsHumanReadableString();
+
+  scoped_ptr<MediaCodecBridge> media_codec(

[xhwang, 2016/02/12 10:15:05]

You can have "scoped_ptr<AudioCodecBridge> audio_codec_bridge" here. Then you
don't need to cast on line 51.

[Tima Vaisburd, 2016/02/13 01:31:24]

How to return scoped_ptr<MediaCodecBridge> on l.65? My compilation complains
that it cannot convert scoped_ptr<Audio> into scoped_ptr<Media>, although Audio
is derived from Media. Maybe we can just return raw pointer?

[xhwang, 2016/02/13 07:43:13]

Can you try?
return std::move(media_codec);

[Tima Vaisburd, 2016/02/15 23:25:42]

That works!

+      AudioCodecBridge::Create(config.codec()));
+  if (!media_codec) {
+    DVLOG(0) << __FUNCTION__ << " failed: cannot create AudioCodecBridge";
+    return nullptr;
+  }
+
+  AudioCodecBridge* audio_codec_bridge =
+      static_cast<AudioCodecBridge*>(media_codec.get());
+  DCHECK(audio_codec_bridge);
+
+  const bool play_audio = false;  // Do not create AudioTrack object.
+  jobject media_crypto = nullptr;
+
+  if (!audio_codec_bridge->ConfigureAndStart(config, play_audio,
+                                             media_crypto)) {
+    DVLOG(0) << __FUNCTION__ << " failed: cannot configure audio codec for "
+             << config.AsHumanReadableString();
+    return nullptr;
+  }
+
+  return media_codec;
+}
+
+}  // namespace (anonymous)
+
 MediaCodecAudioDecoder::MediaCodecAudioDecoder(
     const scoped_refptr<base::SingleThreadTaskRunner>& task_runner)
     : task_runner_(task_runner),
@@ -21,7 +79,12 @@ MediaCodecAudioDecoder::MediaCodecAudioDecoder(
 MediaCodecAudioDecoder::~MediaCodecAudioDecoder() {
   DVLOG(1) << __FUNCTION__;
 
-  NOTIMPLEMENTED();
+  media_codec_.reset();

[xhwang, 2016/02/12 10:15:05]

Is this needed? Can we just let the scoped_ptr dtor do the work?

[Tima Vaisburd, 2016/02/13 01:31:24]

As far as I understand this is not needed right now because there is no specific
need to destroy MediaCodec before any other field. I would still prefer to keep
explicit resets, I find myself searching for it when looking into various bugs
as MediaCodec is quite fragile.

[xhwang, 2016/02/13 07:43:13]

Acknowledged.

+
+  // Report kAborted status for all frames in the input queue.
+  // That includes pending EOS.
+  for (const auto& entry : input_queue_)
+    entry.second.Run(kAborted);
 }
 
 std::string MediaCodecAudioDecoder::GetDisplayName() const {
@@ -33,47 +96,322 @@ void MediaCodecAudioDecoder::Initialize(const AudioDecoderConfig& config,
                                         const InitCB& init_cb,
                                         const OutputCB& output_cb) {
   DVLOG(1) << __FUNCTION__ << ": " << config.AsHumanReadableString();
+  DCHECK_EQ(state_, STATE_UNINITIALIZED);
+
+  InitCB bound_init_cb = BindToCurrentLoop(init_cb);
 
-  NOTIMPLEMENTED();
+  if (config.is_encrypted()) {
+    DVLOG(1) << "Encrypted streams are not supported by " << GetDisplayName();
+    bound_init_cb.Run(false);
+    return;
+  }
+
+  // We can support only the codecs that AudioCodecBridge can decode.
+  const bool is_codec_supported = config.codec() == kCodecVorbis ||
+                                  config.codec() == kCodecAAC ||
+                                  config.codec() == kCodecOpus;
+  if (!is_codec_supported) {
+    DVLOG(1) << "Unsuported codec " << GetCodecName(config.codec());
+    bound_init_cb.Run(false);
+    return;
+  }
+
+  media_codec_ = std::move(CreateMediaCodec(config));

[xhwang, 2016/02/12 10:15:05]

Will it work if you remove std::move?

[Tima Vaisburd, 2016/02/13 01:31:24]

Yes! Unnamed temporary?

[xhwang, 2016/02/13 07:43:13]

rvalue:
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped...

[Tima Vaisburd, 2016/02/15 23:25:42]

Thank you for the link.

+  if (!media_codec_) {
+    bound_init_cb.Run(false);
+    return;
+  }
+
+  config_ = config;
+  output_cb_ = BindToCurrentLoop(output_cb);
+
+  SetState(STATE_READY);
+  bound_init_cb.Run(true);
 }
 
 void MediaCodecAudioDecoder::Decode(const scoped_refptr<DecoderBuffer>& buffer,
                                     const DecodeCB& decode_cb) {
-  NOTIMPLEMENTED();
+  DecodeCB bound_decode_cb = BindToCurrentLoop(decode_cb);
+
+  if (state_ == STATE_ERROR) {
+    DVLOG(2) << __FUNCTION__ << " " << buffer->AsHumanReadableString()
+             << ": Error state, dropping buffer";
+    bound_decode_cb.Run(kDecodeError);
+    return;
+  }
+
+  DVLOG(2) << __FUNCTION__ << " " << buffer->AsHumanReadableString();
+
+  DCHECK_EQ(state_, STATE_READY) << " unexpected state " << AsString(state_);
+
+  input_queue_.push_back(std::make_pair(buffer, bound_decode_cb));
+
+  DoIOTask();
 }
 
 void MediaCodecAudioDecoder::Reset(const base::Closure& closure) {
   DVLOG(1) << __FUNCTION__;
 
-  NOTIMPLEMENTED();
+  io_timer_.Stop();
+
+  // Report kAborted status for all frames in the input queue.
+  for (const auto& entry : input_queue_)
+    entry.second.Run(kAborted);
+
+  input_queue_.clear();
+
+  // Flush if we can, otherwise completely recreate and reconfigure the codec.
+  // Prior to JB-MR2, flush() had several bugs (b/8125974, b/8347958).
+  bool success = false;
+  if (state_ != STATE_ERROR && state_ != STATE_DRAINED &&
+      base::android::BuildInfo::GetInstance()->sdk_int() < 18) {

[xhwang, 2016/02/12 10:15:05]

Why we only Reset/Flush when SDK < 18?

[Tima Vaisburd, 2016/02/13 01:31:24]

Bug! Thank you, should be SDK >= 18. I updated the comment as well.

+    // media_codec_->Reset() calls MediaCodec.flush().
+    success = (media_codec_->Reset() == MEDIA_CODEC_OK);
+  }
+
+  if (!success) {
+    media_codec_.reset();
+    media_codec_ = std::move(CreateMediaCodec(config_));
+    success = !!media_codec_;
+  }
+
+  SetState(success ? STATE_READY : STATE_ERROR);
+
+  task_runner_->PostTask(FROM_HERE, closure);
 }
 
 void MediaCodecAudioDecoder::OnKeyAdded() {
   DVLOG(1) << __FUNCTION__;
 
-  NOTIMPLEMENTED();
+  if (state_ == STATE_WAITING_FOR_KEY)
+    SetState(STATE_READY);
+
+  DoIOTask();
 }
 
 void MediaCodecAudioDecoder::DoIOTask() {
-  NOTIMPLEMENTED();
+  if (state_ == STATE_ERROR)
+    return;
+
+  const bool did_input = QueueInput();
+  const bool did_output = DequeueOutput();
+
+  ManageTimer(did_input || did_output);
 }
 
 bool MediaCodecAudioDecoder::QueueInput() {
   DVLOG(2) << __FUNCTION__;
 
-  NOTIMPLEMENTED();
+  if (input_queue_.empty())
+    return false;
+
+  if (state_ == STATE_WAITING_FOR_KEY)
+    return false;
+
+  if (state_ == STATE_DRAINING)
+    return false;
+
+  scoped_refptr<DecoderBuffer> decoder_buffer = input_queue_.front().first;
+  const DecodeCB& decode_cb = input_queue_.front().second;
+
+  int input_buf_index = pending_input_buf_index_;
+
+  // Do not dequeue a new input buffer if we failed with MEDIA_CODEC_NO_KEY.
+  // That status does not return the input buffer back to the pool of
+  // available input buffers. We have to reuse it in QueueSecureInputBuffer().
+  if (input_buf_index == -1) {
+    media::MediaCodecStatus status =
+        media_codec_->DequeueInputBuffer(NoWaitTimeOut(), &input_buf_index);
+    switch (status) {
+      case media::MEDIA_CODEC_DEQUEUE_INPUT_AGAIN_LATER:
+        return false;
+      case media::MEDIA_CODEC_ERROR:
+        DVLOG(1) << __FUNCTION__ << ": MEDIA_CODEC_ERROR from DequeInputBuffer";
+        SetState(STATE_ERROR);
+
+        // Report an error to the pipeline.
+        decode_cb.Run(kDecodeError);

[xhwang, 2016/02/12 10:15:04]

Do you want to clear |input_queue_| and fire all pending decode callbacks
instead?

If you do that, you actually don't need line 214 and line 215.

[Tima Vaisburd, 2016/02/13 01:31:24]

I created DequeueInputBuffer() and EnqueueInputBuffer(), but slightly
differently than you've suggested. Please take a look.

Right now the code is quite conservative in removing the stuff from
|input_queue_|. This way potentially the caller might try to recover with
Reset() and continue.

On the other hand, as far as I know no one is doing that, and the errors from
DequeueOutput() would require special handling.

I prefer to keep it as is right now and revisit at the time of a planned common
part refactoring, but let me know what you think.

[xhwang, 2016/02/13 07:43:13]

In Reset() you'll clear |input_queue_| anyways, right?

When a decode error happens, there's pretty no way to keep decoding the rest of
the buffers. In the video case, the next buffer may be a B frame, which cannot
be decoded if the previous frame isn't decoded successfully.

Also, holding callbacks seems bad in general. Currently VideoDecoder supports
parallel decoding and it requires all decode_cb to be fired by the decoder, even
if the caller doesn't call into the decoder again [1].

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/video_d...

[Tima Vaisburd, 2016/02/15 23:25:42]

Agreed. Created a new method ClearInputQueue() and called it in the error
situations (except after DequeueOutputBuffer() error and delete/recreate error
in Reset()).

+        return false;
+      case media::MEDIA_CODEC_OK:
+        break;
+      default:
+        NOTREACHED() << "Unknown DequeueInputBuffer status " << status;
+        return false;
+    }
+  }
+
+  DCHECK_NE(input_buf_index, -1);

[xhwang, 2016/02/12 10:15:04]

QueueInput() is too long. Can you wrap line 217-243 to a DequeueInputBuffer()
method?

[Tima Vaisburd, 2016/02/13 01:31:24]

See my comment above.

+

[xhwang, 2016/02/12 10:15:05]

See above. Move line 214 here.

You don't need line 215 until you get an output successfully.

[Tima Vaisburd, 2016/02/13 01:31:24]

See my comment above.

+  if (decoder_buffer->end_of_stream()) {
+    media_codec_->QueueEOS(input_buf_index);
+
+    // After queueing EOS we need to flush decoder, i.e. receive this EOS at
+    // the output, and then call corresponding decoder_cb.
+    // Keep the EOS buffer in the input queue.
+    SetState(STATE_DRAINING);
+    return true;
+  }
+
+  // Make sure our DecoderBuffer is not EOS, otherwise we can't call any method.
+  DCHECK(!decoder_buffer->end_of_stream());

[xhwang, 2016/02/12 10:15:05]

Not needed given line 245 and 252?

[Tima Vaisburd, 2016/02/13 01:31:24]

Removed.

+
+  media::MediaCodecStatus status = MEDIA_CODEC_OK;
+  const DecryptConfig* decrypt_config = decoder_buffer->decrypt_config();
+  if (decrypt_config) {
+    // If pending_input_buf_index_ != -1 the input buffer is already filled up,
+    // no need to copy it again.
+    const uint8_t* memory =
+        (pending_input_buf_index_ == -1) ? decoder_buffer->data() : nullptr;
+
+    DVLOG(2) << __FUNCTION__ << ": QueueSecureInputBuffer:"
+             << " index:" << input_buf_index
+             << " pts:" << decoder_buffer->timestamp()
+             << " size:" << decoder_buffer->data_size();
+
+    status = media_codec_->QueueSecureInputBuffer(
+        input_buf_index, memory, decoder_buffer->data_size(),
+        decrypt_config->key_id(), decrypt_config->iv(),
+        decrypt_config->subsamples(), decoder_buffer->timestamp());
+
+  } else {
+    DVLOG(2) << __FUNCTION__ << ": QueueInputBuffer:"
+             << " index:" << input_buf_index
+             << " pts:" << decoder_buffer->timestamp()
+             << " size:" << decoder_buffer->data_size();
+
+    status = media_codec_->QueueInputBuffer(
+        input_buf_index, decoder_buffer->data(), decoder_buffer->data_size(),
+        decoder_buffer->timestamp());
+  }
+
+  pending_input_buf_index_ = -1;
+
+  switch (status) {
+    case MEDIA_CODEC_ERROR:
+      DVLOG(0) << __FUNCTION__ << ": MEDIA_CODEC_ERROR from QueueInputBuffer";
+      SetState(STATE_ERROR);
+
+      // Report an error to the pipeline.
+      decode_cb.Run(kDecodeError);
+      return false;
+
+    case MEDIA_CODEC_NO_KEY:
+      // Keep trying to enqueue the same input buffer.
+      // The buffer is owned by us (not the MediaCodec) and is filled with data.
+      DVLOG(1) << "QueueSecureInputBuffer failed: MEDIA_CODEC_NO_KEY";
+      pending_input_buf_index_ = input_buf_index;
+      SetState(STATE_WAITING_FOR_KEY);
+      return false;
+
+    case MEDIA_CODEC_OK:
+      break;
+
+    default:
+      NOTREACHED() << "Unknown Queue(Secure)InputBuffer status " << status;
+      break;

[xhwang, 2016/02/12 10:15:05]

Should we go to STATE_ERROR and bail out?

[Tima Vaisburd, 2016/02/13 01:31:24]

Done.

+  }

[xhwang, 2016/02/12 10:15:05]

ditto, wrap this into a QueueInputBuffer() method?

[Tima Vaisburd, 2016/02/13 01:31:24]

Done.

+
+  // Although audio_decoder.h says "Once the buffer is decoded the decoder calls
+  // |decode_cb|", we call |decode_cb| when the buffer is accepted by

[xhwang, 2016/02/12 10:15:05]

The comment actually means that once the buffer is accepted by the decoder. We
can update audio_decoder.h to make this more clear.

[Tima Vaisburd, 2016/02/13 01:31:24]

Done.

+  // MediaCodec, not when it is completely decoded. It seems consistent to what
+  // other decoders do.
+  decode_cb.Run(kOk);
+  input_queue_.pop_front();
   return true;
 }
 
 bool MediaCodecAudioDecoder::DequeueOutput() {
   DVLOG(2) << __FUNCTION__;
 
-  NOTIMPLEMENTED();
-  return true;
+  DCHECK(media_codec_);

[xhwang, 2016/02/12 10:15:04]

Not needed. If this fires we'll just crash on line 332.

[Tima Vaisburd, 2016/02/13 01:31:24]

Done.

I've always wanted to ask though: there are various precondition DCHECKs,
including pointer testing. They would produce helpful debug message if violated.
Why this particular kind is redundant?

+
+  MediaCodecStatus status;
+  OutputBufferInfo out;
+  bool work_done = false;

[xhwang, 2016/02/12 10:15:05]

use |did_work| to be consistent?

[Tima Vaisburd, 2016/02/13 01:31:24]

Done.

+  do {
+    status = media_codec_->DequeueOutputBuffer(NoWaitTimeOut(), &out.buf_index,
+                                               &out.offset, &out.size, &out.pts,
+                                               &out.is_eos, &out.is_key_frame);
+
+    switch (status) {
+      case MEDIA_CODEC_OUTPUT_BUFFERS_CHANGED:
+        // Output buffers are replaced in MediaCodecBridge, nothing to do.
+        DVLOG(2) << __FUNCTION__ << " MEDIA_CODEC_OUTPUT_BUFFERS_CHANGED";
+        work_done = true;
+        break;
+
+      case MEDIA_CODEC_OUTPUT_FORMAT_CHANGED:
+        DVLOG(2) << __FUNCTION__ << " MEDIA_CODEC_OUTPUT_FORMAT_CHANGED";
+        OnOutputFormatChanged();
+        work_done = true;
+        break;
+
+      case MEDIA_CODEC_OK:
+        // We got the decoded frame.
+        if (out.is_eos) {
+          media_codec_->ReleaseOutputBuffer(out.buf_index, false);
+
+          DCHECK_EQ(state_, STATE_DRAINING);
+
+          // Report the end of decoding for EOS DecoderBuffer.
+          const DecodeCB& decode_cb = input_queue_.front().second;
+          decode_cb.Run(kOk);
+
+          // Remove the EOS buffer from the input queue.
+          input_queue_.pop_front();
+

[xhwang, 2016/02/12 10:15:05]

input_queue_ should be empty now?

[Tima Vaisburd, 2016/02/13 01:31:24]

For this audio decoder probably yes, but in general case we can have input
buffers after EOS. AVDA, for instance, uses artificial EOSes diring the
resolution change.

+          // media_decoder_job.cc says: once output EOS has occurred, we should
+          // not be asked to decode again. Have a separate state for this case.
+          SetState(STATE_DRAINED);

[xhwang, 2016/02/12 10:15:05]

The switch/case block is too long. Wrap this block to OnEosFrame()?

[Tima Vaisburd, 2016/02/13 01:31:24]

Yes, I called OnDecodedEos() and renamed OnDecoderFrame() ->
OnDecodedContentFrame() to stress the decoded side.

+        } else {
+          // Process the real decoded frame.
+          OnDecodedFrame(out);
+        }
+
+        work_done = true;
+        break;
+
+      case MEDIA_CODEC_DEQUEUE_OUTPUT_AGAIN_LATER:
+        // Nothing to do.
+        DVLOG(2) << __FUNCTION__ << " MEDIA_CODEC_DEQUEUE_OUTPUT_AGAIN_LATER";
+        break;
+
+      case MEDIA_CODEC_ERROR:
+        DVLOG(0) << __FUNCTION__
+                 << ": MEDIA_CODEC_ERROR from DequeueOutputBuffer";
+
+        // Next Decode() will report the error to the pipeline.
+        SetState(STATE_ERROR);
+        break;
+
+      default:
+        NOTREACHED();
+        break;
+    }
+  } while (status != MEDIA_CODEC_DEQUEUE_OUTPUT_AGAIN_LATER &&
+           status != MEDIA_CODEC_ERROR && !out.is_eos);
+
+  return work_done;
 }
 
-void MediaCodecAudioDecoder::ManageTimer(bool start) {
-  NOTIMPLEMENTED();
+void MediaCodecAudioDecoder::ManageTimer(bool did_work) {
+  bool should_be_running = true;
+
+  base::TimeTicks now = base::TimeTicks::Now();
+  if (!did_work) {
+    // Make sure that we have done work recently enough, else stop the timer.
+    if (now - idle_time_begin_ > IdleTimerTimeOut())

[xhwang, 2016/02/12 10:15:05]

The initial value of idle_time_begin_ would be 0. So if for the first decode we
didn't do any work, |now - idle_time_begin_| could be pretty large. Then we'll
not start the timer. Is this expected?

Maybe instead of checking !did_work, we should check

if (!did_work && !idle_time_begin_)

[Tima Vaisburd, 2016/02/13 01:31:24]

I think you mean
  if (!did_work && idle_time_begin_)

Done.

+      should_be_running = false;
+  } else {
+    idle_time_begin_ = now;
+  }
+
+  if (should_be_running && !io_timer_.IsRunning()) {
+    io_timer_.Start(FROM_HERE, DecodePollDelay(), this,
+                    &MediaCodecAudioDecoder::DoIOTask);
+  } else if (!should_be_running && io_timer_.IsRunning()) {
+    io_timer_.Stop();
+  }
 }
 
 void MediaCodecAudioDecoder::SetState(State new_state) {
@@ -82,22 +420,50 @@ void MediaCodecAudioDecoder::SetState(State new_state) {
   state_ = new_state;
 }
 
-scoped_ptr<MediaCodecBridge> MediaCodecAudioDecoder::ConfigureMediaCodec(
-    const AudioDecoderConfig& config) {
-  NOTIMPLEMENTED();
-  return nullptr;
-}
-
 void MediaCodecAudioDecoder::OnDecodedFrame(const OutputBufferInfo& out) {
+  DCHECK_NE(out.size, 0U);
+  DCHECK_GE(out.buf_index, 0);  // |out->buf_index| must be valid
+  DCHECK(media_codec_);

[xhwang, 2016/02/12 10:15:05]

nit: usually we DVLOG first, then DCHECK.

[Tima Vaisburd, 2016/02/13 01:31:24]

My bad! Done.

+
   DVLOG(2) << __FUNCTION__ << " pts:" << out.pts;
 
-  NOTIMPLEMENTED();
+  // Create AudioOutput buffer based on configuration.
+  const int channel_count = GetChannelCount(config_);
+  const int bytes_per_frame = kBytesPerOutputSample * channel_count;
+  const size_t frame_count = out.size / bytes_per_frame;
+
+  scoped_refptr<AudioBuffer> audio_buffer = AudioBuffer::CreateBuffer(
+      kSampleFormatS16, config_.channel_layout(), channel_count,
+      config_.samples_per_second(), frame_count);
+
+  // Set timestamp.
+  audio_buffer->set_timestamp(out.pts);
+
+  // Copy data into AudioBuffer.
+  DCHECK_LE(out.size, audio_buffer->data_size());
+
+  media_codec_->CopyFromOutputBuffer(out.buf_index, out.offset,
+                                     audio_buffer->channel_data()[0],
+                                     audio_buffer->data_size());
+
+  // Release MediaCodec output buffer.
+  media_codec_->ReleaseOutputBuffer(out.buf_index, false);
+
+  // Call the |output_cb_|.
+  output_cb_.Run(audio_buffer);
 }
 
 void MediaCodecAudioDecoder::OnOutputFormatChanged() {
   DVLOG(2) << __FUNCTION__;
 
-  NOTIMPLEMENTED();
+  // We do not support the change of sampling rate on the fly
+  int new_sampling_rate = media_codec_->GetOutputSamplingRate();
+  if (new_sampling_rate != config_.samples_per_second()) {
+    DVLOG(0) << "Sampling rate change is not supported by" << GetDisplayName()
+             << " (detected change " << config_.samples_per_second() << "->"
+             << new_sampling_rate << ")";
+    SetState(STATE_ERROR);
+  }
 }
 
 #undef RETURN_STRING