media/CrOS VD: Create DRM framebuffers using the usable area.

Problem:

Videos that are decoded using the new direct VD and that get promoted to
a hardware overlay are displayed with non-visible data. This is because
we currently use the coded size to create the DRM framebuffer.

Solution:

This CL simply makes it so that the DRM framebuffer is created using the
"usable area." For most videos, this is simply the size of the visible
rectangle. However, some H.264 videos specify a visible rectangle that
doesn't start at (0, 0). For these videos, the usable area needs to
include everything from (0, 0) to the bottom-right corner of the visible
rectangle: the client is expected to setup the hardware overlay's crop
rectangle in order to exclude the non-visible area on the left and on
the top of the visible area (this seems to be currently broken though).

This change also means that we need to update the way we detect whether
the frames in the pool have to be re-allocated: if the DRM framebuffer
will change after a configuration change, the frames should be
re-allocated. The unit tests are expanded to more extensively cover this
re-allocation logic.

Bug: b:162880731
Test: video.Contents.h264_360p_hw on nami
Change-Id: I6ed36b9685329fe5f675503c2a9a0b580d6ae696
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2340115
Commit-Queue: Andres Calderon Jaramillo <andrescj@chromium.org>
Reviewed-by: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#799510}

media/gpu/chromeos/platform_video_frame_pool.cc

@@ -9,6 +9,7 @@
 #include "base/logging.h"
 #include "base/optional.h"
 #include "base/task/post_task.h"
+#include "media/base/video_util.h"
 #include "media/gpu/chromeos/gpu_buffer_layout.h"
 #include "media/gpu/chromeos/platform_video_frame_utils.h"
 #include "media/gpu/macros.h"
