VDA implementation for Exynos, using V4L2

content/common/gpu/media/exynos_video_decode_accelerator.h

Index: content/common/gpu/media/exynos_video_decode_accelerator.h
diff --git a/content/common/gpu/media/exynos_video_decode_accelerator.h b/content/common/gpu/media/exynos_video_decode_accelerator.h
new file mode 100644
index 0000000000000000000000000000000000000000..9facb4886dbf7704e931c468802f3a8adf25a034
--- /dev/null
+++ b/content/common/gpu/media/exynos_video_decode_accelerator.h
@@ -0,0 +1,374 @@
+// Copyright (c) 2012 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.
+//
+// This file contains an implementation of VideoDecoderAccelerator
+// that utilizes hardware video decoder present on Intel CPUs.

[Ami GONE FROM CHROMIUM, 2012/10/18 08:30:26]

no it doesn't ;)

[sheu, 2012/10/30 09:05:03]

Done.

+
+#ifndef CONTENT_COMMON_GPU_MEDIA_EXYNOS_VIDEO_DECODE_ACCELERATOR_H_
+#define CONTENT_COMMON_GPU_MEDIA_EXYNOS_VIDEO_DECODE_ACCELERATOR_H_
+
+#include <deque>
+#include <vector>
+
+#include "base/callback_forward.h"
+#include "base/memory/linked_ptr.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/threading/thread.h"
+#include "media/base/video_decoder_config.h"
+#include "media/video/video_decode_accelerator.h"
+#include "third_party/angle/include/EGL/egl.h"
+#include "third_party/angle/include/EGL/eglext.h"
+
+namespace base {
+class MessageLoopProxy;
+}
+
+namespace gfx {
+class GLContext;
+}
+
+// This class handls Exynos video acceleration directly through the V4L2 devices
+// exported by the Multi Format Codec and GScaler hardware blocks.
+//
+// The threading model of this class is driven by the fact that it needs to
+// interface two fundamentally different event queues -- the one Chromium
+// provides through MessageLoop, and the one driven by the V4L2 devices which
+// is waited on with epoll().  There are three threads involved in this class:
+//
+// * The API thread, which is the main GPU process thread which calls the

[Ami GONE FROM CHROMIUM, 2012/10/29 23:30:16]

Chromium terminology calls this thread the ChildThread (the "main" thread of
each of the child processes, such as renderer, gpu, plugin, etc).  I don't think
it serves any purpose to refer to it as the "API thread".  Replace references
throughout?

[sheu, 2012/10/30 09:05:03]

Done.

