Add MediaCodecAudioDecoder implementation

media/filters/android/android_audio_decoder.h

Index: media/filters/android/android_audio_decoder.h
diff --git a/media/filters/android/android_audio_decoder.h b/media/filters/android/android_audio_decoder.h
new file mode 100644
index 0000000000000000000000000000000000000000..4e40054482bae6d0db633f1f8a8bfa5278f4a2ee
--- /dev/null
+++ b/media/filters/android/android_audio_decoder.h
@@ -0,0 +1,164 @@
+// 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_ANDROID_AUDIO_DECODER_H_
+#define MEDIA_FILTERS_ANDROID_ANDROID_AUDIO_DECODER_H_

[Tima Vaisburd, 2016/02/04 22:59:12]

Moved the file into media/filters/android. The ANDROID_ANDROID suggests that we
might rename this decoder...

[xhwang, 2016/02/05 21:37:37]

*_audio_decoder.h is more media-ish and *_android.h is more clank-ish.
Personally I feel this is fine, nobody really reads the guards here..

[Tima Vaisburd, 2016/02/06 03:54:12]

Just to clarify: "Android" in the name of the class became redundant since every
file in this directory is suppoed to be about Android. MediaCodecAudioDecoder
would be more precise, but this name is taken. If you have a better idea of a
name please let me know, otherwise keeping as is.

[xhwang, 2016/02/08 20:03:34]

MediaCodecAudioDecoder does sound a better name. The existing
MediaCodecAudioDecoder should really be AudioMediaCodecDecoder since it's a
MediaCodecDecoder.

+
+#include <deque>

[xhwang, 2016/02/05 21:37:37]

add an empty line here

[Tima Vaisburd, 2016/02/06 03:54:12]

Done.

