Separate MediaCodecLoop from MediaCodecAudioDecoder

media/base/android/media_codec_loop.cc

+// 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 {
+
+namespace {
+
+// TODO(liberato): the client should choose these.

[DaleCurtis, 2016/06/08 22:55:01]

You should be able to replace these now with constexpr
base::TimeDelta::FromXXX() kConstant;

[liberato (no reviews please), 2016/06/10 21:34:09]

nifty trick.

+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);
+}
+
+}  // namespace (anonymous)
+
+MediaCodecLoop::InputBufferData::InputBufferData()
+    : memory(nullptr), length(0), is_eos(false), is_encrypted(false) {}
+// TODO(liberato): does anybody call this?  if not, maybe they should.

[DaleCurtis, 2016/06/08 22:55:01]

Delete until then?

[liberato (no reviews please), 2016/06/10 21:34:09]

Done.

+MediaCodecLoop::InputBufferData::InputBufferData(const uint8_t* m,
+                                                 size_t l,
+                                                 const DecodeCB& cb)
+    : memory(m),
+      length(l),
+      completion_cb(cb),
+      is_eos(false),
+      is_encrypted(false) {}
+
+MediaCodecLoop::InputBufferData::InputBufferData(const InputBufferData& 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::InputBufferData::~InputBufferData() {}
+
+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) {
+  DVLOG(1) << __FUNCTION__;
+}
+
+MediaCodecLoop::~MediaCodecLoop() {
+  DVLOG(1) << __FUNCTION__;
+
+  media_codec_.reset();

[watk, 2016/06/09 02:04:33]

Any reason to do this explicitly?

[liberato (no reviews please), 2016/06/10 21:34:09]

nope, removed.

+}
+
+void MediaCodecLoop::OnKeyAdded() {
+  DVLOG(1) << __FUNCTION__;
+
+  if (state_ == STATE_WAITING_FOR_KEY)
+    SetState(STATE_READY);
+
+  DoIOTask();
+}
+
+bool MediaCodecLoop::TryFlush() {
+  DVLOG(1) << __FUNCTION__;
+
+  // 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.
+
+  // Flush if we can, otherwise completely recreate and reconfigure the codec.
+  // Prior to JellyBean-MR2, flush() had several bugs (b/8125974, b/8347958) so
+  // we have to completely destroy and recreate the codec there.
+  bool success = false;
+  if (state_ != STATE_ERROR && state_ != STATE_DRAINED &&
+      base::android::BuildInfo::GetInstance()->sdk_int() >= 18) {
+    io_timer_.Stop();

[DaleCurtis, 2016/06/08 22:55:01]

Step N+1 is to switch to a common timer like AVDA?

[liberato (no reviews please), 2016/06/10 21:34:09]

yup.  the first step aims at no function changes.

+
+    // media_codec_->Reset() calls MediaCodec.flush().
+    success = (media_codec_->Reset() == MEDIA_CODEC_OK);

[DaleCurtis, 2016/06/08 22:55:01]

Avoid unnecessary parens.

[liberato (no reviews please), 2016/06/10 21:34:09]

Done.

+
+    // Transition to the error state if the flush failed.
+    SetState(success ? STATE_READY : STATE_ERROR);
+  }
+
+  return success;
+}
+
+void MediaCodecLoop::DoIOTask() {
+  if (state_ == STATE_ERROR)
+    return;
+
+  const bool did_input = QueueInput();

[DaleCurtis, 2016/06/08 22:55:01]

Should these be done in a while loop like AVDA? Note, the functions below then
need to pick up watk@'s fixes for when the MediaCodec is dropped mid-loop.

[Tima Vaisburd, 2016/06/08 23:24:13]

Each one is a while loop.

[DaleCurtis, 2016/06/09 00:30:49]

Yes, but I don't think each one should be. We know there are cases where inputs
may be queued after an output is dequeued, so the current logic is insufficient
for the most optimal pipeling.

[Tima Vaisburd, 2016/06/09 00:54:44]

I am not aware of these cases (or forgot), could you, please, remind me?
Currently we repeat the round on next timer (i.e. in 10 ms), what are the cases
where it would be better to do differently?

My logic has been as follows. On input, enqueue all available buffers from the
queue if the codec accepts them, there is no point to postpone. Independently of
success of failure in the input proceed to output (that's important, otherwise
decoder might block). On output, pull everything that's available at the current
time (timeout == 0).  

[DaleCurtis, 2016/06/09 00:56:48]

Pulling output may allow new input to be queued; as seen on the MotoX.

[liberato (no reviews please), 2016/06/10 21:34:09]

hrm, i was going to keep this identical until the next CL.

however, yeah, it seems pretty safe to do now and does clean up the naming a
bit.

+  const bool did_output = DequeueOutput();
+
+  ManageTimer(did_input || did_output);
+}
+
+bool MediaCodecLoop::QueueInput() {

[DaleCurtis, 2016/06/08 22:55:01]

I think the naming for these methods should be reconsidered. We have Queue,
QueueOne, Enqueue, and Deque all referring to input buffers -- none of which has
a clear label. How about merging QueueInput into DoIOTask() like we do for AVDA?
Then rename the QueueOneInputBuffer to ProcessOneInputBuffer.

[liberato (no reviews please), 2016/06/10 21:34:09]

Done.

+  DVLOG(2) << __FUNCTION__;
+
+  bool did_work = false;
+  while (QueueOneInputBuffer())
+    did_work = true;
+
+  return did_work;
+}
+
+bool MediaCodecLoop::QueueOneInputBuffer() {
+  DVLOG(2) << __FUNCTION__;
+
+  // 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 &&

[DaleCurtis, 2016/06/08 22:55:00]

Needs {}

[liberato (no reviews please), 2016/06/10 21:34:09]

Done.

+      !client_->IsAnyInputPending())
+    return false;
+
+  if (state_ == STATE_WAITING_FOR_KEY || state_ == STATE_DRAINING ||

[DaleCurtis, 2016/06/08 22:55:01]

Ditto.

[liberato (no reviews please), 2016/06/10 21:34:09]

Done.

+      state_ == STATE_DRAINED)
+    return false;
+
+  // DequeueInputBuffer() may set STATE_ERROR.
+  InputBufferInfo input_info = DequeueInputBuffer();
+
+  if (input_info.buf_index == kInvalidBufferIndex)
+    return false;
+
+  // EnqueueInputBuffer() may set STATE_DRAINING, STATE_WAITING_FOR_KEY or
+  // STATE_ERROR.
+  EnqueueInputBuffer(input_info);
+  return state_ == STATE_READY;
+}
+
+MediaCodecLoop::InputBufferInfo 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().
+  if (pending_input_buf_index_ != kInvalidBufferIndex) {
+    InputBufferInfo result(pending_input_buf_index_, true);
+    pending_input_buf_index_ = kInvalidBufferIndex;
+    return result;
+  }
+
+  int input_buf_index = kInvalidBufferIndex;
+
+  media::MediaCodecStatus status =
+      media_codec_->DequeueInputBuffer(NoWaitTimeout(), &input_buf_index);
+  switch (status) {
+    case media::MEDIA_CODEC_DEQUEUE_INPUT_AGAIN_LATER:
+      DVLOG(2) << __FUNCTION__ << ": MEDIA_CODEC_DEQUEUE_INPUT_AGAIN_LATER";
+      break;
+
+    case media::MEDIA_CODEC_ERROR:
+      DVLOG(1) << __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 InputBufferInfo(input_buf_index, false);
+}
+
+void MediaCodecLoop::EnqueueInputBuffer(const InputBufferInfo& input_info) {
+  DVLOG(2) << __FUNCTION__ << ": index:" << input_info.buf_index;
+
+  DCHECK_NE(input_info.buf_index, kInvalidBufferIndex);
+
+  InputBufferData input_data;
+
+  if (input_info.is_pending) {
+    // A pending buffer is already filled with data, no need to copy it again.
+    DVLOG(2) << __FUNCTION__ << ": QueueSecureInputBuffer (pending):"
+             << " index:" << input_info.buf_index
+             << " pts:" << pending_input_buf_data_.presentation_time
+             << " size:" << pending_input_buf_data_.length;
+
+    input_data = pending_input_buf_data_;
+  } else {
+    input_data = client_->ProvideInputData();
+  }
+
+  // Process this buffer.
+
+  if (input_data.is_eos) {
+    media_codec_->QueueEOS(input_info.buf_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;
+
+  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.
+
+    DVLOG(2) << __FUNCTION__ << ": QueueSecureInputBuffer:"
+             << " index:" << input_info.buf_index
+             << " pts:" << input_data.presentation_time
+             << " size:" << input_data.length;
+
+    status = media_codec_->QueueSecureInputBuffer(
+        input_info.buf_index, input_data.memory, input_data.length,
+        input_data.key_id, input_data.iv, input_data.subsamples,
+        input_data.presentation_time);
+
+  } else {
+    DVLOG(2) << __FUNCTION__ << ": QueueInputBuffer:"
+             << " index:" << input_info.buf_index
+             << " pts:" << input_data.presentation_time
+             << " size:" << input_data.length;
+
+    status = media_codec_->QueueInputBuffer(
+        input_info.buf_index, input_data.memory, input_data.length,
+        input_data.presentation_time);
+  }
+
+  switch (status) {
+    case MEDIA_CODEC_ERROR:
+      DVLOG(0) << __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:
+      DVLOG(1) << "QueueSecureInputBuffer failed: 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_info.buf_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::DequeueOutput() {
+  DVLOG(2) << __FUNCTION__;
+
+  MediaCodecStatus status;
+  OutputBufferInfo out;
+  bool did_work = false;

[DaleCurtis, 2016/06/08 22:55:01]

I'd move this out of here and into DoIOTask then follow similar naming
conventions to the input functions.

[liberato (no reviews please), 2016/06/10 21:34:08]

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";
+        did_work = true;
+        break;
+
+      case MEDIA_CODEC_OUTPUT_FORMAT_CHANGED:
+        DVLOG(2) << __FUNCTION__ << " MEDIA_CODEC_OUTPUT_FORMAT_CHANGED";
+        if (!client_->OnOutputFormatChanged())
+          SetState(STATE_ERROR);
+        did_work = (state_ != STATE_ERROR);
+        break;
+
+      case MEDIA_CODEC_OK:
+        // We got the decoded frame.
+        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.buf_index, kInvalidBufferIndex);
+          DCHECK(media_codec_);
+
+          media_codec_->ReleaseOutputBuffer(out.buf_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.
+        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() << "Unknown DequeueOutputBuffer status " << status;
+        // Next Decode() will report the error to the pipeline.
+        SetState(STATE_ERROR);
+        break;
+    }
+  } while (status != MEDIA_CODEC_DEQUEUE_OUTPUT_AGAIN_LATER &&
+           status != MEDIA_CODEC_ERROR && !out.is_eos);
+
+  return did_work;
+}
+
+void MediaCodecLoop::ManageTimer(bool did_work) {

[DaleCurtis, 2016/06/08 22:55:01]

I think this should actually be refactored out further such that we have a
MediaCodecLoopTimer singleton which manages this for all instances.

[liberato (no reviews please), 2016/06/10 21:34:09]

i think that should be part of the next CL, since it's importing AVDA code.

+  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_ > IdleTimerTimeout())
+      should_be_running = false;
+  }
+
+  if (should_be_running && !io_timer_.IsRunning()) {
+    io_timer_.Start(FROM_HERE, DecodePollDelay(), this,
+                    &MediaCodecLoop::DoIOTask);
+  } else if (!should_be_running && io_timer_.IsRunning()) {
+    io_timer_.Stop();
+  }
+}
+
+void MediaCodecLoop::SetState(State new_state) {
+  DVLOG(1) << __FUNCTION__ << ": " << AsString(state_) << "->"
+           << AsString(new_state);
+  if (state_ != new_state && state_ == STATE_ERROR)

[DaleCurtis, 2016/06/08 22:55:01]

This offers no room for deferring errors since it is unnecessary for audio, just
fyi.

[liberato (no reviews please), 2016/06/10 21:34:09]

i think the error deferring logic stays in the client.  AVDA::OnError can still
defer the post, i think.

actually, i'm going to rename OnError.  it's too generic.

+    client_->OnError();
+  state_ = new_state;
+}
+
+MediaCodecBridge* MediaCodecLoop::GetCodec() const {
+  return media_codec_.get();
+}
+
+#undef RETURN_STRING

[DaleCurtis, 2016/06/08 22:55:01]

No macros, lets avoid this from the beginning :)

[Tima Vaisburd, 2016/06/08 23:14:20]

Dale, what would be your suggestion to implement AsString() then?

[DaleCurtis, 2016/06/09 00:30:49]

Hmm, I thought there'd be a nice C++ solution to this these days, but there's
only duplicate your strings as far as I can tell. So how about making this a
little safer:

#if defined(RETURN_STRING)
#error Unexpected macro collision.
#else
AsString() { 
#define RETURN_STRING(x) case x: return #x;
#undef RETURN_STRING
}
#endif

[liberato (no reviews please), 2016/06/10 21:34:09]

done, though i skipped #else since #error.

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