media/gpu/v4l2: Use NativePixmapHandle to create GL/EGL images

We used to pass a vector of DMABuf FDs to the GL/EGL image creation
functions. This was very error-prone, as these vectors could contain
less FDs than there were planes in the buffer, and this information
was used to signal that the last FD contained several planes. The
function receiving these incomplete FDs would then try to figure out the
actual planes layout, with more or less luck, often introducing bugs in
the way.

Solve this issue once and for all by passing all importable buffer
information within the VDAs through NativePixmapHandles: they always
contain as many FDs as there are planes, and also have detailed layout
information. On top of that, they are what we receive when using import
mode (which means we can pass the exact layout information down to
GL/EGL), and we already have a function that creates them from a
VideoFrame. This helps reduce the amount of code, all the while making
it safer and more readable.

BUG=b:146842214
TEST=VDAtest passes on Hana.
TEST=VDAtest passes on Kevin with VD disabled.
TEST=VDAtest passes on Kukui.
TEST=crosvideo.appspot.com plays videos properly on Hana.
TEST=crosvideo.appspot.com plays videos properly on Kevin with VD disabled.
TEST=crosvideo.appspot.com plays videos properly on Kukui.
TEST=Youtube Android plays properly on Hana.
TEST=Youtube Android plays properly on Kevin with VD disabled.
TEST=Youtube Android plays properly on Kukui.

Change-Id: I88a78b786b1c90820221c9f0770a52a339913537
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1982499
Commit-Queue: Alexandre Courbot <acourbot@chromium.org>
Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#728450}

media/gpu/v4l2/generic_v4l2_device.cc

@@ -30,6 +30,7 @@
 #include "media/gpu/macros.h"
 #include "media/gpu/v4l2/generic_v4l2_device.h"
 #include "ui/gfx/native_pixmap.h"
+#include "ui/gfx/native_pixmap_handle.h"
 #include "ui/gl/egl_util.h"
 #include "ui/gl/gl_bindings.h"
 #include "ui/gl/gl_image_native_pixmap.h"
@@ -211,14 +212,13 @@ bool GenericV4L2Device::CanCreateEGLImageFrom(const Fourcc fourcc) {
          kEGLImageDrmFmtsSupported + base::size(kEGLImageDrmFmtsSupported);
 }
 
