AndroidVDA by using Android's MediaCodec API.

content/common/gpu/media/android_video_decode_accelerator.h

+// Copyright (c) 2013 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 CONTENT_COMMON_GPU_MEDIA_ANDROID_VIDEO_DECODE_ACCELERATOR_H_
+#define CONTENT_COMMON_GPU_MEDIA_ANDROID_VIDEO_DECODE_ACCELERATOR_H_
+
+#include <dlfcn.h>
+#include <map>
+#include <queue>
+#include <set>

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

unused

[dwkang1, 2013/01/28 14:54:30]

Done.

+#include <string>
+#include <utility>

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

unused?

[dwkang1, 2013/01/28 14:54:30]

Done.

+#include <vector>
+
+#include "base/compiler_specific.h"
+#include "base/logging.h"
+#include "base/message_loop.h"

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

replace with fwd-declaration

[dwkang1, 2013/01/28 14:54:30]

Done.

+#include "base/shared_memory.h"

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

unnecessary (please audit the rest of the #includes)

[dwkang1, 2013/01/28 14:54:30]

Done.

+#include "content/common/android/surface_texture_bridge.h"
+#include "content/common/content_export.h"
+#include "media/video/video_decode_accelerator.h"
+
+namespace media {
+class MediaCodecBridge;
+}
+
+namespace content {
+
+class Gles2ExternalTextureCopier;
+
+typedef std::map<int32, media::PictureBuffer> PictureMap;

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

Only used for a private member; move into the private section of the class.

[dwkang1, 2013/01/28 14:54:30]

Done.

+
+// A VideoDecodeAccelerator implementation for Android.
+// This class decodes the input encoded stream by using Android's MediaCodec
+// class. http://developer.android.com/reference/android/media/MediaCodec.html
+class CONTENT_EXPORT AndroidVideoDecodeAccelerator :
+    public media::VideoDecodeAccelerator {
+ public:
+  // Does not take ownership of |client| which must outlive |*this|.
+  AndroidVideoDecodeAccelerator(
+      media::VideoDecodeAccelerator::Client* client,
+      const base::Callback<bool(void)>& make_context_current);
+  virtual ~AndroidVideoDecodeAccelerator();

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

dtor should be private since only Destroy should be deleting.
(I guess the other VDA impl's need this change too; I'll probably send a
separate CL for that)

[dwkang1, 2013/01/28 14:54:30]

Done.

+
+  // media::VideoDecodeAccelerator implementation.
+  bool Initialize(media::VideoCodecProfile profile) OVERRIDE;
+  void Decode(const media::BitstreamBuffer& bitstream_buffer) OVERRIDE;
+  virtual void AssignPictureBuffers(
+      const std::vector<media::PictureBuffer>& buffers) OVERRIDE;
+  void ReusePictureBuffer(int32 picture_buffer_id) OVERRIDE;
+  void Flush() OVERRIDE;
+  void Reset() OVERRIDE;
+  void Destroy() OVERRIDE;
+
+ private:
+  enum Codec {

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

You can drop this enum and the codec_ parameter by simply passing the profile
param from Initialize to ConfigureMediaCodec().

[dwkang1, 2013/01/28 14:54:30]

Actually, codec_ is needed when we reconfigure MediaCodec.

+    UNKNOWN,
+    H264,
+    VP8
+  };
+  enum State {
+    NO_ERROR,
+    ERROR,
+  };
+
+  static const base::TimeDelta kDecodePollDelay;

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

I don't know what this is used for yet, but I suspect I'm not going to be happy
when I find out... ;)

[dwkang1, 2013/01/28 14:54:30]

As you found, I am afraid that I have to say MediaCodec requires it.

+
+  void SendCurrentSurfaceToClient();

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

You need to document everything that's not obvious, which is pretty much all the
methods in this class...

[dwkang1, 2013/01/28 14:54:30]

Done.

+  void CopyCurrentFrameToPictureBuffer(
+      int32 picture_buffer_id, float transfrom_matrix[16]);
+
+  void DoDecode();
+  void ConfigureMediaCodec();
+  void QueueInput();
+  void DequeueOutput();
+
+  MessageLoop* message_loop_;
+
+  // To expose client callbacks from VideoDecodeAccelerator.
+  // NOTE: all calls to this object *MUST* be executed in message_loop_.

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

This is the first time there's an implication that perhaps more than one thread
will be touching this class.  Is that true?  If it is, you need a class-level
comment explaining the threading characteristics of this class.
If it is not true, then I suspect this comment can be dropped, and uses of
message_loop_ replaced with MessageLoop::current() (and drop the message_loop_
member).  To enforce single-threadedness, use a ThreadChecker.

[dwkang1, 2013/01/28 14:54:30]

That is not true. I should have deleted this comments before uploading. Will add
ThreadChecker.

+  Client* client_;
+
+  base::Callback<bool(void)> make_context_current_;
+
+  // These members are only used during Initialization.

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

I think this comment refers to just one member, so s/members/member/ but
hopefully both the comment and the member can be dropped.

[dwkang1, 2013/01/28 14:54:30]

Ditto. My bad. I should have removed this also.

+  Codec codec_;
+
+  State state_;
+  PictureMap picture_map_;
+  std::queue<int32> free_picture_ids_;
+  scoped_ptr<media::MediaCodecBridge> media_codec_;
+  scoped_refptr<SurfaceTextureBridge> surface_texture_;
+  uint32 surface_texture_id_;
+  bool picturebuffer_requested_;

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

s/buffer/buffers/ ?

[dwkang1, 2013/01/28 14:54:30]

Done.

+
+  typedef std::queue<media::BitstreamBuffer> BitstreamBufferList;
+  BitstreamBufferList pending_bitstream_buffers_;

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

pending where?  At the decoder or awaiting submission to the decoder (e.g. b/c a
Decode() arrived during a Reset())?
Comments comments comments :)

[dwkang1, 2013/01/28 14:54:30]

Done.

+
+  int32 color_format_;

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

Can this not be a more specifically-typed member?  
Document the possibly-held values, or point at something that documents (an
enumeration, etc).

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

I believe this and the next two members (width/height) are only used in a single
function, so can be local vars there instead of class members.

[dwkang1, 2013/01/28 14:54:30]

Done.

[dwkang1, 2013/01/28 14:54:30]

Done.

+  int32 width_;
+  int32 height_;

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

Replace width/height with gfx::Size

[dwkang1, 2013/01/28 14:54:30]

Removed.

+  int32 current_bitstream_id_;
+
+  scoped_ptr<Gles2ExternalTextureCopier> texture_copier_;
+};
+
+}  // namespace content
+
+#endif  // CONTENT_COMMON_GPU_MEDIA_ANDROID_VIDEO_DECODE_ACCELERATOR_H_