+//   media::VideoDecodeAccelerator entry points.  Calls from this thread
+//   generally do not block (with the exception of Initialize() and Destroy()).
+//   They post tasks to the decoder_thread_, which actually services the task
+//   and calls back when complete through the
+//   media::VideoDecodeAccelerator::Client interface.
+// * The decoder_thread_, owned by this class.  It services API tasks as well as
+//   device events.  Pretty much all state modification is done on this thread.
+// * The device_thread_, owned by this class.  All it does is epoll() on the
+//   V4L2 device and notify the decoder_thread_ when something interesting
+//   happens.
+//
+// Note that this class has no locks!  Everything's serviced on the
+// decoder_thread_, so there are no synchronization issues.
+// ... well, there are, but it's a matter of getting messages posted in the
+// right order, not fiddling with locks.
+class ExynosVideoDecodeAccelerator : public media::VideoDecodeAccelerator {
+ public:
+  ExynosVideoDecodeAccelerator(gfx::GLContext* gl_context,

[Ami GONE FROM CHROMIUM, 2012/10/29 23:30:16]

style: here & elsewhere, arg lists should be either all +4 indented or indented
to the opening paren, not mixed (so here you want to add a newline before the
first arg)

[sheu, 2012/10/30 09:05:03]

Done.

+      Client* client,
+      const base::Callback<bool(void)>& make_context_current);
+  virtual ~ExynosVideoDecodeAccelerator();
+
+  // media::VideoDecodeAccelerator implementation.
+  // Note: Initialize() and Destroy() are synchronous.
+  virtual bool Initialize(media::VideoCodecProfile profile) OVERRIDE;
+  virtual void Decode(const media::BitstreamBuffer& bitstream_buffer) OVERRIDE;
+  virtual void AssignPictureBuffers(
+      const std::vector<media::PictureBuffer>& buffers) OVERRIDE;
+  virtual void ReusePictureBuffer(int32 picture_buffer_id) OVERRIDE;
+  virtual void Flush() OVERRIDE;
+  virtual void Reset() OVERRIDE;
+  virtual void Destroy() OVERRIDE;
+
+  // Do any necessary initialization before the sandbox is enabled.
+  static void PreSandboxInitialization();
+
+  // Lazily initialize static data after sandbox is enabled.  Return false on
+  // init failure.
+  static bool PostSandboxInitialization();
+
+  // Call through the static function pointers we hold.

[Ami GONE FROM CHROMIUM, 2012/10/29 23:30:16]

Can't these be private?
For that matter, they seem like they could just live in the .cc file since they
are fully static.

[sheu, 2012/10/30 09:05:03]

Done.

+  static EGLSyncKHR eglCreateSyncKHR(EGLDisplay dpy, EGLenum type,
+      const EGLint* attrib_list);
+  static EGLBoolean eglDestroySyncKHR(EGLDisplay dpy, EGLSyncKHR sync);
+  static EGLint eglClientWaitSyncKHR(EGLDisplay dpy, EGLSyncKHR sync,
+      EGLint flags, EGLTimeKHR timeout);
+
+ private:
+  // These are rather subjectively tuned.

[Ami GONE FROM CHROMIUM, 2012/10/29 23:30:16]

Have you tested flash?  ihf@ found he needed 6-8 output buffers in flight to
keep smoothness in some cases.

[sheu, 2012/10/30 09:05:03]

I tested flash.  Over and over again.  I can turn things down pretty well on
this VDA :-)

+  enum {
+    kMfcInputBufferCount = 4,
+    kMfcOutputBufferExtraCount = 2,
+    kMfcInputBufferMaxSize = 1024 * 1024,
+    kGscInputBufferCount = 4,
+    kGscOutputBufferCount = 6,
+  };
+
+  // Internal state of the decoder.
+  enum State {
+    kUninitialized, // Initialize() not yet called.
+    kInitialized,   // Initialize() called.  Ready to start decoding.
+    kDecoding,      // DecodeBufferInitial() successful; decoding frames.
+    kResetting,     // Presently resetting.
+    kAfterReset,    // After Reset(), ready to start decoding again.

[Ami GONE FROM CHROMIUM, 2012/10/29 23:30:16]

I believe the only diff between this and kInitialized is buffer-allocation
stuff, which makes me think that this state should be deleted and instead have a
kNeedBuffers state between kInitialized and kDecoding.

[sheu, 2012/10/30 09:05:03]

I think the kInitialized/kDecoding/kAfterReset stages match the logical flow of
the state machine better.  Something like kNeedsBuffers would be better suited
for an independent flag, but I don't see that we need this.

+    kError,         // Error in kDecoding state.
+  };
+
+  // Record for decoder input buffers.

[Ami GONE FROM CHROMIUM, 2012/10/29 23:30:16]

Comments that apply to each of the structs defined below:

- Can move into the .cc file w/ a fwd-decl here?
- Declare the dtor explicitly instead of accepting a compiler-generated dtor in
every compilation unit that includes this file.
- Any reason not to initialize the values through the ctor?  That'll let you
make the fields const and simplify call-sites, I think.

[sheu, 2012/10/30 09:05:03]

I suppose I could move BitstreamBufferRecord, but the rest have containers
declared on them and have to be declared here.  I'd keep them in the same place
for consistency.