-EGLImageKHR GenericV4L2Device::CreateEGLImage(
-    EGLDisplay egl_display,
-    EGLContext /* egl_context */,
-    GLuint texture_id,
-    const gfx::Size& size,
-    unsigned int buffer_index,
-    const Fourcc fourcc,
-    std::vector<base::ScopedFD>&& dmabuf_fds) {
+EGLImageKHR GenericV4L2Device::CreateEGLImage(EGLDisplay egl_display,
+                                              EGLContext /* egl_context */,
+                                              GLuint texture_id,
+                                              const gfx::Size& size,
+                                              unsigned int buffer_index,
+                                              const Fourcc fourcc,
+                                              gfx::NativePixmapHandle handle) {
   DVLOGF(3);
 
   if (!CanCreateEGLImageFrom(fourcc)) {
@@ -226,17 +226,10 @@ EGLImageKHR GenericV4L2Device::CreateEGLImage(
     return EGL_NO_IMAGE_KHR;
   }
 
-  const VideoPixelFormat vf_format = fourcc.ToVideoPixelFormat();
   // Number of components, as opposed to the number of V4L2 planes, which is
   // just a buffer count.
-  size_t num_planes = VideoFrame::NumPlanes(vf_format);
+  const size_t num_planes = handle.planes.size();
   DCHECK_LE(num_planes, 3u);
-  if (num_planes < dmabuf_fds.size()) {
-    // It's possible for more than one DRM plane to reside in one V4L2 plane,
-    // but not the other way around. We must use all V4L2 planes.
-    LOG(ERROR) << "Invalid plane count";
-    return EGL_NO_IMAGE_KHR;
-  }
 
   std::vector<EGLint> attrs;
   attrs.push_back(EGL_WIDTH);
@@ -246,28 +239,13 @@ EGLImageKHR GenericV4L2Device::CreateEGLImage(
   attrs.push_back(EGL_LINUX_DRM_FOURCC_EXT);
   attrs.push_back(V4L2PixFmtToDrmFormat(fourcc.ToV4L2PixFmt()));
 
-  // For existing formats, if we have less buffers (V4L2 planes) than
-  // components (planes), the remaining planes are stored in the last
-  // V4L2 plane. Use one V4L2 plane per each component until we run out of V4L2
-  // planes, and use the last V4L2 plane for all remaining components, each
-  // with an offset equal to the size of the preceding planes in the same
-  // V4L2 plane.
-  size_t v4l2_plane = 0;
-  size_t plane_offset = 0;
   for (size_t plane = 0; plane < num_planes; ++plane) {
     attrs.push_back(EGL_DMA_BUF_PLANE0_FD_EXT + plane * 3);
-    attrs.push_back(dmabuf_fds[v4l2_plane].get());
+    attrs.push_back(handle.planes[plane].fd.get());
     attrs.push_back(EGL_DMA_BUF_PLANE0_OFFSET_EXT + plane * 3);
-    attrs.push_back(plane_offset);
+    attrs.push_back(handle.planes[plane].offset);
     attrs.push_back(EGL_DMA_BUF_PLANE0_PITCH_EXT + plane * 3);
-    attrs.push_back(VideoFrame::RowBytes(plane, vf_format, size.width()));
-
-    if (v4l2_plane + 1 < dmabuf_fds.size()) {
-      ++v4l2_plane;
-      plane_offset = 0;
-    } else {
-      plane_offset += VideoFrame::PlaneSize(vf_format, plane, size).GetArea();
-    }
+    attrs.push_back(handle.planes[plane].stride);
   }
 
   attrs.push_back(EGL_NONE);
@@ -287,52 +265,19 @@ EGLImageKHR GenericV4L2Device::CreateEGLImage(
 scoped_refptr<gl::GLImage> GenericV4L2Device::CreateGLImage(
     const gfx::Size& size,
     const Fourcc fourcc,
-    std::vector<base::ScopedFD>&& dmabuf_fds) {
+    gfx::NativePixmapHandle handle) {
   DVLOGF(3);
   DCHECK(CanCreateEGLImageFrom(fourcc));
 
-  const VideoPixelFormat vf_format = fourcc.ToVideoPixelFormat();
-  size_t num_planes = VideoFrame::NumPlanes(vf_format);
+  size_t num_planes = handle.planes.size();
   DCHECK_LE(num_planes, 3u);
-  DCHECK_LE(dmabuf_fds.size(), num_planes);
 
   gfx::NativePixmapHandle native_pixmap_handle;
 
-  std::vector<base::ScopedFD> duped_fds;
-  // The number of file descriptors can be less than the number of planes when
-  // v4l2 pix fmt, |fourcc|, is a single plane format. Duplicating the last
-  // file descriptor should be safely used for the later planes, because they
-  // are on the last buffer.
-  for (size_t i = 0; i < num_planes; ++i) {
-    int fd =
-        i < dmabuf_fds.size() ? dmabuf_fds[i].get() : dmabuf_fds.back().get();
-    duped_fds.emplace_back(HANDLE_EINTR(dup(fd)));
-    if (!duped_fds.back().is_valid()) {
-      VPLOGF(1) << "Failed duplicating a dmabuf fd";
-      return nullptr;
-    }
-  }
-
-  // For existing formats, if we have less buffers (V4L2 planes) than
-  // components (planes), the remaining planes are stored in the last
-  // V4L2 plane. Use one V4L2 plane per each component until we run out of V4L2
-  // planes, and use the last V4L2 plane for all remaining components, each
-  // with an offset equal to the size of the preceding planes in the same
-  // V4L2 plane.
-  size_t v4l2_plane = 0;
-  size_t plane_offset = 0;
   for (size_t p = 0; p < num_planes; ++p) {
     native_pixmap_handle.planes.emplace_back(
-        VideoFrame::RowBytes(p, vf_format, size.width()), plane_offset,
-        VideoFrame::PlaneSize(vf_format, p, size).GetArea(),
-        std::move(duped_fds[p]));
-
-    if (v4l2_plane + 1 < dmabuf_fds.size()) {
-      ++v4l2_plane;
-      plane_offset = 0;
-    } else {
-      plane_offset += VideoFrame::PlaneSize(vf_format, p, size).GetArea();
-    }
+        handle.planes[p].stride, handle.planes[p].offset, handle.planes[p].size,
+        std::move(handle.planes[p].fd));
   }
 
   gfx::BufferFormat buffer_format = gfx::BufferFormat::BGRA_8888;

media/gpu/v4l2/generic_v4l2_device.h

@@ -18,6 +18,7 @@
 #include "base/macros.h"
 #include "media/gpu/buildflags.h"
 #include "media/gpu/v4l2/v4l2_device.h"
+#include "ui/gfx/native_pixmap_handle.h"
 
 namespace media {
 
@@ -50,12 +51,12 @@ class GenericV4L2Device : public V4L2Device {
                              const gfx::Size& size,
                              unsigned int buffer_index,
                              const Fourcc fourcc,
-                             std::vector<base::ScopedFD>&& dmabuf_fds) override;
+                             gfx::NativePixmapHandle handle) override;
 
   scoped_refptr<gl::GLImage> CreateGLImage(
       const gfx::Size& size,
       const Fourcc fourcc,
-      std::vector<base::ScopedFD>&& dmabuf_fds) override;
+      gfx::NativePixmapHandle handle) override;
 
   EGLBoolean DestroyEGLImage(EGLDisplay egl_display,
                              EGLImageKHR egl_image) override;

media/gpu/v4l2/tegra_v4l2_device.cc

@@ -10,6 +10,7 @@
 #include "base/trace_event/trace_event.h"
 #include "media/gpu/macros.h"
 #include "media/gpu/v4l2/tegra_v4l2_device.h"
+#include "ui/gfx/native_pixmap_handle.h"
 #include "ui/gl/gl_bindings.h"
 
 namespace media {
@@ -227,7 +228,7 @@ EGLImageKHR TegraV4L2Device::CreateEGLImage(
     const gfx::Size& /* size */,
     unsigned int buffer_index,
     const Fourcc fourcc,
-    std::vector<base::ScopedFD>&& /* dmabuf_fds */) {
+    gfx::NativePixmapHandle /* handle */) {
   DVLOGF(3);
 
   if (!CanCreateEGLImageFrom(fourcc)) {
@@ -253,7 +254,7 @@ EGLImageKHR TegraV4L2Device::CreateEGLImage(
 scoped_refptr<gl::GLImage> TegraV4L2Device::CreateGLImage(
     const gfx::Size& size,
     const Fourcc fourcc,
-    std::vector<base::ScopedFD>&& dmabuf_fds) {
+    gfx::NativePixmapHandle /* handle */) {
   NOTREACHED();
   return nullptr;
 }

media/gpu/v4l2/tegra_v4l2_device.h

@@ -16,6 +16,7 @@
 
 #include "base/macros.h"
 #include "media/gpu/v4l2/v4l2_device.h"
+#include "ui/gfx/native_pixmap_handle.h"
 #include "ui/gl/gl_bindings.h"
 
 namespace media {
@@ -50,11 +51,11 @@ class TegraV4L2Device : public V4L2Device {
                              const gfx::Size& size,
                              unsigned int buffer_index,
                              const Fourcc fourcc,
-                             std::vector<base::ScopedFD>&& dmabuf_fds) override;
+                             gfx::NativePixmapHandle handle) override;
   scoped_refptr<gl::GLImage> CreateGLImage(
       const gfx::Size& size,
       const Fourcc fourcc,
-      std::vector<base::ScopedFD>&& dmabuf_fds) override;
+      gfx::NativePixmapHandle handle) override;
   EGLBoolean DestroyEGLImage(EGLDisplay egl_display,
                              EGLImageKHR egl_image) override;
   GLenum GetTextureTarget() override;

media/gpu/v4l2/v4l2_device.h

@@ -32,6 +32,7 @@
 #include "media/video/video_decode_accelerator.h"
 #include "media/video/video_encode_accelerator.h"
 #include "ui/gfx/geometry/size.h"
+#include "ui/gfx/native_pixmap_handle.h"
 #include "ui/gl/gl_bindings.h"
 #include "ui/gl/gl_image.h"
 
@@ -596,31 +597,23 @@ class MEDIA_GPU_EXPORT V4L2Device
   // for the current platform.
   virtual bool CanCreateEGLImageFrom(const Fourcc fourcc) = 0;
 
-  // Create an EGLImage from provided |dmabuf_fds| and bind |texture_id| to it.
+  // Create an EGLImage from provided |handle|, taking full ownership of it.
   // Some implementations may also require the V4L2 |buffer_index| of the buffer
-  // for which |dmabuf_fds| have been exported.
-  // This method takes full ownership of |dmabuf_fds|. The caller shall not use
-  // them after this method returns, and must duplicate them before passing them
-  // if it needs to access the buffers through them afterwards.
+  // for which |handle| has been exported.
   // Return EGL_NO_IMAGE_KHR on failure.
-  virtual EGLImageKHR CreateEGLImage(
-      EGLDisplay egl_display,
-      EGLContext egl_context,
-      GLuint texture_id,
-      const gfx::Size& size,
-      unsigned int buffer_index,
-      const Fourcc fourcc,
-      std::vector<base::ScopedFD>&& dmabuf_fds) = 0;
-
-  // Create a GLImage from provided |dmabuf_fds|.
-  // This method takes full ownership of |dmabuf_fds|. The caller shall not use
-  // them after this method returns, and must duplicate them before passing them
-  // if it needs to access the buffers through them afterwards.
-  // Return the newly created GLImage.
+  virtual EGLImageKHR CreateEGLImage(EGLDisplay egl_display,
+                                     EGLContext egl_context,
+                                     GLuint texture_id,
+                                     const gfx::Size& size,
+                                     unsigned int buffer_index,
+                                     const Fourcc fourcc,
+                                     gfx::NativePixmapHandle handle) = 0;
+
+  // Create a GLImage from provided |handle|, taking full ownership of it.
   virtual scoped_refptr<gl::GLImage> CreateGLImage(
       const gfx::Size& size,
       const Fourcc fourcc,
-      std::vector<base::ScopedFD>&& dmabuf_fds) = 0;
+      gfx::NativePixmapHandle handle) = 0;
 
   // Destroys the EGLImageKHR.
   virtual EGLBoolean DestroyEGLImage(EGLDisplay egl_display,

media/gpu/v4l2/v4l2_slice_video_decode_accelerator.h

@@ -31,6 +31,7 @@
 #include "media/gpu/vp8_decoder.h"
 #include "media/gpu/vp9_decoder.h"
 #include "media/video/video_decode_accelerator.h"
+#include "ui/gfx/native_pixmap_handle.h"
 #include "ui/gl/gl_fence_egl.h"
 
 namespace media {
@@ -264,14 +265,11 @@ class MEDIA_GPU_EXPORT V4L2SliceVideoDecodeAccelerator
   // via AssignPictureBuffers() on decoder thread.
   void AssignPictureBuffersTask(const std::vector<PictureBuffer>& buffers);
 
-  // Use buffer backed by dmabuf file descriptors in |passed_dmabuf_fds| for the
-  // OutputRecord associated with |picture_buffer_id|, taking ownership of the
-  // file descriptors. |stride| is the number of bytes from one row of pixels
-  // to the next row.
-  void ImportBufferForPictureTask(
-      int32_t picture_buffer_id,
-      std::vector<base::ScopedFD>&& passed_dmabuf_fds,
-      int32_t stride);
+  // Use buffer backed by |handle| for the OutputRecord associated with
+  // |picture_buffer_id|. |handle| does not need to be valid if we are in
+  // ALLOCATE mode and using an image processor.
+  void ImportBufferForPictureTask(int32_t picture_buffer_id,
+                                  gfx::NativePixmapHandle handle);
 
   // Check that |planes| and |dmabuf_fds| are valid in import mode and call
   // ImportBufferForPictureTask.
@@ -285,7 +283,7 @@ class MEDIA_GPU_EXPORT V4L2SliceVideoDecodeAccelerator
   // The GLImage will be associated |client_texture_id| in gles2 decoder.
   void CreateGLImageFor(size_t buffer_index,
                         int32_t picture_buffer_id,
-                        std::vector<base::ScopedFD>&& dmabuf_fds,
+                        gfx::NativePixmapHandle handle,
                         GLuint client_texture_id,
                         GLuint texture_id,
                         const gfx::Size& size,

media/gpu/v4l2/v4l2_video_decode_accelerator.h

@@ -239,28 +239,24 @@ class MEDIA_GPU_EXPORT V4L2VideoDecodeAccelerator
   // via AssignPictureBuffers() on decoder thread.
   void AssignPictureBuffersTask(const std::vector<PictureBuffer>& buffers);
 
-  // Use buffer backed by dmabuf file descriptors in |dmabuf_fds| for the
-  // OutputRecord associated with |picture_buffer_id|, taking ownership of the
-  // file descriptors. |stride| is the number of bytes from one row of pixels
-  // to the next row.
+  // Use buffer backed by |handle| for the OutputRecord associated with
+  // |picture_buffer_id|. |handle| does not need to be valid if we are in
+  // ALLOCATE mode and using an image processor.
   void ImportBufferForPictureTask(int32_t picture_buffer_id,
-                                  std::vector<base::ScopedFD>&& dmabuf_fds,
-                                  int32_t stride);
+                                  gfx::NativePixmapHandle handle);
 
-  // Check |planes| and |dmabuf_fds| are valid in import mode, besides
-  // ImportBufferForPicture.
+  // Check |handle| is valid in import mode, besides ImportBufferForPicture.
   void ImportBufferForPictureForImportTask(int32_t picture_buffer_id,
                                            VideoPixelFormat pixel_format,
                                            gfx::NativePixmapHandle handle);
 
   // Create an EGLImage for the buffer associated with V4L2 |buffer_index| and
-  // for |picture_buffer_id|, backed by dmabuf file descriptors in
-  // |passed_dmabuf_fds|, taking ownership of them.
+  // for |picture_buffer_id|, and backed by |handle|.
   // The buffer should be bound to |texture_id| and is of |size| and format
   // described by |fourcc|.
   void CreateEGLImageFor(size_t buffer_index,
                          int32_t picture_buffer_id,
-                         std::vector<base::ScopedFD>&& dmabuf_fds,
+                         gfx::NativePixmapHandle handle,
                          GLuint texture_id,
                          const gfx::Size& size,
                          const Fourcc fourcc);