VDA: Restructure EVDA

content/common/gpu/media/exynos_video_decode_accelerator.h

 // 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 the hardware video decoder present on the Exynos SoC.
 
 #ifndef CONTENT_COMMON_GPU_MEDIA_EXYNOS_VIDEO_DECODE_ACCELERATOR_H_
 #define CONTENT_COMMON_GPU_MEDIA_EXYNOS_VIDEO_DECODE_ACCELERATOR_H_
 
 #include <queue>
 #include <vector>
+#include <poll.h>

[Pawel Osciak, 2013/12/24 03:45:24]

Please keep lexicographical order.

In general, please follow Chromium coding style:
http://www.chromium.org/developers/coding-style.

Here, and everywhere else in this patch. I will not be commenting on all the
coding style errors in the first pass.

As I mentioned before, you may also find "git cl format" useful for automatic
formatting.

 
 #include "base/callback_forward.h"
 #include "base/memory/linked_ptr.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/threading/thread.h"
 #include "content/common/content_export.h"
 #include "content/common/gpu/media/video_decode_accelerator_impl.h"
 #include "media/base/limits.h"
 #include "media/base/video_decoder_config.h"
 #include "media/video/picture.h"
 #include "ui/gfx/size.h"
 #include "ui/gl/gl_bindings.h"
 
 namespace base {
 class MessageLoopProxy;
 }
 
 namespace content {
+
+class V4L2Device {

[Pawel Osciak, 2013/12/24 03:45:24]

Please either make the interface a nested class in V4L2VDA or extract to a
separate file.

+  public :

[Pawel Osciak, 2013/12/24 03:45:24]

Coding style for class definitions:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Class_...

+    virtual int dev_open(const char *fd,int flags) = 0;

[Pawel Osciak, 2013/12/24 03:45:24]

Please add documentation for all members.

[Pawel Osciak, 2013/12/24 03:45:24]

Style, method names:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

also spacing (space before the comma).

Also please drop the "dev_" prefix.

+    virtual int dev_close (int fd) = 0;

[Pawel Osciak, 2013/12/24 03:45:24]

Does the client of this class need to be aware of the fd at all? I think it
doesn't have to. Please keep it as an implementation detail of V4L2Device if
possible.

+    virtual int dev_ioctl (int fd, int flags, void *arg) = 0;
+    virtual int dev_poll (struct pollfd *fds, nfds_t nfds, int n) = 0;

[Pawel Osciak, 2013/12/24 03:45:24]

Similarly, we should be able to spare the client at least some of these
arguments (probably all).

+    virtual bool SetDevicePollInterrupt(int fd) = 0;
+    virtual bool ClearDevicePollInterrupt(int fd) = 0;
+    virtual void *dev_mmap (void *addr,
+        unsigned int len, int prot, int flags, int fd, unsigned int offset) = 0;

[Pawel Osciak, 2013/12/24 03:45:24]

Parameter stale wrapping is wrong, please see:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

+    virtual void dev_munmap (void *addr, unsigned int len);
+};

[Pawel Osciak, 2013/12/24 03:45:24]

Please also, as I mentioned before, have a factory method to create
implementations and use it instead.

+
 class H264Parser;
 
 // This class handles Exynos video acceleration directly through the V4L2
 // device exported by the Multi Format Codec hardware block.
 //
 // 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 child thread, which is the main GPU process thread which calls the
 //   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, through
 //   the *Task() routines, as well as V4L2 device events, through
 //   ServiceDeviceTask().  Almost all state modification is done on this thread.
 // * The device_poll_thread_, owned by this class.  All it does is epoll() on
 //   the V4L2 in DevicePollTask() and schedule a ServiceDeviceTask() on the
 //   decoder_thread_ when something interesting happens.
 //   TODO(sheu): replace this thread with an TYPE_IO decoder_thread_.
 //
 // 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 CONTENT_EXPORT ExynosVideoDecodeAccelerator
+class CONTENT_EXPORT V4L2VideoDecodeAccelerator
     : public VideoDecodeAcceleratorImpl {
  public:
-  ExynosVideoDecodeAccelerator(
+  V4L2VideoDecodeAccelerator(
       EGLDisplay egl_display,
-      EGLContext egl_context,

[Pawel Osciak, 2013/12/24 03:45:24]

You are not rebased on top of ToT if you still have this.

       Client* client,
       const base::WeakPtr<Client>& io_client_,
       const base::Callback<bool(void)>& make_context_current,
       const scoped_refptr<base::MessageLoopProxy>& io_message_loop_proxy);
-  virtual ~ExynosVideoDecodeAccelerator();
+  virtual ~V4L2VideoDecodeAccelerator();
 
   // 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;
 
   // VideoDecodeAcceleratorImpl implementation.
   virtual bool CanDecodeOnIOThread() OVERRIDE;
 
+  class ExynosV4L2Device : public V4L2Device {

[Pawel Osciak, 2013/12/24 03:45:24]

Please abstract to a separate file.

+  public :

[Pawel Osciak, 2013/12/24 03:45:24]

Style (indent, spacing).

+    ExynosV4L2Device() {}
+    int dev_open(const char *fd, int flags);

[Pawel Osciak, 2013/12/24 03:45:24]

Please use the OVERRIDE macro.

+    int dev_close (int fd);
+    int dev_ioctl (int fd, int flags, void *arg);
+    int dev_poll (struct pollfd *fds, nfds_t nfds, int n);
+    // Set/clear the device poll interrupt (using device_poll_interrupt_fd_).
+    bool SetDevicePollInterrupt(int fd);
+    bool ClearDevicePollInterrupt(int fd);
+    void *dev_mmap (void *addr,
+           unsigned int len, int prot, int flags, int fd, unsigned int offset);
+    void dev_munmap (void *addr, unsigned int len);
+  };
+  ExynosV4L2Device *device;

[Pawel Osciak, 2013/12/24 03:45:24]

This should be a a scoped_ptr.

+
  private:
   // These are rather subjectively tuned.
   enum {
     kMfcInputBufferCount = 8,
     // TODO(posciak): determine MFC input buffer size based on level limits.
     // See http://crbug.com/255116.
     kMfcInputBufferMaxSize = 1024 * 1024,
     // Number of output buffers to use for each VDA stage above what's required
     // by the decoder (e.g. DPB size, in H264).  We need
     // media::limits::kMaxVideoFrames to fill up the GpuVideoDecode pipeline,
@@ skipping 132 matching lines @@
   void ResetDoneTask();
 
   // Device destruction task.
   void DestroyTask();
 
   // Attempt to start/stop device_poll_thread_.
   bool StartDevicePoll();
   // If |keep_mfc_input_state| is true, don't reset MFC input state; used during
   // resolution change.
   bool StopDevicePoll(bool keep_mfc_input_state);
-  // Set/clear the device poll interrupt (using device_poll_interrupt_fd_).
-  bool SetDevicePollInterrupt();
-  bool ClearDevicePollInterrupt();
 
   void StartResolutionChangeIfNeeded();
   void FinishResolutionChange();
   void ResumeAfterResolutionChange();
 
   // Try to get output format from MFC, detected after parsing the beginning
   // of the stream. Sets |again| to true if more parsing is needed.
   bool GetFormatInfo(struct v4l2_format* format, bool* again);
   // Create MFC output buffers for the given |format|.
   bool CreateBuffersForFormat(const struct v4l2_format& format);
@@ skipping 46 matching lines @@
 
   // Message loop of the IO thread.
   scoped_refptr<base::MessageLoopProxy> io_message_loop_proxy_;
 
   // WeakPtr<> pointing to |this| for use in posting tasks from the decoder or
   // device worker threads back to the child thread.  Because the worker threads
   // are members of this class, any task running on those threads is guaranteed
   // that this object is still alive.  As a result, tasks posted from the child
   // thread to the decoder or device thread should use base::Unretained(this),
   // and tasks posted the other way should use |weak_this_|.
-  base::WeakPtr<ExynosVideoDecodeAccelerator> weak_this_;
+  base::WeakPtr<V4L2VideoDecodeAccelerator> weak_this_;
 
   // To expose client callbacks from VideoDecodeAccelerator.
   // NOTE: all calls to these objects *MUST* be executed on
   // child_message_loop_proxy_.
   base::WeakPtrFactory<Client> client_ptr_factory_;
   base::WeakPtr<Client> client_;
   // Callbacks to |io_client_| must be executed on |io_message_loop_proxy_|.
   base::WeakPtr<Client> io_client_;
 
   //
@@ skipping 39 matching lines @@
 
   //
   // Hardware state and associated queues.  Since decoder_thread_ services
   // the hardware, decoder_thread_ owns these too.
   //
 
   // Completed decode buffers, waiting for MFC.
   std::queue<int> mfc_input_ready_queue_;
 
   // MFC decode device.
-  int mfc_fd_;
+  int videodec_fd_;

[Pawel Osciak, 2013/12/24 03:45:24]

Don't need this anymore, we are using the device class abstraction instead.

 
   // MFC input buffer state.
   bool mfc_input_streamon_;
   // MFC input buffers enqueued to device.
   int mfc_input_buffer_queued_count_;
   // Input buffers ready to use, as a LIFO since we don't care about ordering.
   std::vector<int> mfc_free_input_buffers_;
   // Mapping of int index to MFC input buffer record.
   std::vector<MfcInputRecord> mfc_input_buffer_map_;
 
@@ skipping 32 matching lines @@
 
   //
   // Other state, held by the child (main) thread.
   //
 
   // Make our context current before running any EGL entry points.
   base::Callback<bool(void)> make_context_current_;
 
   // EGL state
   EGLDisplay egl_display_;
-  EGLContext egl_context_;
 
   // The codec we'll be decoding for.
   media::VideoCodecProfile video_profile_;
 
-  DISALLOW_COPY_AND_ASSIGN(ExynosVideoDecodeAccelerator);
+  DISALLOW_COPY_AND_ASSIGN(V4L2VideoDecodeAccelerator);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_COMMON_GPU_MEDIA_EXYNOS_VIDEO_DECODE_ACCELERATOR_H_