+  struct BitstreamBufferRecord {
+    BitstreamBufferRecord();
+    scoped_ptr<base::SharedMemory> shm;
+    size_t size;
+    int32 input_id;
+  };
+
+  // Record for MFC input buffers.
+  struct MfcInputRecord {
+    MfcInputRecord();
+    bool queued : 1;       // queued to device.

[Ami GONE FROM CHROMIUM, 2012/10/29 23:30:16]

here and below: why bother with the :1?
If the compiler actually obeyed you it'd make the following fields unaligned,
and probably cost in performance.

Generally unless you're going to COMPILE_ASSERT about the size of the struct,
specifying field widths is unwarranted (at least in chrome code).

[Ami GONE FROM CHROMIUM, 2012/10/29 23:30:16]

s/queued/at_device/ ?
(ditto for other "queued" fields below)

[sheu, 2012/10/30 09:05:03]

Done.

+    void* offset;          // mmap() offset.
+    size_t length;         // mmap() length.
+    off_t bytes_used;      // bytes filled in the mmap() segment.
+    int32 input_id;        // triggering input_id as given to Decode().
+  };
+
+  // Record for MFC output buffers.
+  struct MfcOutputRecord {
+    MfcOutputRecord();
+    bool queued : 1;       // queued to device.
+    size_t bytes_used[2];  // bytes used in each dmabuf.
+    void* offset[2];       // mmap() offset for each plane.
+    size_t length[2];      // mmap() length for each plane.

[Ami GONE FROM CHROMIUM, 2012/10/29 23:30:16]

what are these dmabufs/planes?

[sheu, 2012/10/30 09:05:03]

It's hardware-specific business; I'm not sure that a comment here would help if
you didn't already know what was up with dmabufs.

+    int32 input_id;        // triggering input_id as given to Decode().
+  };
+
+  // Record for GSC input buffers.
+  struct GscInputRecord {
+    GscInputRecord();
+    bool queued : 1;       // queued to device.
+    int mfc_output;        // MFC output to recycle when this input is complete

[Ami GONE FROM CHROMIUM, 2012/10/29 23:30:16]

s/MFC output/MfcOutputRecord/

[sheu, 2012/10/30 09:05:03]

Well, we're tracking the buffer index here, not really the record itself. 
Updated slightly.

[sheu, 2012/10/30 09:05:03]

We're tracking the output buffer itself, not the record.  Updated slightly.

+  };
+
+  // Record for GSC output buffers.
+  struct GscOutputRecord {
+    GscOutputRecord();
+    bool queued : 1;       // queued to device.
+    bool in_vda : 1;       // held by VDA.

[Ami GONE FROM CHROMIUM, 2012/10/29 23:30:16]

_this_ is the VDA :)
Do you mean at_client?

[sheu, 2012/10/30 09:05:03]

Done.

+    int fd;                // file descriptor from backing EGLImage.
+    EGLImageKHR egl_image; // backing EGLImage.
+    EGLSyncKHR egl_sync;   // sync the compositor's use of the EGLImage.
+    int32 picture_id;      // picture ID as returned to PictureReady().
+  };
+
+  // Auto-destruction reference for an array of EGLImage (for message-passing)
+  struct EGLImageKHRArrayRef {
+    EGLImageKHRArrayRef();
+    ~EGLImageKHRArrayRef();
+    EGLDisplay egl_display;
+    scoped_array<EGLImageKHR> egl_images;
+    scoped_array<int> egl_image_fds;
+    int egl_images_count;
+  };
+
+  // Auto-destruction reference for EGLSync (for message-passing)
+  struct EGLSyncKHRRef {
+    EGLSyncKHRRef();
+    ~EGLSyncKHRRef();
+    EGLDisplay egl_display;
+    EGLSyncKHR egl_sync;
+  };
+
+  //
+  // Decoding tasks, to be run on decode_thread_.
+  //
+
+  // Enqueue a BitstreamBuffer to decode.  This will enqueue a
+  // DecodeBufferTask() to actually decoe the buffer.

[Ami GONE FROM CHROMIUM, 2012/10/29 23:30:16]

typo: decoe

[sheu, 2012/10/30 09:05:03]

Done.

+  void DecodeTask(scoped_ptr<BitstreamBufferRecord> bitstream_record);