+#include "base/macros.h"
+#include "base/memory/scoped_ptr.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.h"
+#include "media/base/audio_decoder.h"
+#include "media/base/audio_decoder_config.h"
+#include "media/base/cdm_context.h"
+#include "media/base/media_export.h"
+#include "media/base/media_keys.h"
+
+namespace base {
+class SingleThreadTaskRunner;
+}
+
+namespace media {
+
+// AudioDecoder based on Android's MediaCodec API.
+//
+// It is intended for EME encrypted audio streams wich on Android require
+// the MediaCodec API for the playback and coordination with MediaDRM API.
+// Therefore, this decoder is a decrypting decoder: it is able to accept an
+// encrypted stream and produce the decrypted and decoded output.

[xhwang, 2016/02/05 21:37:37]

When encrypted audio streams work, I suppose clear streams will also work. We'd
like to be as general as possible (especially when it doesn't involve extra
work, which I suppose is the case here).

So in this comment we don't really need limit ourselves to encrypted audio
streams. Also, it'll actually be harder to test using encrypted stream +
widevine since you can't get widevine licenses from unittests. We'll figure that
out later. But for now you can at least add unittest for clear streams.

[Tima Vaisburd, 2016/02/06 03:54:12]

I did not mean to limit, rather I wanted to say that it can accept encrypted as
well as non-encrypted streams. Rewrote the comment a little, please take a look.

[xhwang, 2016/02/08 20:03:33]

I guess "this decoder is a decrypting decoder" is a bit ambiguous to me. Mainly
because we already have a DecryptingAudioDecoder which only accepts encrypted
streams. Can we somehow make the comment more explicit, e.g. This decoder can
handle clear and encrypted audio streams.

+//
+// In order to use MediaCodec and MediaDRM API we plan to use it in the GPU
+// process through Mojo IPC. See https://docs.google.com/a/google.com/document/
+// d/1YwMZdC-eWe6EkAshSqy8P1b7h2XU469RvbyGPW6EpZs

[Tima Vaisburd, 2016/02/04 22:59:12]

This link points to the Google restricted doc, is this ok?

[xhwang, 2016/02/05 21:37:37]

We'd like to avoid it.

Actually, you don't really need this comment. The AAD is a generic class that
can be used anywhere for audio decoding (while the sandbox rules allow). For
example, someone may want to use it in the browser process some day. We
shouldn't care how it will be used.

[xhwang, 2016/02/05 21:37:37]

nit: A link that across two lines with "//" and a space in the middle is very
hard to use. In this case, consider using a short URL (goo.gl/).

[Tima Vaisburd, 2016/02/06 03:54:11]

Removed

[Tima Vaisburd, 2016/02/06 03:54:12]

Acknowledged.

+
+class MEDIA_EXPORT AndroidAudioDecoder : public AudioDecoder {
+ public:
+  AndroidAudioDecoder(scoped_refptr<base::SingleThreadTaskRunner> task_runner);

[xhwang, 2016/02/05 21:37:37]

explicit

[xhwang, 2016/02/05 21:37:37]

in media/ we like to pass scoped_refptr by const-ref to avoid an extra
AddRef/Release.

[Tima Vaisburd, 2016/02/06 03:54:12]

Done.

[Tima Vaisburd, 2016/02/06 03:54:12]

Done.

+  ~AndroidAudioDecoder() override;
+
+  // AudioDecoder implementation.
+  std::string GetDisplayName() const override;
+  void Initialize(const AudioDecoderConfig& config,
+                  const SetCdmReadyCB& set_cdm_ready_cb,
+                  const InitCB& init_cb,
+                  const OutputCB& output_cb) override;
+  void Decode(const scoped_refptr<DecoderBuffer>& buffer,
+              const DecodeCB& decode_cb) override;
+  void Reset(const base::Closure& closure) override;
+
+ private:
+  // Possible states.
+  enum State {

[xhwang, 2016/02/05 21:37:37]

Can we have comments/doc about how these states work?

[xhwang, 2016/02/08 20:03:34]

This is not done :) I'll continue to review the implementation after learning
how the code is supposed to work.

+    kStateUninitialized,
+    kStateReady,
+    kStateWaitingForKey,
+    kStateDraining,
+    kStateDrained,
+    kStateError,

[xhwang, 2016/02/05 21:37:37]

Use MACRO_STYLE for enums:

https://www.chromium.org/developers/coding-style

"Though the Google C++ Style Guide now says to use kConstantNaming for enums,
Chromium was written using MACRO_STYLE naming.  In enums that are actually
enumerations (i.e. have multiple values), continue to use this style for
consistency.  Use kConstantNaming when using the "enum hack" to define a single
constant, as you would for a const int or the like."

[Tima Vaisburd, 2016/02/06 03:54:11]

This rationale argues for deviation from the Google style guide for the sake of
consistency, but the decoders in this very directory are already using
kConstantNaming:
https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/ffmp...
https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/ffmp...
https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/gpu_...
https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/vpx_...
https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/decr...

It seems I have followed an established convention.

[xhwang, 2016/02/08 20:03:33]

Once upon a time kCamelCase was acceptable for enums and that's why you see them
in these "old" decoders. These code should be updated but nobody have the time
to do it. All new file  should follow the style guide.

Some new code is already using the UPPER_CASE style:
https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/deco...
https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/fake...

+  };
+
+  // Information about the MediaCodec's output buffer.
+  struct OutputBufferInfo {
+    int buf_index;
+    size_t offset;
+    size_t size;
+    base::TimeDelta pts;
+    bool is_eos;
+    bool is_key_frame;
+  };
+
+  // Callback called when a new key is available after the codec received
+  // the status MEDIA_CODEC_NO_KEY.
+  void OnKeyAdded();
+
+  // Does the MediaCodec processing cycle: enqueues an input buffer, then
+  // dequeues output buffers.
+  void DoIOTask();
+
+  // Enqueue one pending input buffer into MediaCodec if MediaCodec has a room.

[xhwang, 2016/02/05 21:37:37]

nit: here and below, Enqueue_s_

[Tima Vaisburd, 2016/02/06 03:54:12]

Done.

+  // Returns true if any input was processed.
+  bool QueueInput();
+
+  // Dequeue all output buffers from MediaCodec that are immediately available.

[xhwang, 2016/02/05 21:37:37]

what's the return value?

[Tima Vaisburd, 2016/02/06 03:54:11]

Done.

+  bool DequeueOutput();
+
+  // Start or stop our work-polling timer based on whether we did any work, and
+  // how long it has been since we've done work.  Calling this with true will
+  // start the timer.  Calling it with false may stop the timer.
+  void ManageTimer(bool did_work);

[xhwang, 2016/02/05 21:37:37]

This seems complicated: "will start the timer" and "may stop the timer".

Again I feel we should have some high level doc explaining how things are
supposed to work, including IOTask, timer, etc.

(BTW, I am looking at the implementation yet on purpose.)

[Tima Vaisburd, 2016/02/06 03:54:11]

Rewrote the explanation.

+
+  // Helper method to change the state.
+  void SetState(State new_state);
+
+  // The following helper methods ConfigureMediaCodec(), OnDecodedFrame(),
+  // OnOutputFormatChanged() are specific to the stream (audio/video), but
+  // others seem to apply to any decoder which would be based on MediaCodec.
+  // TODO(timav): refactor the common part out and use it here and in AVDA
+  // (http://crbug.com/583082).
+
+  // Configures |media_codec_| with the given codec parameters from the client.

[xhwang, 2016/02/05 21:37:37]

Can we s/given codec parameters from the client/|config_|/?

What's the return value?

[Tima Vaisburd, 2016/02/06 03:54:12]

Done.

+  bool ConfigureMediaCodec();
+
+  // Processes the output buffer after it comes from MediaCodec.
+  void OnDecodedFrame(const OutputBufferInfo* out);

[xhwang, 2016/02/05 21:37:37]

We don't typically use const-pointer. Can we use const-ref here?

[Tima Vaisburd, 2016/02/06 03:54:12]

Done.

+
+  // Processed the output format change.
+  void OnOutputFormatChanged();
+
+  // A helper function for logging.
+  static const char* AsString(State state);

[xhwang, 2016/02/05 21:37:37]

Can you make this a file-scope function in the .cc file?

[Tima Vaisburd, 2016/02/06 03:54:12]

Wouldn't it require making the State public?

+
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner_;

[xhwang, 2016/02/05 21:37:37]

Is this class single threaded? Will everything run on |task_runner_|? Maybe
worth a comment.

+
+  // The current state of the object.
+  State state_;
+
+  // The queue of encoded (and maybe encrypted) buffers.
+  using BufferCBPair = std::pair<scoped_refptr<DecoderBuffer>, DecodeCB>;

[xhwang, 2016/02/05 21:37:37]

Add include for std::pair: <utility>

[Tima Vaisburd, 2016/02/06 03:54:12]

Done.

+  using InputQueue = std::deque<BufferCBPair>;
+  InputQueue input_queue_;

[xhwang, 2016/02/05 21:37:37]

Can you comment on why we need to put input buffers in a queue? (e.g. we don't
need a queue for other decoders.)

[Tima Vaisburd, 2016/02/06 03:54:12]

Done.

+
+  // Decoder configuration cache.
+  AudioDecoderConfig config_;
+
+  // Frequently used constants derived from |config_|.
+  int channel_count_;
+  int bytes_per_frame_;
+
+  // Callback that reports the end of initialization and its status.
+  InitCB init_cb_;
+
+  // Callback that delivers output frames.
+  OutputCB output_cb_;
+
+  // DecodeCB that should be fired after the decoder has been flashed in case
+  // of EOS.
+  DecodeCB eos_decode_cb_;
+
+  // The bridge to Android's MediaCodec API.
+  scoped_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 most_recent_work_;

[xhwang, 2016/02/05 21:37:37]

nit: |most_recent_work_| doesn't sounds like a time.

[Tima Vaisburd, 2016/02/06 03:54:12]

s/most_recent_work_/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.
+  int pending_input_buf_index_;
+
+  // Weak pointer factory must be the last member variable.
+  base::WeakPtrFactory<AndroidAudioDecoder> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(AndroidAudioDecoder);
+};
+
+}  // namespace media
+
+#endif  // MEDIA_FILTERS_ANDROID_ANDROID_AUDIO_DECODER_H_