ozone: unify GpuMemoryBufferFactoryOzoneNativePixmap in content/

ui/gl/gl_image_ozone_native_pixmap.h

+// Copyright 2015 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 UI_GL_GL_IMAGE_OZONE_NATIVE_PIXMAP_H_
+#define UI_GL_GL_IMAGE_OZONE_NATIVE_PIXMAP_H_
+
+#include "ui/gfx/gpu_memory_buffer.h"
+#include "ui/gl/gl_image.h"
+#include "ui/ozone/public/native_pixmap.h"
+
+namespace gfx {
+
+class GL_EXPORT GLImageOzoneNativePixmap : public GLImage {

[reveman, 2015/07/28 14:40:57]

While it might not be ideal we currently use inheritance rather than composition
to share code between GLImage implementations so we should do the same here for
consistency. GLImageOzoneNativePixmap is a GLImageEGL.

[dshwang, 2015/07/28 16:21:55]

GLImageOzoneNativePixmap inherits GLImageLinuxDMABuffer, which inherits
GLImageEGL.
It's because GLImageLinuxDMABuffer can handle both cases;
"pixmap_->GetEGLClientBuffer()" and "pixmap_->GetDmaBufFd() >= 0"

[spang, 2015/07/28 19:34:27]

This is an abuse of inheritance.

I don't think we should be stacking this class on top of GLImageLinuxDMABuffer,
since it might not be a dmabuf.

If we need to support both EGL_LINUX_DMA_BUF_EXT nad EGL_NATIVE_PIXMAP_KHR in a
unified GLImageOzoneNativePixmap class, we should merge GLImageLinuxDMABuffer
directly into the class rather than use inheritance as a way to import
functionality.

One reason this is wrong is that in order to support a 3rd kind of image using
this pattern, we'd need multiple inheritance, but it's not allowed by the style
guide.

[dshwang, 2015/07/29 13:23:09]

Got it. It's same opinion to reveman's. I did make this class inherit GLImageEGL
and remove GLImageLinuxDMABuffer.

+ public:
+  static scoped_refptr<GLImageOzoneNativePixmap> Create(
+      scoped_refptr<ui::NativePixmap> pixmap,
+      const Size& size,
+      GpuMemoryBuffer::Format format,
+      unsigned internalformat);

[reveman, 2015/07/28 14:40:57]

Some consistency requests:

Please remove this Create function and make the ctor public like this:

GLImageOzoneNativePixmap(const gfx::Size& size, unsigned internalformat);

and a public Initialize function that looks like this:

bool Initialize(ui::NativePixmap* pixmap, gfx::GpuMemoryBuffer::Format format);

Initialize should set the pixmap and Destroy should reset it. Take a look at
other GLImage implementations if it's not clear exactly what this should look
like. 

Be keeping all this GLImage code consistent it's easier to maintain and easier
to make large changes to the GLImage system.

[dshwang, 2015/07/28 16:21:55]

Thank you for detailed explanation. Done.

+
+  void Destroy(bool have_context) override;
+  Size GetSize() override;
+  unsigned GetInternalFormat() override;
+  bool BindTexImage(unsigned target) override;
+  void ReleaseTexImage(unsigned target) override;
+  bool CopyTexSubImage(unsigned target,
+                       const Point& offset,
+                       const Rect& rect) override;
+  void WillUseTexImage() override;
+  void DidUseTexImage() override;
+  void WillModifyTexImage() override;
+  void DidModifyTexImage() override;
+  bool ScheduleOverlayPlane(AcceleratedWidget widget,
+                            int z_order,
+                            OverlayTransform transform,
+                            const Rect& bounds_rect,
+                            const RectF& crop_rect);
+
+ private:
+  GLImageOzoneNativePixmap(scoped_refptr<ui::NativePixmap> pixmap,
+                           const Size& size,
+                           GpuMemoryBuffer::Format format,
+                           unsigned internalformat);
+  ~GLImageOzoneNativePixmap() override;
+  bool Initialize();
+
+  scoped_refptr<ui::NativePixmap> pixmap_;
+  const Size size_;
+  GpuMemoryBuffer::Format format_;
+  unsigned internalformat_;
+  scoped_refptr<GLImage> image_;
+};
+
+}  // namespace gfx
+
+#endif  // UI_GL_GL_IMAGE_OZONE_NATIVE_PIXMAP_H_