[Ami GONE FROM CHROMIUM, 2012/10/29 23:30:16]

I think you can improve function naming here.  WDYT of:
rename: EnqueueDecodeBuffer

[sheu, 2012/10/30 09:05:03]

Generally I'm matching each "Foo" call from the API with a "FooTask" call to the
decoder thread, so in this case it's named DecodeTask because it comes from
Decode.

+
+  // Decode from the queued BitstreamBuffers.  Calls DecodeBufferInitial()
+  // or DecodeBufferContinue().
+  void DecodeBufferTask();

[Ami GONE FROM CHROMIUM, 2012/10/29 23:30:16]

...and rename: DoSomeDecodeWork
(and below)

[sheu, 2012/10/30 09:05:03]

They're pretty closely associated with DecodeBufferTask().  Did you mean to
rename all the helper functions to Do*() calls?

+  bool DecodeBufferInitial(const void* data, size_t size);
+  bool DecodeBufferContinue(const void* data, size_t size);
+  // Helpers: accumulate and flush data for one decoded frame.
+  bool AppendToInputFrame(const void* data, size_t size);
+  bool FlushInputFrame();
+
+  // Process an AssignPictureBuffers() API call.  After this, the device_thread_
+  // can be started safely, since we have all our buffers.
+  void AssignPictureBuffersTask(scoped_ptr<EGLImageKHRArrayRef> egl_images_ref);
+
+  // Service I/O on the V4L2 devices.
+  void ServiceDeviceTask();
+  // Handle the various device queues.
+  void EnqueueMfc();
+  void DequeueMfc();
+  void EnqueueGsc();
+  void DequeueGsc();
+
+  // Process a ReusePictureBuffer() API call.  The API call create an EGLSync
+  // object on the main (GPU process) thread; we will record this object so we
+  // can wait on it before reusing the buffer.
+  void ReusePictureBufferTask(int32 picture_buffer_id,
+      scoped_ptr<EGLSyncKHRRef> egl_sync_ref);
+
+  // Flush() task.  API thread should not submit any more buffers until it
+  // receives the NotifyFlushDone callback.

[Ami GONE FROM CHROMIUM, 2012/10/29 23:30:16]

this is fine, but make sure you can deal with a Reset() or Destroy() coming in
before NotifyFlushDone fires.  (this is tested by v_d_a_unittest.cc, I believe)

[sheu, 2012/10/30 09:05:03]

Yep, that case should work.  It just sets a flag actually, that either Reset or
Destroy will unset.

+  void FlushTask();
+
+  // Reset() task.  This task will schedule a ResetDoneTask() that will send
+  // the NotifyResetDone callback.
+  void ResetTask();
+  void ResetDoneTask();
+
+  // Device destruction task.
+  void DestroyTask();
+
+  // Cleanup() task, mostly used on the error path.
+  void CleanupTask();
+
+  // Start/stop DeviceTask() running on device_thread_.
+  bool StartDevice();
+  bool StopDevice();

[Ami GONE FROM CHROMIUM, 2012/10/29 23:30:16]

You should doco the return value of these methods, but more alarming is that
almost all of their callsites ignore the return value.  Either void'ify if safe,
or make use of ret code everywhere (or ignore_result() it).

[sheu, 2012/10/30 09:05:03]

Done.

+
+  //
+  // Device tasks, to be run on device_thread_.
+  //
+
+  // The device task.
+  void DeviceTask();
+
+  //
+  // Safe from any thread (uses PostTask() to API thread).
+  //
+
+  // Error notification.
+  void NotifyError(Error error);
+
+  //
+  // Other utility functions
+  //
+
+  // Create the buffers we need.
+  bool CreateMfcInputBuffers();
+  bool CreateMfcOutputBuffers();
+  bool CreateGscInputBuffers();
+  bool CreateGscOutputBuffers();
+
+  // Destroy these buffers.
+  bool DestroyMfcInputBuffers();
+  bool DestroyMfcOutputBuffers();
+  bool DestroyGscInputBuffers();
+  bool DestroyGscOutputBuffers();

