Separate MediaCodecLoop from MediaCodecAudioDecoder

media/base/android/media_codec_loop.h

Index: media/base/android/media_codec_loop.h
diff --git a/media/base/android/media_codec_loop.h b/media/base/android/media_codec_loop.h
new file mode 100644
index 0000000000000000000000000000000000000000..719da5d3f438547adab847e6dd76278ae0a6b068
--- /dev/null
+++ b/media/base/android/media_codec_loop.h
@@ -0,0 +1,308 @@
+// 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.
+
+#ifndef MEDIA_FILTERS_ANDROID_MEDIA_CODEC_LOOP_H_
+#define MEDIA_FILTERS_ANDROID_MEDIA_CODEC_LOOP_H_
+
+#include <deque>
+#include <memory>
+#include <utility>
+#include <vector>
+
+#include "base/macros.h"
+#include "base/memory/weak_ptr.h"
+#include "base/time/time.h"
+#include "base/timer/timer.h"
+#include "media/base/android/media_codec_bridge.h"
+#include "media/base/android/media_drm_bridge_cdm_context.h"
+#include "media/base/audio_decoder.h"
+#include "media/base/audio_decoder_config.h"
+#include "media/base/media_export.h"
+
+// MediaCodecLoop is based on Android's MediaCodec API.
+// The MediaCodec API is required to play encrypted (as in EME) content on
+// Android. It is also a way to employ hardware-accelerated decoding.
+// One MediaCodecLoop instance owns a single MediaCodec(Bridge) instance, and
+// drives it to perform decoding in conjunction with a MediaCodecLoop::Client.
+// The Client provides the input data and consumes the output data.  A typical
+// example is AndroidVideoDecodeAccelerator.
+
+// Implementation notes.
+//
+// The MediaCodec
+// (http://developer.android.com/reference/android/media/MediaCodec.html) works
+// by exchanging buffers between the client and the codec itself. On the input
+// side an "empty" buffer has to be dequeued from the codec, filled with data
+// and queued back. On the output side a "full" buffer with data should be
+// dequeued, the data is to be used somehow (copied out, or rendered to a pre-
+// defined texture for video) and the buffer has to be returned back (released).
+// Neither input nor output dequeue operations are guaranteed to succeed: the
+// codec might not have available input buffers yet, or not every encoded buffer
+// has arrived to complete an output frame. In such case the client should try
+// to dequeue a buffer again at a later time.
+//
+// There is also a special situation related to an encrypted stream, where the
+// enqueuing of a filled input buffer might fail due to lack of the relevant key
+// in the CDM module.  While MediaCodecLoop does not handle the CDM directly,
+// it does understand the codec state.
+//
+// Much of that logic is common to all users of MediaCodec.  The MediaCodecLoop
+// provides the main driver loop to talk to MediaCodec.  Via the Client
+// interface, MediaCodecLoop can request input data, send output buffers, etc.
+// MediaCodecLoop does not construct a MediaCodec, but instead takes ownership
+// of one that it provided during construction.
+//
+// Although one can specify a delay in the MediaCodec's dequeue operations,
+// this implementation follows the simple logic which is similar to
+// AndroidVideoDecodeAccelerator: no delay for either input or output buffers,
+// the processing is initated by the timer with short period (10 ms). Using no
+// delay for enqueue operations has an extra benefit of not blocking the current
+// thread.
+//
+// This implementation detects the MediaCodec idle run (no input or output
+// buffer processing) and after being idle for a predefined time the timer
+// stops.  The client is responsible for signalling us to wake up via a call
+// to DoIOTask.  It is okay for the client to call this even when the timer is
+// already running.
+//
+// The current implementation is single threaded. Every method is supposed to
+// run on the same thread.
+//
+// State diagram.
+//
+//  [Ready]
+//     |
+// (MediaCodec error)
+//     |
+//   [Error]
+//
+//  [Ready]
+//     |
+// (EOS enqueued)
+//     |
+//  [Draining]
+//     |
+// (EOS dequeued on output)
+//     |
+//   [Drained]
+//
+//      [Ready]
+//         |
+// (MEDIA_CODEC_NO_KEY)
+//         |
+//   [WaitingForKey]
+//         |
+//    (OnKeyAdded)
+//         |
+//      [Ready]
+//
+//     -[Any state]-
+//    |             |
+// (Flush ok) (Flush fails)
+//    |             |
+// [Ready]       [Error]
+
+namespace base {
+class SingleThreadTaskRunner;
+}
+
+namespace media {
+
+class MEDIA_EXPORT MediaCodecLoop {
+ public:
+  // TODO(liberato): this exists in video_decoder.h and audio_decoder.h too.
+  using InitCB = base::Callback<void(bool success)>;
+  using DecodeCB = base::Callback<void(DecodeStatus status)>;
+
+  // Information about dequeued input buffer.
+  struct InputBufferInfo {
+    int buf_index;    // The codec input buffers are referred to by this index.
+    bool is_pending;  // True if we tried to enqueue this buffer before.
+    InputBufferInfo(int i, bool p) : buf_index(i), is_pending(p) {}
+  };
+
+  // Data that the client wants to put into an input buffer.
+  struct InputBufferData {

[Tima Vaisburd, 2016/06/01 00:52:40]

Would it be possible to have a queue of same elements for both audio and video?
Or we already agreed that it's more hassle than good?

[liberato (no reviews please), 2016/06/01 21:47:54]

now that MediaCodecLoop handles only a single MediaCodecBridge instance, it's
not clear that we want this in MCL.  if the client creates a new media codec
bridge / MCL, then the unused part of the queue would have to be transferred to
a new instance.

better, i think, if we want to consolidate this, is to use a helper class that's
separate from MCL.

[Tima Vaisburd, 2016/06/02 00:35:21]

Sorry, I should have been clearer. I hinted at avoiding the InputBufferData
altogether, but instead of subtle hints should have talked straight.

If I get it correctly,
right now you copy the DecoderBuffer from the |input_queue_| into
InputBufferData variable and from this variable to QueueInputBuffer() field by
field. I was wondering maybe ProvideInputData() can return a pointer into the
input_queue_ directly.

I'm not sure though it would be easy to provide a common format for MCAD and
AVDA (or common class for DecoderBuffer and BitstreamBuffer?), so please
consider these remarks as my thinking out loud.

+    InputBufferData();
+    InputBufferData(const InputBufferData&);
+    InputBufferData(const uint8_t* m, size_t l, const DecodeCB& cb);
+    ~InputBufferData();
+
+    const uint8_t* memory;
+    size_t length;
+
+    std::string key_id;
+    std::string iv;
+    std::vector<SubsampleEntry> subsamples;
+
+    base::TimeDelta presentation_time;
+
+    // Called when this is queued.
+    DecodeCB completion_cb;
+
+    bool is_eos : 1;
+    bool is_encrypted : 1;
+  };
+
+  // Information about the MediaCodec's output buffer.
+  struct OutputBufferInfo {
+    int buf_index;  // The codec output buffers are referred to by this index.
+    size_t offset;  // Position in the buffer where data starts.
+    size_t size;    // The size of the buffer (includes offset).
+    base::TimeDelta pts;  // Presentation timestamp.
+    bool is_eos;          // true if this buffer is the end of stream.
+    bool is_key_frame;    // true if this buffer is a key frame.
+  };
+
+  // Possible states.
+  enum State {
+    STATE_READY,
+    STATE_WAITING_FOR_KEY,
+    STATE_DRAINING,
+    STATE_DRAINED,
+    STATE_ERROR,
+  };
+
+  class Client {
+   public:
+    // Return true if and only if there is input that is pending to be
+    // queued with MediaCodec.  ProvideInputData() will not be called more than
+    // once in response to this returning true once.  It is not guaranteed that
+    // ProvideInputData will be called at all.
+    virtual bool IsAnyInputPending() = 0;
+
+    // Fills and returns an input buffer for MediaCodecLoop to queue.  It is
+    // assumed that IsAnyInputPending() has already verified that there is data
+    // to be queued.
+    virtual InputBufferData ProvideInputData() = 0;
+

[Tima Vaisburd, 2016/06/01 00:52:40]

I think we want another method that controls the queue advancement (pop_front),
something like AdvanceInput(). At least right now the data retrieval for
EnqueueInput() is separated from queue_.pop_front() and pending buffer remains
in the queue. I'll try to comment in the cc.

[liberato (no reviews please), 2016/06/01 21:47:54]

i don't think that i understand what you mean.  when would MCL call
AdvanceInput?

right now, the completion callback goes with the input buffer.  MCAD does remove
it from the queue when it hands the buffer off to MCL in ProvideInputData.  MCL
owns the buffer, and MCAD should never return that buffer in response to
ProvideInputData again.

the only exception is EOS, where it neither pops the buffer nor sends the
callback.  it depends that the EOS transitions to DRAINING, so MCL won't ask for
input again.  that seems fragile to me, but i couldn't figure out how to remove
it without making it worse.  :)

[Tima Vaisburd, 2016/06/02 00:35:21]

The idea was to decouple MCAD::input_queue_.pop_front() from ProvideInputData().
ProvideInputData() would return the same front buffer over and over until
somebody calls pop_front(). In a sense, MCL does not own the buffer, the
|input_queue_| does. MCL could call this pop_front() e.g. by
client_->AdvanceQueue().

It is a different approach, not necessary better than what you have right now.

[liberato (no reviews please), 2016/06/03 16:23:28]

i've given some thought to this.  based on our discussion, i'm going to try to
avoid the whole issue :)

the MCL already knows to switch to DRAINING when an EOS arrives at the input. it
might as well know to defer the EOS completion callback until the EOS is
dequeued.  this avoids making MCAD handle the input queue differently for data
vs EOS buffers.

MCAD should always advance the input queue as part of ProvideInputData().

i'm going to leave OnDecodedEos() for now, though in MCAD it does nothing.  If
this new approach ends up being the one we go with, then i'll probably remove
it.

+    // Called when an EOS buffer is dequeued from the output.
+    virtual void OnDecodedEos(const OutputBufferInfo& out) = 0;
+
+    // Processes the output buffer after it comes from MediaCodec.  The client
+    // has the responsibility to release the codec buffer, though it doesn't
+    // need to do so before this call returns.  If it does not do so before
+    // returning, then the client must call DoIOTask when it does release it.
+    virtual void OnDecodedFrame(const OutputBufferInfo& out) = 0;
+
+    // Processes the output format change on |media_codec|.  Returns true on
+    // success, or false to transition to the error state.
+    virtual bool OnOutputFormatChanged() = 0;
+
+    // Calls DecodeCB with |decode_status| for every frame in |input_queue| and
+    // then clears it.
+    // TODO(liberato): update this description to be more generic.  Perhaps this
+    // is really OnStateChanged(), and let the client decide what to do.  In
+    // that case, OnStateChanged is sufficient.  Currently, transitions to the
+    // error state are always accompanied by a call to ClearInputQueue, except
+    // when an error occurs in DequeueOutput().
+    // TODO(timav): Is it okay for MCAD to clear the input queue in that case?
+    virtual void ClearInputQueue(DecodeStatus decode_status) = 0;
+
+    // Notify the client about our state changes.
+    virtual void OnStateChanged(State new_state) = 0;

[Tima Vaisburd, 2016/06/01 00:52:40]

Why would the client want to know about MediaCodecLoop states? If it is just to
pass an error, maybe have OnError() instead?

Then the client itself can clear the input queue, and hopefully
ClearInputQueue() can be removed from the Client interface.

[liberato (no reviews please), 2016/06/01 21:47:54]

done.

+  };
+
+  // Handy enum for "no buffer".
+  enum { kInvalidBufferIndex = -1 };
+
+  // We will take ownership of |media_codec|.  We will not destroy it until
+  // we are destructed.
+  explicit MediaCodecLoop(Client* client,

[Tima Vaisburd, 2016/06/01 00:52:40]

nit: no "explicit". As far as I understand an "explicit" would only makes sense
if constructor has exactly one parameter (and thus defines an implicit
conversion) and here we have two parameters.

[liberato (no reviews please), 2016/06/01 21:47:54]

Done.

+                          std::unique_ptr<MediaCodecBridge>&& media_codec);
+  ~MediaCodecLoop();
+
+  // Does the MediaCodec processing cycle: enqueues an input buffer, then
+  // dequeues output buffers.  This should be called by the client when more
+  // work becomes available, such as when new input data arrives.  If codec
+  // output buffers are freed after OnDecodedFrame returns, then this should
+  // also be called.
+  void DoIOTask();
+
+  // Try to flush this media codec.  Returns true on success, false on failure.
+  // Failures can result in a state change to the Error state.  If this returns
+  // false but the state is still READY, then the codec may continue to be used.
+  bool TryFlush();
+
+  // Callback called by the client when a new key is available.  This allows
+  // decoding to resume if it was stopped in the WAITING_FOR_KEY state.
+  void OnKeyAdded();
+
+  // Return our current state.
+  State GetState() const;
+
+  // Return our codec.  It is guaranteed that this will be non-null, if we were
+  // given a non-null codec during construction.
+  MediaCodecBridge* GetCodec() const;

