VDA: Restructure EVDA

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
index 7ccd5940474ff2c8138796a7336e0357e87fe951..cb8cc70bbcbdd98474dc4737ffcd78b43cedbcb0 100644
--- a/content/common/gpu/media/exynos_video_decode_accelerator.h
+++ b/content/common/gpu/media/exynos_video_decode_accelerator.h
@@ -10,6 +10,7 @@
 
 #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"
@@ -28,6 +29,20 @@ 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
@@ -56,17 +71,16 @@ class H264Parser;
 // 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.
@@ -82,6 +96,22 @@ class CONTENT_EXPORT ExynosVideoDecodeAccelerator
   // 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 {
@@ -234,9 +264,6 @@ class CONTENT_EXPORT ExynosVideoDecodeAccelerator
   // 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();
@@ -303,7 +330,7 @@ class CONTENT_EXPORT ExynosVideoDecodeAccelerator
   // 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
@@ -363,7 +390,7 @@ class CONTENT_EXPORT ExynosVideoDecodeAccelerator
   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_;
@@ -416,12 +443,11 @@ class CONTENT_EXPORT ExynosVideoDecodeAccelerator
 
   // 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