[Ami GONE FROM CHROMIUM, 2012/10/29 23:30:16]

Again, return values seem to be silently ignored sometimes.

[sheu, 2012/10/30 09:05:03]

Done.

+
+  // Our original calling message loop for the API thread.
+  scoped_refptr<base::MessageLoopProxy> message_loop_proxy_;

[Ami GONE FROM CHROMIUM, 2012/10/29 23:30:16]

name it something more distinct, like {main,child}_loop_proxy_ ?

[sheu, 2012/10/30 09:05:03]

Done.

+
+  // WeakPtr<> pointing to |this| for use in posting tasks from the decoder
+  // thread back to the API thread.  Because the decoder thread is a member of
+  // this class, any task running on the decoder thread is guaranteed that this
+  // object is still alive.  As a result, tasks posted from API thread to
+  // decoder thread should use base::Unretained(this), and tasks posted from the
+  // decoder thread to the API thread should use |weak_this_|.

[Ami GONE FROM CHROMIUM, 2012/10/29 23:30:16]

Comment that |device_thread_| follows the same logic as |decoder_thread_| ?

[sheu, 2012/10/30 09:05:03]

Done.

+  base::WeakPtr<ExynosVideoDecodeAccelerator> weak_this_;
+
+  // To expose client callbacks from VideoDecodeAccelerator.
+  // NOTE: all calls to these objects *MUST* be executed on message_loop_proxy_.
+  base::WeakPtrFactory<Client> client_ptr_factory_;
+  base::WeakPtr<Client> client_;
+
+  // This thread handles VDA API entry points and callbacks.

[Ami GONE FROM CHROMIUM, 2012/10/29 23:30:16]

I think this comment is misleading.

[sheu, 2012/10/30 09:05:03]

Done.

+  base::Thread decoder_thread_;
+  // Decoder state.  Owned by decoder_thread_.

[Ami GONE FROM CHROMIUM, 2012/10/29 23:30:16]

Replace "Owned by..." with a blanket comment at l.271 since all of this state
below is read/written only by decoder_thread_ ?

[sheu, 2012/10/30 09:05:03]

Done.  I did some declaration moving-around to hopefully make this more clear.

+  State decoder_state_;

[Ami GONE FROM CHROMIUM, 2012/10/29 23:30:16]

...except this seems to be read from both the decoder_thread_ and the main
thread.  What's up with that?

[sheu, 2012/10/30 09:05:03]

Probably the trickiest bit of state.  It's only read and written from the
decoder thread, except for when the decoder thread hasn't started yet, at which
time the child thread does read/write it.  Not even NotifyError touches it.

I should probably document this better.  I'm not sure though how I should
explicitly mark ownership of this by one thread.

[Ami GONE FROM CHROMIUM, 2012/10/31 01:06:50]

If you wanted to be super-XTREME about it, you could pass it in to the
decoder_thread_'s ctor, and make it ThreadLocal.  Just sayin'.

+  // Bitstream buffer we're presently reading.
+  scoped_ptr<BitstreamBufferRecord> decoder_current_bitstream_buffer_;

[Ami GONE FROM CHROMIUM, 2012/10/29 23:30:16]

s/decoder_//

+  // MFC input buffer we're presently filling.
+  int decoder_current_input_buffer_;

[Ami GONE FROM CHROMIUM, 2012/10/29 23:30:16]

ditto s/decoder_// here and below.

[sheu, 2012/10/30 09:05:03]

I was trying to mark these as more closely "decoder" state, as opposed to all
the mfc_ and gsc_ state bits which are also owned by the decoder thread, but are
more closely associated with the device.

I've updated the comments and moved things around.

+  // We track the number of buffer decode tasks we have scheduled, since each
+  // task execution should complete one buffer.  If we fall behind (due to
+  // resource backpressure, etc.), we'll have to schedule more to catch up.
+  int decoder_decode_buffer_tasks_scheduled_;
+  // Buffers in the pipe, five-by-five.

[Ami GONE FROM CHROMIUM, 2012/10/29 23:30:16]