@@ -76,12 +77,19 @@ scoped_refptr<VideoFrame> PlatformVideoFramePool::GetFrame() {
     if (GetTotalNumFrames_Locked() >= max_num_frames_)
       return nullptr;
 
-    // VideoFrame::WrapVideoFrame() will check whether the updated visible_rect
-    // is sub rect of the original visible_rect. Therefore we set visible_rect
-    // as large as coded_size to guarantee this condition.
-    scoped_refptr<VideoFrame> new_frame = create_frame_cb_.Run(
-        gpu_memory_buffer_factory_, format, coded_size, gfx::Rect(coded_size),
-        coded_size, base::TimeDelta());
+    // We want to be able to re-use |new_frame| if possible even when the pool
+    // is re-initialized with a different |visible_rect_|. DRM framebuffers can
+    // be re-used if the visible-rect-from-the-origin (a.k.a "usable area") is
+    // the same. For example, say we have a |visible_rect_| of (10, 20), 640x360
+    // (DRM framebuffer of size 650x380); if we re-initialize the pool with
+    // (5, 2), 645x378, we can re-use the resources, but if we re-initialize
+    // with (10, 20), 100x100 we cannot (even though it's contained in the
+    // former). Hence the use of GetRectSizeFromOrigin() to calculate the
+    // visible rect for |new_frame|.
+    scoped_refptr<VideoFrame> new_frame =
+        create_frame_cb_.Run(gpu_memory_buffer_factory_, format, coded_size,
+                             gfx::Rect(GetRectSizeFromOrigin(visible_rect_)),
+                             coded_size, base::TimeDelta());
     if (!new_frame)
       return nullptr;
 
@@ -118,10 +126,6 @@ base::Optional<GpuBufferLayout> PlatformVideoFramePool::Initialize(
   DVLOGF(4);
   base::AutoLock auto_lock(lock_);
 
-  visible_rect_ = visible_rect;
-  natural_size_ = natural_size;
-  max_num_frames_ = max_num_frames;
-
   // Only support the Fourcc that could map to VideoPixelFormat.
   VideoPixelFormat format = fourcc.ToVideoPixelFormat();
   if (format == PIXEL_FORMAT_UNKNOWN) {
@@ -130,11 +134,18 @@ base::Optional<GpuBufferLayout> PlatformVideoFramePool::Initialize(
   }
 
   // If the frame layout changed we need to allocate new frames so we will clear
-  // the pool here. If only the visible or natural size changed we don't need to
-  // allocate new frames, but will just update the properties of wrapped frames
-  // returned by GetFrame().
-  // NOTE: It is assumed layout is determined by |format| and |coded_size|.
-  if (!IsSameFormat_Locked(format, coded_size)) {
+  // the pool here. If only the visible rect or natural size changed, we don't
+  // need to allocate new frames (unless the change in the visible rect causes a
+  // change in the size of the DRM framebuffer, see note below): we'll just
+  // update the properties of wrapped frames returned by GetFrame().
+  //
+  // NOTE: It is assumed layout is determined by |format|, |coded_size|, and
+  // possibly |visible_rect|. The reason for the "possibly" is that
+  // |visible_rect| is used to compute the size of the DRM framebuffer for
+  // hardware overlay purposes. The caveat is that different visible rectangles
+  // can map to the same framebuffer size, i.e., all the visible rectangles with
+  // the same bottom-right corner map to the same framebuffer size.
+  if (!IsSameFormat_Locked(format, coded_size, visible_rect)) {
     DVLOGF(4) << "The video frame format is changed. Clearing the pool.";
     free_frames_.clear();
 
@@ -142,7 +153,7 @@ base::Optional<GpuBufferLayout> PlatformVideoFramePool::Initialize(
     // VideoFrame that will be allocated in GetFrame() has.
     auto frame =
         create_frame_cb_.Run(gpu_memory_buffer_factory_, format, coded_size,
-                             visible_rect_, natural_size_, base::TimeDelta());
+                             visible_rect, natural_size, base::TimeDelta());
     if (!frame) {
       VLOGF(1) << "Failed to create video frame " << format << " (fourcc "
                << fourcc.ToString() << ")";
@@ -152,6 +163,10 @@ base::Optional<GpuBufferLayout> PlatformVideoFramePool::Initialize(
                                             frame->layout().planes());
   }
 
+  visible_rect_ = visible_rect;
+  natural_size_ = natural_size;
+  max_num_frames_ = max_num_frames;
+
   // The pool might become available because of |max_num_frames_| increased.
   // Notify the client if so.
   if (frame_available_cb_ && !IsExhausted_Locked())
@@ -219,8 +234,10 @@ void PlatformVideoFramePool::OnFrameReleased(
   DCHECK(it != frames_in_use_.end());
   frames_in_use_.erase(it);
 
-  if (IsSameFormat_Locked(origin_frame->format(), origin_frame->coded_size()))
+  if (IsSameFormat_Locked(origin_frame->format(), origin_frame->coded_size(),
+                          origin_frame->visible_rect())) {
     InsertFreeFrame_Locked(std::move(origin_frame));
+  }
 
   if (frame_available_cb_ && !IsExhausted_Locked())
     std::move(frame_available_cb_).Run();
@@ -245,13 +262,16 @@ size_t PlatformVideoFramePool::GetTotalNumFrames_Locked() const {
 
 bool PlatformVideoFramePool::IsSameFormat_Locked(
     VideoPixelFormat format,
-    const gfx::Size& coded_size) const {
+    const gfx::Size& coded_size,
+    const gfx::Rect& visible_rect) const {
   DVLOGF(4);
   lock_.AssertAcquired();
 
   return frame_layout_ &&
          frame_layout_->fourcc().ToVideoPixelFormat() == format &&
-         frame_layout_->size() == coded_size;
+         frame_layout_->size() == coded_size &&
+         GetRectSizeFromOrigin(visible_rect_) ==
+             GetRectSizeFromOrigin(visible_rect);
 }
 
 size_t PlatformVideoFramePool::GetPoolSizeForTesting() {

media/gpu/chromeos/platform_video_frame_pool.h

@@ -86,7 +86,8 @@ class MEDIA_GPU_EXPORT PlatformVideoFramePool : public DmabufVideoFramePool {
       EXCLUSIVE_LOCKS_REQUIRED(lock_);
   size_t GetTotalNumFrames_Locked() const EXCLUSIVE_LOCKS_REQUIRED(lock_);
   bool IsSameFormat_Locked(VideoPixelFormat format,
-                           const gfx::Size& coded_size) const
+                           const gfx::Size& coded_size,
+                           const gfx::Rect& visible_rect) const
       EXCLUSIVE_LOCKS_REQUIRED(lock_);
   bool IsExhausted_Locked() EXCLUSIVE_LOCKS_REQUIRED(lock_);
 

media/gpu/chromeos/platform_video_frame_pool_unittest.cc

@@ -14,6 +14,7 @@
 #include "base/threading/thread_task_runner_handle.h"
 #include "gpu/command_buffer/common/mailbox_holder.h"
 #include "media/base/format_utils.h"
+#include "media/base/video_util.h"
 #include "media/gpu/chromeos/fourcc.h"
 #include "media/video/fake_gpu_memory_buffer.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -53,12 +54,17 @@ class PlatformVideoFramePoolTest
 
   bool Initialize(const Fourcc& fourcc) {
     constexpr gfx::Size kCodedSize(320, 240);
-    constexpr size_t kNumFrames = 10;
-
-    visible_rect_.set_size(kCodedSize);
-    natural_size_ = kCodedSize;
+    return Initialize(kCodedSize, /*visible_rect=*/gfx::Rect(kCodedSize),
+                      fourcc);
+  }
 
-    layout_ = pool_->Initialize(fourcc, kCodedSize, visible_rect_,
+  bool Initialize(const gfx::Size& coded_size,
+                  const gfx::Rect& visible_rect,
+                  const Fourcc& fourcc) {
+    constexpr size_t kNumFrames = 10;
+    visible_rect_ = visible_rect;
+    natural_size_ = visible_rect.size();
+    layout_ = pool_->Initialize(fourcc, coded_size, visible_rect_,
                                 natural_size_, kNumFrames);
     return !!layout_;
   }
@@ -164,6 +170,64 @@ TEST_P(PlatformVideoFramePoolTest, InitializeWithDifferentFourcc) {
   EXPECT_EQ(0u, pool_->GetPoolSizeForTesting());
 }
 
+TEST_P(PlatformVideoFramePoolTest, InitializeWithDifferentUsableArea) {
+  const auto fourcc = Fourcc::FromVideoPixelFormat(GetParam());
+  ASSERT_TRUE(fourcc.has_value());
+
+  constexpr gfx::Size kCodedSize(640, 368);
+  constexpr gfx::Rect kInitialVisibleRect(10, 20, 300, 200);
+  ASSERT_TRUE(Initialize(kCodedSize, kInitialVisibleRect, fourcc.value()));
+  scoped_refptr<VideoFrame> frame_a = GetFrame(10);
+  scoped_refptr<VideoFrame> frame_b = GetFrame(10);
+
+  // Clear frame references to return the frames to the pool.
+  frame_a = nullptr;
+  frame_b = nullptr;
+  task_environment_.RunUntilIdle();
+
+  // Verify that both frames are in the pool.
+  EXPECT_EQ(2u, pool_->GetPoolSizeForTesting());
+
+  // Verify that requesting a frame with a different "usable area" causes the
+  // pool to get drained. The usable area is the area of the buffer starting at
+  // (0, 0) and extending to the bottom-right corner of the visible rectangle.
+  // It corresponds to the area used to import the video frame into a graphics
+  // API or to create the DRM framebuffer for hardware overlay purposes.
+  constexpr gfx::Rect kDifferentVisibleRect(5, 15, 300, 200);
+  ASSERT_EQ(kDifferentVisibleRect.size(), natural_size_);
+  ASSERT_NE(GetRectSizeFromOrigin(kInitialVisibleRect),
+            GetRectSizeFromOrigin(kDifferentVisibleRect));
+  ASSERT_TRUE(Initialize(kCodedSize, kDifferentVisibleRect, fourcc.value()));
+  scoped_refptr<VideoFrame> new_frame = GetFrame(10);
+  EXPECT_EQ(0u, pool_->GetPoolSizeForTesting());
+}
+
+TEST_P(PlatformVideoFramePoolTest, InitializeWithDifferentCodedSize) {
+  const auto fourcc = Fourcc::FromVideoPixelFormat(GetParam());
+  ASSERT_TRUE(fourcc.has_value());
+
+  constexpr gfx::Size kInitialCodedSize(640, 368);
+  constexpr gfx::Rect kVisibleRect(10, 20, 300, 200);
+  ASSERT_TRUE(Initialize(kInitialCodedSize, kVisibleRect, fourcc.value()));
+  scoped_refptr<VideoFrame> frame_a = GetFrame(10);
+  scoped_refptr<VideoFrame> frame_b = GetFrame(10);
+
+  // Clear frame references to return the frames to the pool.
+  frame_a = nullptr;
+  frame_b = nullptr;
+  task_environment_.RunUntilIdle();
+
+  // Verify that both frames are in the pool.
+  EXPECT_EQ(2u, pool_->GetPoolSizeForTesting());
+
+  // Verify that requesting a frame with a different coded size causes the pool
+  // to get drained.
+  constexpr gfx::Size kDifferentCodedSize(624, 368);
+  ASSERT_TRUE(Initialize(kDifferentCodedSize, kVisibleRect, fourcc.value()));
+  scoped_refptr<VideoFrame> new_frame = GetFrame(10);
+  EXPECT_EQ(0u, pool_->GetPoolSizeForTesting());
+}
+
 TEST_P(PlatformVideoFramePoolTest, UnwrapVideoFrame) {
   const auto fourcc = Fourcc::FromVideoPixelFormat(GetParam());
   ASSERT_TRUE(fourcc.has_value());
@@ -180,10 +244,14 @@ TEST_P(PlatformVideoFramePoolTest, UnwrapVideoFrame) {
   EXPECT_NE(frame_1->GetGpuMemoryBuffer(), frame_3->GetGpuMemoryBuffer());
 }
 
-TEST_P(PlatformVideoFramePoolTest, InitializeWithSameFourcc) {
+TEST_P(PlatformVideoFramePoolTest,
+       InitializeWithSameFourccUsableAreaAndCodedSize) {
   const auto fourcc = Fourcc::FromVideoPixelFormat(GetParam());
   ASSERT_TRUE(fourcc.has_value());
-  ASSERT_TRUE(Initialize(fourcc.value()));
+
+  constexpr gfx::Size kCodedSize(640, 368);
+  constexpr gfx::Rect kInitialVisibleRect(kCodedSize);
+  ASSERT_TRUE(Initialize(kCodedSize, kInitialVisibleRect, fourcc.value()));
   scoped_refptr<VideoFrame> frame1 = GetFrame(10);
   gfx::GpuMemoryBufferId id1 =
       PlatformVideoFramePool::GetGpuMemoryBufferId(*frame1);
@@ -192,8 +260,17 @@ TEST_P(PlatformVideoFramePoolTest, InitializeWithSameFourcc) {
   frame1 = nullptr;
   task_environment_.RunUntilIdle();
 
-  // Request frame with the same format. The pool should not request new frames.
-  ASSERT_TRUE(Initialize(fourcc.value()));
+  // Request frame with the same format, coded size, and "usable area." To make
+  // things interesting, we change the visible rect while keeping the
+  // "usable area" constant. The usable area is the area of the buffer starting
+  // at (0, 0) and extending to the bottom-right corner of the visible
+  // rectangle. It corresponds to the area used to import the video frame into a
+  // graphics API or to create the DRM framebuffer for hardware overlay
+  // purposes. The pool should not request new frames.
+  constexpr gfx::Rect kDifferentVisibleRect(10, 20, 630, 348);
+  ASSERT_EQ(GetRectSizeFromOrigin(kInitialVisibleRect),
+            GetRectSizeFromOrigin(kDifferentVisibleRect));
+  ASSERT_TRUE(Initialize(kCodedSize, kDifferentVisibleRect, fourcc.value()));
 
   scoped_refptr<VideoFrame> frame2 = GetFrame(20);
   gfx::GpuMemoryBufferId id2 =

media/gpu/chromeos/platform_video_frame_utils.cc

@@ -20,6 +20,7 @@
 #include "media/base/format_utils.h"
 #include "media/base/scopedfd_helper.h"
 #include "media/base/video_frame_layout.h"
+#include "media/base/video_util.h"
 #include "media/gpu/buffer_validation.h"
 #include "media/gpu/macros.h"
 #include "ui/gfx/gpu_memory_buffer.h"
@@ -33,6 +34,7 @@ gfx::GpuMemoryBufferHandle AllocateGpuMemoryBufferHandle(
     gpu::GpuMemoryBufferFactory* factory,
     VideoPixelFormat pixel_format,
     const gfx::Size& coded_size,
+    const gfx::Rect& visible_rect,
     gfx::BufferUsage buffer_usage) {
   DCHECK(factory);
   gfx::GpuMemoryBufferHandle gmb_handle;
@@ -52,8 +54,9 @@ gfx::GpuMemoryBufferHandle AllocateGpuMemoryBufferHandle(
   // TODO(hiroh): Rename the client id to more generic one.
   gmb_handle = factory->CreateGpuMemoryBuffer(
       gfx::GpuMemoryBufferId(gpu_memory_buffer_id), coded_size,
-      /*framebuffer_size=*/coded_size, *buffer_format, buffer_usage,
-      gpu::kPlatformVideoFramePoolClientId, gfx::kNullAcceleratedWidget);
+      /*framebuffer_size=*/GetRectSizeFromOrigin(visible_rect), *buffer_format,
+      buffer_usage, gpu::kPlatformVideoFramePoolClientId,
+      gfx::kNullAcceleratedWidget);
   DCHECK(gmb_handle.is_null() || gmb_handle.type != gfx::NATIVE_PIXMAP ||
          VideoFrame::NumPlanes(pixel_format) ==
              gmb_handle.native_pixmap_handle.planes.size());
@@ -71,8 +74,8 @@ scoped_refptr<VideoFrame> CreateGpuMemoryBufferVideoFrame(
     base::TimeDelta timestamp,
     gfx::BufferUsage buffer_usage) {
   DCHECK(factory);
-  auto gmb_handle = AllocateGpuMemoryBufferHandle(factory, pixel_format,
-                                                  coded_size, buffer_usage);
+  auto gmb_handle = AllocateGpuMemoryBufferHandle(
+      factory, pixel_format, coded_size, visible_rect, buffer_usage);
   if (gmb_handle.is_null())
     return nullptr;
 
@@ -113,8 +116,8 @@ scoped_refptr<VideoFrame> CreatePlatformVideoFrame(
     base::TimeDelta timestamp,
     gfx::BufferUsage buffer_usage) {
   DCHECK(factory);
-  auto gmb_handle = AllocateGpuMemoryBufferHandle(factory, pixel_format,
-                                                  coded_size, buffer_usage);
+  auto gmb_handle = AllocateGpuMemoryBufferHandle(
+      factory, pixel_format, coded_size, visible_rect, buffer_usage);
   if (gmb_handle.is_null())
     return nullptr;