Separate MediaCodecLoop from MediaCodecAudioDecoder

media/base/android/media_codec_loop.cc

Index: media/base/android/media_codec_loop.cc
diff --git a/media/base/android/media_codec_loop.cc b/media/base/android/media_codec_loop.cc
new file mode 100644
index 0000000000000000000000000000000000000000..bded0986190f8766401272bd624398786af3028f
--- /dev/null
+++ b/media/base/android/media_codec_loop.cc
@@ -0,0 +1,365 @@
+// Copyright 2016 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/base/android/media_codec_loop.h"
+
+#include "base/android/build_info.h"
+#include "base/bind.h"
+#include "base/callback_helpers.h"
+#include "base/logging.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "media/base/android/sdk_media_codec_bridge.h"
+#include "media/base/audio_buffer.h"
+#include "media/base/audio_timestamp_helper.h"
+#include "media/base/bind_to_current_loop.h"
+#include "media/base/timestamp_constants.h"
+
+namespace media {
+
+constexpr base::TimeDelta kDecodePollDelay =
+    base::TimeDelta::FromMilliseconds(10);
+constexpr base::TimeDelta kNoWaitTimeout = base::TimeDelta::FromMicroseconds(0);
+constexpr base::TimeDelta kIdleTimerTimeout = base::TimeDelta::FromSeconds(1);
+
+MediaCodecLoop::InputData::InputData() {}
+
+MediaCodecLoop::InputData::InputData(const InputData& other)
+    : memory(other.memory),
+      length(other.length),
+      key_id(other.key_id),
+      iv(other.iv),
+      subsamples(other.subsamples),
+      presentation_time(other.presentation_time),
+      completion_cb(other.completion_cb),
+      is_eos(other.is_eos),
+      is_encrypted(other.is_encrypted) {}
+
+MediaCodecLoop::InputData::~InputData() {}
+
+MediaCodecLoop::MediaCodecLoop(Client* client,
+                               std::unique_ptr<MediaCodecBridge>&& media_codec)
+    : state_(STATE_READY),
+      client_(client),
+      media_codec_(std::move(media_codec)),
+      pending_input_buf_index_(kInvalidBufferIndex),
+      weak_factory_(this) {
+  if (media_codec_ == nullptr)

[DaleCurtis, 2016/06/14 19:29:03]

if (!media_codec_) -- but really should this be a DCHECK?

[liberato (no reviews please), 2016/06/14 21:56:44]

probably, but then i've nothing to test without a media codec bridge mock.

i'll keep it as-is, since that particular test actually found a bug.  i'll add a
TODO to sort it out once the tests are better.

+    SetState(STATE_ERROR);
+}
+
+MediaCodecLoop::~MediaCodecLoop() {}
+
+void MediaCodecLoop::OnKeyAdded() {
+  if (state_ == STATE_WAITING_FOR_KEY)
+    SetState(STATE_READY);
+
+  DoIOTask();
+}
+
+static bool codec_flush_requires_destruction() {

[DaleCurtis, 2016/06/14 19:29:03]

Should go above all other methods.

[liberato (no reviews please), 2016/06/14 21:56:44]

Done.

+  // Flush if we can, otherwise completely recreate and reconfigure the codec.

[watk, 2016/06/15 00:29:17]

First sentence of this comment should be reworded or deleted.

TIL about the naming style for "cheap" functions.

[liberato (no reviews please), 2016/06/15 20:27:29]

TIL?

it mentions to use unix_hacker_style for inline functions, so i added 'inline'. 
:)

[watk, 2016/06/15 20:52:11]

Today I learned :) I looked up the style guide and saw the section about
functions that are cheap to call should be named in this style.