lolwat?

[sheu, 2012/10/30 09:05:03]

http://www.youtube.com/watch?v=fOZk--oZdQk

(My recollection of it is mostly from StarCraft, but the original is Aliens)

+  int decoder_frames_inflight_;
+  // Buffers unreturned by the VDA
+  int decoder_frames_unreturned_;
+  // Are we waiting for a flush notification?
+  bool decoder_notify_flush_;

[Ami GONE FROM CHROMIUM, 2012/10/29 23:30:16]

Can you make a pass and align these variable names with their purposes?

[sheu, 2012/10/30 09:05:03]

Done.

+
+  // This thread handles notifications of V4L2 device changes.
+  base::Thread device_thread_;
+
+  // GL state
+  gfx::GLContext* gl_context_;
+
+  // Make our context current before running any EGL entry points.
+  base::Callback<bool(void)> make_context_current_;
+
+  // The codec we'll be decoding for.
+  media::VideoCodecProfile video_codec_;

[Ami GONE FROM CHROMIUM, 2012/10/29 23:30:16]

if you passed this to CreateMfcInputBuffers as a param you wouldn't need this
member.

[Ami GONE FROM CHROMIUM, 2012/10/29 23:30:16]

it's a profile, not a codec.

[sheu, 2012/10/30 09:05:03]

Done.

+
+  // EGL state
+  EGLContext egl_context_;

[Ami GONE FROM CHROMIUM, 2012/10/29 23:30:16]

you could assign these in the ctor and avoid the need for gl_context_ as a
member

[sheu, 2012/10/30 09:05:03]

A little forward-thinking: I'd like to standardize the constructors that the
various VDAs present in GpuVideoDecodeAccelerator::Initialize().  The lowest
common denominator seems to be a gfx::GLContext*, a Client, and a make_current
callback.

+  EGLDisplay egl_display_;
+
+  // Input queue for decoder_thread_: BitstreamBuffers in.
+  std::deque<linked_ptr<BitstreamBufferRecord> > decode_input_queue_;
+
+  // Completed decode buffers, waiting for MSC.

[Ami GONE FROM CHROMIUM, 2012/10/29 23:30:16]

typo: MSC

[Ami GONE FROM CHROMIUM, 2012/10/29 23:30:16]

I'm a bit confused about what makes an input decode buffer "complete".  Is there
a reference you can link to from the top of this file for how the V4L2 decode
API is used?

[sheu, 2012/10/30 09:05:03]

Done.  The bit about "decode_output_mfc_input_queue_" was a hold-over from when
I did some pre-processing before I stuffed it into the MFC.  Renamed now.

+  std::deque<int> decode_output_mfc_input_queue_;
+
+  // MFC decode device.
+  int mfc_fd_;

[Ami GONE FROM CHROMIUM, 2012/10/29 23:30:16]

Use base::ScopedFD, per IM convo.

[sheu, 2012/10/30 09:05:03]

Done.

+  // MFC input buffer state.
+  bool mfc_input_streamon_;
+  int mfc_input_buffer_count_;
+  std::deque<int> mfc_free_input_buffers_;
+  // Mapping to track MFC input buffers.

[Ami GONE FROM CHROMIUM, 2012/10/29 23:30:16]

There's a lot of "map" commentary/variable-naming below, but it's all applied to
vectors.  You want to clarify that other code refers to elements of these
vectors by their index?
If that's the case, you should describe when these are populated and the fact
that they are never reordered or otherwise modified until tear-down time.

[sheu, 2012/10/30 09:05:03]

I tweaked the comment a bit.  I think the fact that they're called a mapping and
accessed with the usual [] business might be self-documenting enough.

+  std::vector<MfcInputRecord> mfc_input_buffer_map_;
+
+  // MFC output buffer state.
+  bool mfc_output_streamon_;
+  int mfc_output_buffer_count_;
+  std::deque<int> mfc_free_output_buffers_;
+  // Mapping to track MFC output buffers.
+  std::vector<MfcOutputRecord> mfc_output_buffer_map_;
+  size_t mfc_output_buffer_size_[2];