[Tima Vaisburd, 2016/06/01 00:52:40]

I would try to avoid exposing MediaCodecBridge to the outside world. I think it
is only used to get an access to the buffer's data, see my comment there.

[liberato (no reviews please), 2016/06/01 21:47:54]

it's also used for OnFormatChanged to query the new format.

there are other cases where we might want it in avda (setOutputSurface, for
example).  i'm not sure that wrapping the calls in MCL really adds much
abstraction -- if you're using MCL, then you already have to understand MC
pretty deeply.

plus. since each MCL instance is 1:1 with a MC instance, it's well defined which
codec is intended.

[Tima Vaisburd, 2016/06/02 00:35:21]

Yes, I missed that, you are absolutely right.
Still this GetCodec() breaks encapsulation the way I understand it. I hoped MCL
would conceal MediaCodecBridge. The methods like OnFormatChanged() and
OnFrameDecoded() are actually good candidates for inheritance. The old pipeline
used a combination of inheritance and delegation to solve this kind of problem,
i.e.

                     MCL
                      ^
                      |

AudioDecoder ---->  AudioMCL

At this point I agree to leave it as is and would like to hear what other
reviewers think.

[liberato (no reviews please), 2016/06/03 16:23:28]

Acknowledged.

+
+ protected:
+  // Enqueues pending input buffers into MediaCodec as long as it can happen
+  // without delay in dequeuing and enqueueing input buffers.
+  // Returns true if any input was processed.
+  bool QueueInput();
+
+  // Enqueues one pending input buffer into MediaCodec if MediaCodec has room.
+  // Returns true if any input was processed.
+  bool QueueOneInputBuffer();
+
+  // Get data for |input_info| from the client and queue it.
+  void EnqueueInputBuffer(const InputBufferInfo& input_info);
+
+  // A helper method for QueueInput(). Dequeues an empty input buffer from the
+  // codec and returns the information about it. OutputBufferInfo.buf_index is
+  // the index of the dequeued buffer or -1 if the codec is busy or an error
+  // occured.  OutputBufferInfo.is_pending is set to true if we tried to enqueue
+  // this buffer before. In this case the buffer is already filled with data.
+  // In the case of an error sets STATE_ERROR.
+  InputBufferInfo DequeueInputBuffer();
+
+  // Dequeues all output buffers from MediaCodec that are immediately available.
+  // Returns true if any output buffer was received from MediaCodec.
+  bool DequeueOutput();
+
+  // Start the timer immediately if |start| is true or stop it based on elapsed
+  // idle time if |start| is false.
+  void ManageTimer(bool start);
+
+  // Helper method to change the state.
+  void SetState(State new_state);
+
+  // A helper function for logging.
+  static const char* AsString(State state);
+
+  // Used to post tasks. This class is single threaded and every method should
+  // run on this task runner.
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
+
+  State state_;
+
+  // The client that we notify about MediaCodec events.
+  Client* client_;
+
+  // The MediaCodec instance that we're using.
+  std::unique_ptr<MediaCodecBridge> media_codec_;
+
+  // Repeating timer that kicks MediaCodec operation.
+  base::RepeatingTimer io_timer_;
+
+  // Time at which we last did useful work on |io_timer_|.
+  base::TimeTicks idle_time_begin_;
+
+  // Index of the dequeued and filled buffer that we keep trying to enqueue.
+  // Such buffer appears in MEDIA_CODEC_NO_KEY processing. The -1 value means
+  // there is no such buffer.
+  int pending_input_buf_index_;
+
+  // When processing a pending input bfufer, this is the data that was returned
+  // to us by the client.  |memory| has been cleared, since the codec has it.
+  InputBufferData pending_input_buf_data_;
+
+  base::WeakPtrFactory<MediaCodecLoop> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(MediaCodecLoop);
+};
+
+}  // namespace media
+
+#endif  // MEDIA_FILTERS_ANDROID_MEDIA_CODEC_LOOP_H_