+  // Prior to JellyBean-MR2, flush() had several bugs (b/8125974, b/8347958) so
+  // we have to completely destroy and recreate the codec there.
+  return base::android::BuildInfo::GetInstance()->sdk_int() < 18;
+}
+
+bool MediaCodecLoop::TryFlush() {

[watk, 2016/06/15 00:29:17]

I think this should become void Flush(), because currently it's complicated to
detect whether the flush failed because of a codec error (which should be
considered terminal) or because it's not supported by the device, in which case
you need to recreate it. The error case calls OnCodecLoopError before TryFlush
returns. So you'd have to save the state variable and check it after TryFlush to
detect that flush errored.

So the calling code would look like:

if (NeedsFlushWorkaround()) {
  DeleteAndRecreateLoop();
} else {
  loop->Flush();
}

And Flush() should DCHECK that it doesn't need the flush workaround.

[liberato (no reviews please), 2016/06/15 20:27:30]

i don't understand why the caller cares about the difference between a codec
error and not supported.  either way, it has to recreate the codec if it wants
to keep using it.  for example, MCAD::Reset doesn't currently treat a previous
codec error any differently -- it just skips the MCB::Flush() in that case and
makes a new codec.

if it does what to differentiate (avda might), then it can check the state
before calling TryFlush.

i suppose that calling OnCodecError() during TryFlush() might be confusing to
the caller.  MCAD already fails any pending input in the input queue at the top
of Reset, which would is a side-effect of OnCodecError too otherwise.  however,
i think that AVDA has cases where it doesn't want to do that, so maybe this will
have to change.

[watk, 2016/06/15 20:52:11]

Gotcha. I assumed we wanted to treat a codec error as a terminal error, and not
attempt to continue by recreating a codec. This makes sense if not. But still
means we have to be careful when calling TryFlush to not care too much about the
OnCodecLoopError that we might get.

+  // We do not clear the input queue here.  It depends on the caller.
+  // For decoder reset, then it is appropriate.  Otherwise, the requests might
+  // simply be sent to us later, such as on a format change.
+
+  if (state_ == STATE_ERROR || state_ == STATE_DRAINED)

[watk, 2016/06/15 00:29:17]

It's not clear to me why we can't flush in STATE_DRAINED? Or is it that we don't
need to yet?

[liberato (no reviews please), 2016/06/15 20:27:30]

that's a very good question.  we can, according to MC docs.

+    return false;
+
+  if (codec_flush_requires_destruction())
+    return false;
+
+  // Actually try to flush!

[watk, 2016/06/15 00:29:17]

nit: Comment seems attached to an unrelated line. Probably if you delete the
newline after io_time_.Stop() it won't seem weird.

[liberato (no reviews please), 2016/06/15 20:27:29]

Done.

+  io_timer_.Stop();
+
+  if (media_codec_->Flush() != MEDIA_CODEC_OK) {
+    // Transition to the error state if the flush failed.

[watk, 2016/06/15 00:29:17]

Code is clear. I'd delete this comment.

[liberato (no reviews please), 2016/06/15 20:27:30]

Done.

+    SetState(STATE_ERROR);
+    return false;
+  }
+
+  SetState(STATE_READY);
+  return true;
+}
+
+void MediaCodecLoop::DoIOTask() {
+  if (state_ == STATE_ERROR)
+    return;
+
+  bool did_work = false, did_input = false, did_output = false;
+  do {
+    did_input = ProcessOneInputBuffer();
+    did_output = ProcessOneOutputBuffer();
+    if (did_input || did_output)
+      did_work = true;
+  } while (did_input || did_output);
+
+  // TODO(liberato): add "start_timer" for AVDA.
+  ManageTimer(did_work);
+}
+
+bool MediaCodecLoop::ProcessOneInputBuffer() {
+  // We can only queue a buffer if there is input from the client, or if we
+  // tried previously but had to wait for a key.  In the latter case, MediaCodec
+  // already has the data.
+  if (pending_input_buf_index_ == kInvalidBufferIndex &&
+      !client_->IsAnyInputPending()) {
+    return false;
+  }
+
+  if (state_ == STATE_WAITING_FOR_KEY || state_ == STATE_DRAINING ||
+      state_ == STATE_DRAINED || state_ == STATE_ERROR) {
+    return false;
+  }

[watk, 2016/06/15 00:29:17]

nit: this could be != READY, and checking if the state is valid feels like it
should come first, even if it probably doesn't really matter.

Slightly weird for the client to possibly get a call to IsAnyInputPending after
OnError().

[liberato (no reviews please), 2016/06/15 20:27:30]

Done, good point.

+
+  // DequeueInputBuffer() may set STATE_ERROR.
+  InputBuffer input_buffer = DequeueInputBuffer();
+
+  if (input_buffer.index == kInvalidBufferIndex)
+    return false;
+
+  // EnqueueInputBuffer() may set STATE_DRAINING, STATE_WAITING_FOR_KEY or
+  // STATE_ERROR.

[watk, 2016/06/15 00:29:17]

Up to you, but feels fragile to list these here. Could just say it may change
the state.

[liberato (no reviews please), 2016/06/15 20:27:30]

Done.

+  EnqueueInputBuffer(input_buffer);
+  return state_ == STATE_READY;

[watk, 2016/06/15 00:29:17]

If state is DRAINING we still did work right?

[liberato (no reviews please), 2016/06/15 20:27:29]

good point.  it could just return true at this point.

i changed it to !=ERROR, just since it can't transition out of error so there's
no point in resetting the timer.

+}
+
+MediaCodecLoop::InputBuffer MediaCodecLoop::DequeueInputBuffer() {
+  DVLOG(2) << __FUNCTION__;
+
+  // 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().

[watk, 2016/06/15 00:29:17]

We don't have a QueueSecureInputBuffer here (but we call one). 

[liberato (no reviews please), 2016/06/15 20:27:29]

Done.

+  if (pending_input_buf_index_ != kInvalidBufferIndex) {
+    InputBuffer result(pending_input_buf_index_, true);
+    pending_input_buf_index_ = kInvalidBufferIndex;
+    return result;
+  }
+
+  int input_buf_index = kInvalidBufferIndex;
+
+  media::MediaCodecStatus status =
+      media_codec_->DequeueInputBuffer(kNoWaitTimeout, &input_buf_index);
+  switch (status) {
+    case media::MEDIA_CODEC_DEQUEUE_INPUT_AGAIN_LATER:
+      break;
+
+    case media::MEDIA_CODEC_ERROR:
+      DLOG(ERROR) << __FUNCTION__
+                  << ": MEDIA_CODEC_ERROR from DequeInputBuffer";
+      SetState(STATE_ERROR);
+      break;
+
+    case media::MEDIA_CODEC_OK:
+      break;
+
+    default:
+      NOTREACHED() << "Unknown DequeueInputBuffer status " << status;
+      SetState(STATE_ERROR);
+      break;
+  }
+
+  return InputBuffer(input_buf_index, false);
+}
+
+void MediaCodecLoop::EnqueueInputBuffer(const InputBuffer& input_buffer) {
+  DCHECK_NE(input_buffer.index, kInvalidBufferIndex);
+
+  InputData input_data;

[DaleCurtis, 2016/06/14 19:29:03]

Could be written with ternary:

InputData input_data = is_pending ? pending_input_buf_data_ : provide; if you
prefer.

[liberato (no reviews please), 2016/06/14 21:56:44]

i'll keep it as is, so that the comment is clearer.

+  if (input_buffer.is_pending) {
+    // A pending buffer is already filled with data, no need to copy it again.
+    input_data = pending_input_buf_data_;
+  } else {
+    input_data = client_->ProvideInputData();
+  }
+
+  if (input_data.is_eos) {
+    media_codec_->QueueEOS(input_buffer.index);
+    SetState(STATE_DRAINING);
+
+    // For EOS, the completion callback is called when the EOS arrives at the
+    // output queue.
+    pending_eos_completion_cb_ = input_data.completion_cb;
+    return;
+  }
+
+  media::MediaCodecStatus status = MEDIA_CODEC_OK;

[DaleCurtis, 2016/06/14 19:29:03]

Ditto on ternary usage, again up to you.

[liberato (no reviews please), 2016/06/14 21:56:44]

Acknowledged.

+
+  if (input_data.is_encrypted) {
+    // Note that input_data might not have a valid memory ptr if this is a
+    // re-send of a buffer that was sent before decryption keys arrived.
+
+    status = media_codec_->QueueSecureInputBuffer(
+        input_buffer.index, input_data.memory, input_data.length,
+        input_data.key_id, input_data.iv, input_data.subsamples,
+        input_data.presentation_time);
+
+  } else {
+    status = media_codec_->QueueInputBuffer(
+        input_buffer.index, input_data.memory, input_data.length,
+        input_data.presentation_time);
+  }
+
+  switch (status) {
+    case MEDIA_CODEC_ERROR:
+      DLOG(ERROR) << __FUNCTION__
+                  << ": MEDIA_CODEC_ERROR from QueueInputBuffer";
+      input_data.completion_cb.Run(DecodeStatus::DECODE_ERROR);
+      // Transition to the error state after running the completion cb, to keep
+      // it in order if the client chooses to flush its queue.
+      SetState(STATE_ERROR);
+      break;
+
+    case MEDIA_CODEC_NO_KEY:
+      // Do not call the completion cb here.  It will be called when we retry
+      // after getting the key.
+      pending_input_buf_index_ = input_buffer.index;
+      pending_input_buf_data_ = input_data;
+      // MediaCodec has a copy of the data already.  When we call again, be sure
+      // to send in nullptr for the source.  Note that the client doesn't
+      // guarantee that the pointer will remain valid after we return anyway.
+      pending_input_buf_data_.memory = nullptr;
+      SetState(STATE_WAITING_FOR_KEY);
+      break;
+
+    case MEDIA_CODEC_OK:
+      input_data.completion_cb.Run(DecodeStatus::OK);
+      break;
+
+    default:
+      NOTREACHED() << "Unknown Queue(Secure)InputBuffer status " << status;
+      input_data.completion_cb.Run(DecodeStatus::DECODE_ERROR);
+      SetState(STATE_ERROR);
+      break;
+  }
+}
+
+bool MediaCodecLoop::ProcessOneOutputBuffer() {
+  MediaCodecStatus status;

[DaleCurtis, 2016/06/14 19:29:03]

Avoid c-style variable declarations where possible.

[liberato (no reviews please), 2016/06/14 21:56:44]

done, i think.

[watk, 2016/06/15 00:29:17]

I believe the point was that these declarations are earlier than they need to
be. And status could be declare and initialized at the same time.

[liberato (no reviews please), 2016/06/15 20:27:30]

that's what i ended up doing.

+  OutputBuffer out;
+  bool did_work = false;
+
+  // TODO(liberato): When merging AVDA, we will also have to ask the client if
+  // it can accept another output buffer.
+
+  status = media_codec_->DequeueOutputBuffer(kNoWaitTimeout, &out.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.
+      did_work = true;
+      break;
+
+    case MEDIA_CODEC_OUTPUT_FORMAT_CHANGED:
+      if (!client_->OnOutputFormatChanged())
+        SetState(STATE_ERROR);
+      did_work = (state_ != STATE_ERROR);

[DaleCurtis, 2016/06/14 19:29:03]

Remove unnesscary parens.

[liberato (no reviews please), 2016/06/14 21:56:44]

Done.

+      break;
+
+    case MEDIA_CODEC_OK:
+      // We got the decoded frame.

[watk, 2016/06/15 00:29:17]

minor nit: we didn't necessarily get a frame. Could be EOS buffer.

[liberato (no reviews please), 2016/06/15 20:27:30]

Done.

+      if (out.is_eos) {
+        // Set state STATE_DRAINED after we have received EOS frame at the
+        // output. media_decoder_job.cc says: once output EOS has occurred, we
+        // should not be asked to decode again.
+        DCHECK_EQ(state_, STATE_DRAINING);
+        SetState(STATE_DRAINED);
+
+        DCHECK_NE(out.index, kInvalidBufferIndex);
+        DCHECK(media_codec_);
+
+        media_codec_->ReleaseOutputBuffer(out.index, false);
+
+        // Run the EOS completion callback now, since we deferred it until
+        // the EOS was completely processed.
+        pending_eos_completion_cb_.Run(DecodeStatus::OK);
+        pending_eos_completion_cb_ = DecodeCB();
+
+        client_->OnDecodedEos(out);
+      } else {
+        if (!client_->OnDecodedFrame(out))
+          SetState(STATE_ERROR);
+      }
+
+      did_work = true;
+      break;
+
+    case MEDIA_CODEC_DEQUEUE_OUTPUT_AGAIN_LATER:
+      // Nothing to do.
+      break;
+
+    case MEDIA_CODEC_ERROR:
+      DLOG(ERROR) << __FUNCTION__
+                  << ": MEDIA_CODEC_ERROR from DequeueOutputBuffer";
+      // Next Decode() will report the error to the pipeline.

[watk, 2016/06/15 00:29:17]

pipeline comment is not applicable.

[liberato (no reviews please), 2016/06/15 20:27:30]

Done.

+      SetState(STATE_ERROR);
+      break;
+
+    default:
+      NOTREACHED() << "Unknown DequeueOutputBuffer status " << status;
+      // Next Decode() will report the error to the pipeline.

[watk, 2016/06/15 00:29:17]

same pipeline comment

[liberato (no reviews please), 2016/06/15 20:27:30]

Done.

+      SetState(STATE_ERROR);
+      break;
+  }
+
+  return did_work;
+}
+
+void MediaCodecLoop::ManageTimer(bool did_work) {
+  bool should_be_running = true;
+
+  base::TimeTicks now = base::TimeTicks::Now();
+  if (did_work || idle_time_begin_ == base::TimeTicks()) {
+    idle_time_begin_ = now;
+  } else {
+    // Make sure that we have done work recently enough, else stop the timer.
+    if (now - idle_time_begin_ > kIdleTimerTimeout)
+      should_be_running = false;
+  }
+
+  if (should_be_running && !io_timer_.IsRunning()) {
+    io_timer_.Start(FROM_HERE, kDecodePollDelay, this,
+                    &MediaCodecLoop::DoIOTask);
+  } else if (!should_be_running && io_timer_.IsRunning()) {
+    io_timer_.Stop();
+  }
+}
+
+void MediaCodecLoop::SetState(State new_state) {
+  if (state_ != new_state && new_state == STATE_ERROR)
+    client_->OnCodecLoopError();

[watk, 2016/06/15 00:29:17]

Should this set the state first? To avoid any weirdness with reentrant calls to
MCL by the client? Probably not a problem with the public interface we currently
have.

[liberato (no reviews please), 2016/06/15 20:27:30]

Done.

+  state_ = new_state;
+}
+
+MediaCodecBridge* MediaCodecLoop::GetCodec() const {
+  return media_codec_.get();
+}
+
+// static
+const char* MediaCodecLoop::AsString(State state) {
+#define RETURN_STRING(x) \
+  case x:                \
+    return #x;
+
+  switch (state) {
+    RETURN_STRING(STATE_READY);
+    RETURN_STRING(STATE_WAITING_FOR_KEY);
+    RETURN_STRING(STATE_DRAINING);
+    RETURN_STRING(STATE_DRAINED);
+    RETURN_STRING(STATE_ERROR);
+  }
+#undef RETURN_STRING
+
+  NOTREACHED() << "Unknown state " << state;
+  return nullptr;
+}
+
+}  // namespace media