[Ami GONE FROM CHROMIUM, 2012/10/29 23:30:16]

doco what the [2] is about.

[sheu, 2012/10/30 09:05:03]

Done.

+  uint32 mfc_output_buffer_pixelformat_;
+
+  // Completed MFC outputs, waiting for GSC.
+  std::deque<int> mfc_output_gsc_input_queue_;
+
+  // GSC decode device.
+  int gsc_fd_;

[Ami GONE FROM CHROMIUM, 2012/10/29 23:30:16]

scoped

[sheu, 2012/10/30 09:05:03]

Done.

+
+  // GSC input buffer state.
+  bool gsc_input_streamon_;
+  int gsc_input_buffer_count_;
+  std::deque<int> gsc_free_input_buffers_;

[Ami GONE FROM CHROMIUM, 2012/10/29 23:30:16]

does this really need to be a FIFO or could it just as well be a LIFO and use a
vector (pushing & popping on the back)?

[sheu, 2012/10/30 09:05:03]

Done.

+  // Mapping to track GSC input buffers.
+  std::vector<GscInputRecord> gsc_input_buffer_map_;
+
+  // GSC output buffer state.
+  bool gsc_output_streamon_;
+  int gsc_output_buffer_count_;
+  // GSC output buffers enqueued to device and ready, but not yet in use.
+  int gsc_output_buffer_prepared_count_;
+  // GSC output buffers enqueued to device, total.
+  int gsc_output_buffer_queued_count_;
+  std::deque<int> gsc_free_output_buffers_;

[Ami GONE FROM CHROMIUM, 2012/10/29 23:30:16]

ditto

[sheu, 2012/10/30 09:05:03]

This one does have to be a FIFO -- before we can reuse a buffer, we have to do
an expensive client sync on it.  The longer the buffer's been in the queue, the
less likely it is that we'll actually have to stall during the sync.

+  // Mapping to track GSC output buffers.
+  std::vector<GscOutputRecord> gsc_output_buffer_map_;
+

[Ami GONE FROM CHROMIUM, 2012/10/29 23:30:16]

ISTM there's a ton of repetition among the above 4 blocks.
Would it make sense to have a common DeviceState struct and just have 4 of them,
one per MFC/GSC input/output?

[sheu, 2012/10/30 09:05:03]

The only things really common to them are *_streamon_ and *_buffer_count_.

If I end up generalizing this to a V4L VDA, then it would most likely make sense
to break off that state into a separate *_decoder class like VAAPI has it at the
moment.

+  // Output picture size.
+  unsigned int frame_width_;

[Ami GONE FROM CHROMIUM, 2012/10/29 23:30:16]

gfx::Size

[sheu, 2012/10/30 09:05:03]

Done.

+  unsigned int frame_height_;
+
+  // "private" API pointers as exported by libmali, that we require.

[Ami GONE FROM CHROMIUM, 2012/10/29 23:30:16]

see above; l.358-369 belong only in the .cc file IMO

[sheu, 2012/10/30 09:05:03]

Done.

+  static void* libmali_handle;
+  static EGLBoolean(*mali_egl_image_get_buffer_ext_phandle)(
+      EGLImageKHR image, EGLint *attribs, void *phandle);
+
+  // Entry points for uncommon EGL extensions we use.
+  static EGLSyncKHR (*egl_create_sync_khr)(
+      EGLDisplay dpy, EGLenum type, const EGLint* attrib_list);
+  static EGLBoolean (*egl_destroy_sync_khr)(
+      EGLDisplay dpy, EGLSyncKHR sync);
+  static EGLint (*egl_client_wait_sync_khr)(
+      EGLDisplay dpy, EGLSyncKHR sync, EGLint flags, EGLTimeKHR timeout);
+
+  DISALLOW_COPY_AND_ASSIGN(ExynosVideoDecodeAccelerator);
+};
+
+#endif  // CONTENT_COMMON_GPU_MEDIA_EXYNOS_VIDEO_DECODE_ACCELERATOR_H_