Zero-copy: introduce a way to letterbox the BlitRequest results

Currently, CopyOutputRequest API enables its callers to obtain a copy
of the contents produced by a render pass. BlitRequest allows those
results to be placed in a specific region of caller-provided textures,
without modifying contents outside of that region. This is an
almost-ideal situation for FrameSinkVideoCapturerImpl, with an
exception that when it produces a VideoFrame, it may be forced to
letterbox part of the frame. To avoid doing this work in the capturer
(by memory-mapping the GpuMemoryBuffer), a letterbox region is
introduced to BlitRequest.

Changes:
- Introduce `BlitRequest::letterbox_region()` that specifies the
  region of the caller-provided textures outside of which everything
  will be filled with black.
- `FrameSinkVideoCaprturerImpl` takes advantage of newly introduced
  capability on `BlitRequest`. Letterboxing only happens in the
  capturer for non-GMB-backed VideoFrames, and only if there are parts
  of a frame that we have not already written to.
- `skia::BlitRGBAToYUVA()` now exposes additional parameter to allow
  letterboxing of the destination image.
- Test changes in SkiaReadbackPixeltest to account for new parameter
  to `BlitRequest`.
- Minor: `RenderableGpuMemoryBufferVideoFramePool` now tags
  GpuMemoryBuffers with the color space.
- Minor cleanup in
  `SkiaOutputSurfaceImplOnGpu::ImportSurfacesForNV12Planes()` - rename
  variables, add/expand comments, add DVLOG(3)s to facilitate
  future investigations.
- Minor cleanup in `FrameSinkVideoCapturerImpl` - comments, DVLOGs,
  helpers.

Bug: 1310411
Change-Id: I292ab78b9aabc29e23eb1489ea1ca8c9752b88eb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3617204
Reviewed-by: Jordan Bayles <jophba@chromium.org>
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Reviewed-by: Michael Ludwig <michaelludwig@google.com>
Commit-Queue: Piotr Bialecki <bialpio@chromium.org>
Reviewed-by: Kyle Charbonneau <kylechar@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1003391}

components/viz/common/frame_sinks/README.md

@@ -56,8 +56,8 @@ properties:
    Note that all coordinates are constrained to be integer values, to avoid
    introducing alignment, rounding or other "fuzz" issues.
 
- * Result format: An RGBA-interleaved bitmap (SkBitmap) or I420 Y+U+V image
-   planes.
+ * Result format: An RGBA-interleaved bitmap (SkBitmap), I420 Y+U+V image
+   planes, or NV12 Y+UV image planes.
 
 For efficient video capture, the above are used as follows: An issuer of
 CopyOutputRequests "locks into" a target area within the Surface (usually the

components/viz/common/frame_sinks/blit_request.cc

@@ -31,9 +31,11 @@ std::string BlendBitmap::ToString() const {
 
 BlitRequest::BlitRequest(
     const gfx::Point& destination_region_offset,
+    LetterboxingBehavior letterboxing_behavior,
     const std::array<gpu::MailboxHolder, CopyOutputResult::kMaxPlanes>&
         mailboxes)
     : destination_region_offset_(destination_region_offset),
+      letterboxing_behavior_(letterboxing_behavior),
       mailboxes_(mailboxes) {}
 
 BlitRequest::BlitRequest(BlitRequest&& other) = default;

components/viz/common/frame_sinks/blit_request.h

@@ -53,6 +53,16 @@ class VIZ_COMMON_EXPORT BlendBitmap {
   sk_sp<SkImage> image_;
 };
 
+// Enum used to specify letteboxing behavior for a BlitRequest.
+enum class LetterboxingBehavior {
+  // No letterboxing is needed - only the destination region will be written
+  // into by the handler of CopyOutputRequest.
+  kDoNotLetterbox,
+  // Letterboxing is needed - everything outside of the destination region
+  // will be filled with black by the handler of CopyOutputRequest.
+  kLetterbox
+};
+
 // Structure describing a blit operation that can be appended to
 // `CopyOutputRequest` if the callers want to place the results of the operation
 // in textures that they own.
@@ -60,6 +70,7 @@ class VIZ_COMMON_EXPORT BlitRequest {
  public:
   explicit BlitRequest(
       const gfx::Point& destination_region_offset,
+      LetterboxingBehavior letterboxing_behavior,
       const std::array<gpu::MailboxHolder, CopyOutputResult::kMaxPlanes>&
           mailboxes);
 
@@ -74,6 +85,10 @@ class VIZ_COMMON_EXPORT BlitRequest {
     return destination_region_offset_;
   }
 
+  LetterboxingBehavior letterboxing_behavior() const {
+    return letterboxing_behavior_;
+  }
+
   const std::array<gpu::MailboxHolder, CopyOutputResult::kMaxPlanes>&
   mailboxes() const {
     return mailboxes_;
@@ -106,6 +121,9 @@ class VIZ_COMMON_EXPORT BlitRequest {
   // images.
   gfx::Point destination_region_offset_;
 
+  // Specifies the letterboxing behavior of this request.
+  LetterboxingBehavior letterboxing_behavior_;
+
   // Mailboxes with planes that will be populated.
   // The textures can (but don't have to be) backed by
   // a GpuMemoryBuffer. The pixel format of the request determines

components/viz/common/frame_sinks/copy_output_request.cc

@@ -107,6 +107,7 @@ void CopyOutputRequest::set_blit_request(BlitRequest blit_request) {
   DCHECK(!blit_request_);
   DCHECK_EQ(result_destination(), ResultDestination::kNativeTextures);
   DCHECK_EQ(result_format(), ResultFormat::NV12_PLANES);
+  DCHECK(has_result_selection());
 
   // Destination region must start at an even offset for NV12 results:
   DCHECK_EQ(blit_request.destination_region_offset().x() % 2, 0);

components/viz/common/frame_sinks/copy_output_request.h

@@ -116,12 +116,14 @@ class VIZ_COMMON_EXPORT CopyOutputRequest {
   // Optionally specify that only a portion of the result be generated. The
   // selection rect will be clamped to the result bounds, which always starts at
   // 0,0 and spans the post-scaling size of the copy area (see set_area()
-  // above). Only RGBA format supports odd-sized result selection.
+  // above). Only RGBA format supports odd-sized result selection. Can only be
+  // called before blit request was set on the copy request.
   void set_result_selection(const gfx::Rect& selection) {
     DCHECK(result_format_ == ResultFormat::RGBA ||
            (selection.width() % 2 == 0 && selection.height() % 2 == 0))
         << "CopyOutputRequest supports odd-sized result_selection() only for "
            "RGBA!";
+    DCHECK(!has_blit_request());
     result_selection_ = selection;
   }
   bool has_result_selection() const { return result_selection_.has_value(); }
@@ -129,7 +131,15 @@ class VIZ_COMMON_EXPORT CopyOutputRequest {
 
   // Requests that the region copied by the CopyOutputRequest be blitted into
   // the caller's textures. Can be called only for CopyOutputRequests that
-  // target native textures.
+  // target native textures. Requires that result selection was set, in which
+  // case the caller's textures will be populated with the results of the
+  // copy request. The region in the caller's textures that will be populated
+  // is specified by `gfx::Rect(blit_request.destination_region_offset(),
+  // result_selection().size())`. If blit request is configured to perform
+  // letterboxing, all contents outside of that region will be overwritten with
+  // black, otherwise they will be unchanged. If the copy request's result would
+  // be smaller than `result_selection().size()`, the request will fail (i.e.
+  // empty result will be sent).
   void set_blit_request(BlitRequest blit_request);
   bool has_blit_request() const { return blit_request_.has_value(); }
   const BlitRequest& blit_request() const { return *blit_request_; }

components/viz/service/display/skia_readback_pixeltest.cc

@@ -17,6 +17,7 @@
 #include "cc/test/pixel_test.h"
 #include "cc/test/pixel_test_utils.h"
 #include "cc/test/resource_provider_test_utils.h"
+#include "components/viz/common/frame_sinks/blit_request.h"
 #include "components/viz/common/frame_sinks/copy_output_request.h"
 #include "components/viz/common/frame_sinks/copy_output_result.h"
 #include "components/viz/common/frame_sinks/copy_output_util.h"
@@ -572,7 +573,8 @@ GTEST_ALLOW_UNINSTANTIATED_PARAMETERIZED_TEST(SkiaReadbackPixelTestNV12);
 
 class SkiaReadbackPixelTestNV12WithBlit
     : public SkiaReadbackPixelTest,
-      public testing::WithParamInterface<bool> {
+      public testing::WithParamInterface<
+          std::tuple<bool, LetterboxingBehavior>> {
  public:
   CopyOutputResult::Destination RequestDestination() const {
     return CopyOutputResult::Destination::kNativeTextures;
@@ -583,7 +585,11 @@ class SkiaReadbackPixelTestNV12WithBlit
   }
 
   void SetUp() override {
-    SkiaReadbackPixelTest::SetUpReadbackPixeltest(GetParam());
+    SkiaReadbackPixelTest::SetUpReadbackPixeltest(std::get<0>(GetParam()));
+  }
+
+  LetterboxingBehavior GetLetterboxingBehavior() const {
+    return std::get<1>(GetParam());
   }
 };
 
@@ -625,8 +631,7 @@ TEST_P(SkiaReadbackPixelTestNV12WithBlit, ExecutesCopyRequestWithBlit) {
       << " The test case expects the blit region's origin to be even for NV12 "
          "blit requests";
 
-  const SkColor rgba_red = SkColorSetARGB(0xff, 0xff, 0, 0);
-  const SkColor yuv_red = GLScalerTestUtil::ConvertRGBAColorToYUV(rgba_red);
+  const SkColor yuv_red = GLScalerTestUtil::ConvertRGBAColorToYUV(SK_ColorRED);
 
   const std::vector<uint8_t> luma_pattern = {
       static_cast<uint8_t>(SkColorGetR(yuv_red))};
@@ -669,8 +674,9 @@ TEST_P(SkiaReadbackPixelTestNV12WithBlit, ExecutesCopyRequestWithBlit) {
 
         request.set_result_selection(result_selection);
 
-        request.set_blit_request(
-            BlitRequest(destination_subregion.origin(), mailboxes));
+        request.set_blit_request(BlitRequest(destination_subregion.origin(),
+                                             GetLetterboxingBehavior(),
+                                             mailboxes));
       }));
 
   // Check that a result was produced and is of the expected rect/size.
@@ -716,7 +722,18 @@ TEST_P(SkiaReadbackPixelTestNV12WithBlit, ExecutesCopyRequestWithBlit) {
   // The textures that we passed in to BlitRequest contained NV12 plane data for
   // an all-red image, let's re-create such a bitmap:
   SkBitmap expected = GLScalerTestUtil::AllocateRGBABitmap(source_size);
-  expected.eraseColor(rgba_red);
+
+  if (GetLetterboxingBehavior() == LetterboxingBehavior::kLetterbox) {
+    // We have requested the results to be letterboxed, so everything that
+    // CopyOutputRequest is not populating w/ render pass contents should be
+    // black:
+    expected.eraseColor(SK_ColorBLACK);
+  } else {
+    // We have requested the results to not be letterboxed, so everything that
+    // CopyOutputRequest is not populating w/ render pass will have original
+    // contents (red in our case):
+    expected.eraseColor(SK_ColorRED);
+  }
 
   // Blit request should "stitch" the pixels from the source image into a
   // sub-region of caller-provided texture - let's write our expected pixels
@@ -732,10 +749,12 @@ TEST_P(SkiaReadbackPixelTestNV12WithBlit, ExecutesCopyRequestWithBlit) {
 }
 
 #if !BUILDFLAG(IS_ANDROID) || !defined(ARCH_CPU_X86_FAMILY)
-INSTANTIATE_TEST_SUITE_P(,
-                         SkiaReadbackPixelTestNV12WithBlit,
-                         // Result scaling: Scale by half?
-                         testing::Values(true, false));
+INSTANTIATE_TEST_SUITE_P(
+    ,
+    SkiaReadbackPixelTestNV12WithBlit,
+    testing::Combine(testing::Bool(),  // Result scaling: Scale by half?
+                     testing::Values(LetterboxingBehavior::kDoNotLetterbox,
+                                     LetterboxingBehavior::kLetterbox)));
 #else
 // Don't instantiate the NV12 tests when run on Android emulator, they won't
 // work since the SkiaRenderer currently does not support CopyOutputRequests

components/viz/service/display_embedder/skia_output_surface_impl_on_gpu.cc

@@ -849,8 +849,8 @@ bool SkiaOutputSurfaceImplOnGpu::ImportSurfacesForNV12Planes(
   for (size_t i = 0; i < CopyOutputResult::kNV12MaxPlanes; ++i) {
     const gpu::MailboxHolder& mailbox_holder = blit_request.mailbox(i);
 
-    // Should never happen, maiboxes are validated when setting blit request on
-    // a CopyOutputResult.
+    // Should never happen, mailboxes are validated when setting blit request on
+    // a CopyOutputResult and we only access `kNV12MaxPlanes` mailboxes.
     DCHECK(!mailbox_holder.mailbox.IsZero());
 
     PlaneAccessData& plane_data = plane_access_datas[i];
@@ -912,6 +912,9 @@ void SkiaOutputSurfaceImplOnGpu::CopyOutputNV12(
              CopyOutputRequest::ResultDestination::kNativeTextures)
       << "Only CopyOutputRequests that hand out native textures support blit "
          "requests!";
+  DCHECK(!request->has_blit_request() || request->has_result_selection())
+      << "Only CopyOutputRequests that specify result selection support blit "
+         "requests!";
 
   // Overview:
   // 1. Try to create surfaces for NV12 planes (we know the needed size in
@@ -934,16 +937,29 @@ void SkiaOutputSurfaceImplOnGpu::CopyOutputNV12(
   // copied, as well as area, scaling & result_selection of the `request`.
   // This represents the size of the intermediate texture that will be then
   // blitted to the destination textures.
-  const gfx::Size destination_size = geometry.result_selection.size();
+  const gfx::Size intermediate_dst_size = geometry.result_selection.size();
 
   std::array<PlaneAccessData, CopyOutputResult::kNV12MaxPlanes>
       plane_access_datas;
 
   SkYUVAInfo yuva_info;
 
-  bool destination_surfaces_created = false;
+  bool destination_surfaces_ready = false;
   if (request->has_blit_request()) {
-    destination_surfaces_created = ImportSurfacesForNV12Planes(
+    if (request->result_selection().size() != intermediate_dst_size) {
+      DLOG(WARNING)
+          << __func__
+          << ": result selection is different than render pass output, "
+             "geometry="
+          << geometry.ToString() << ", request=" << request->ToString();
+      // Send empty result, we have a blit request that asks for a different
+      // size than what we have available - the behavior in this case is
+      // currently unspecified as we'd have to leave parts of the caller's
+      // region unpopulated.
+      return;
+    }
+
+    destination_surfaces_ready = ImportSurfacesForNV12Planes(
         request->blit_request(), plane_access_datas);
 
     // The entire destination image size is the same as the size of the luma
@@ -954,7 +970,8 @@ void SkiaOutputSurfaceImplOnGpu::CopyOutputNV12(
 
     // Check if the destination will fit in the blit target:
     const gfx::Rect blit_destination_rect(
-        request->blit_request().destination_region_offset(), destination_size);
+        request->blit_request().destination_region_offset(),
+        intermediate_dst_size);
     const gfx::Rect blit_target_image_rect(
         gfx::SkISizeToSize(plane_access_datas[0].size));
 
@@ -964,24 +981,25 @@ void SkiaOutputSurfaceImplOnGpu::CopyOutputNV12(
       return;
     }
   } else {
-    yuva_info = SkYUVAInfo(
-        gfx::SizeToSkISize(destination_size), SkYUVAInfo::PlaneConfig::kY_UV,
-        SkYUVAInfo::Subsampling::k420, kRec709_Limited_SkYUVColorSpace);
+    yuva_info = SkYUVAInfo(gfx::SizeToSkISize(intermediate_dst_size),
+                           SkYUVAInfo::PlaneConfig::kY_UV,
+                           SkYUVAInfo::Subsampling::k420,
+                           kRec709_Limited_SkYUVColorSpace);
 
-    destination_surfaces_created =
+    destination_surfaces_ready =
         CreateSurfacesForNV12Planes(yuva_info, color_space, plane_access_datas);
   }
 
-  if (!destination_surfaces_created) {
-    DVLOG(1) << "failed to create destination surfaces";
+  if (!destination_surfaces_ready) {
+    DVLOG(1) << "failed to create / import destination surfaces";
     // Send empty result.
     return;
   }
 
   // Create a destination for the scaled & clipped result:
-  auto representation = CreateSharedImageRepresentationSkia(
-      ResourceFormat::RGBA_8888, destination_size, color_space);
-  if (!representation) {
+  auto intermediate_representation = CreateSharedImageRepresentationSkia(
+      ResourceFormat::RGBA_8888, intermediate_dst_size, color_space);
+  if (!intermediate_representation) {
     DVLOG(1) << "failed to create shared image representation for the "
                 "intermediate surface";
     // Send empty result.
@@ -992,9 +1010,11 @@ void SkiaOutputSurfaceImplOnGpu::CopyOutputNV12(
   std::vector<GrBackendSemaphore> begin_semaphores;
   std::vector<GrBackendSemaphore> end_semaphores;
 
-  auto scoped_write = representation->BeginScopedWriteAccess(
-      /*final_msaa_count=*/1, surface_props, &begin_semaphores, &end_semaphores,
-      gpu::SharedImageRepresentation::AllowUnclearedAccess::kYes);
+  auto intermediate_scoped_write =
+      intermediate_representation->BeginScopedWriteAccess(
+          /*final_msaa_count=*/1, surface_props, &begin_semaphores,
+          &end_semaphores,
+          gpu::SharedImageRepresentation::AllowUnclearedAccess::kYes);
 
   absl::optional<SkVector> scaling;
   if (request->is_scaled()) {
@@ -1004,21 +1024,22 @@ void SkiaOutputSurfaceImplOnGpu::CopyOutputNV12(
                                  request->scale_from().y());
   }
 
-  scoped_write->surface()->wait(begin_semaphores.size(),
-                                begin_semaphores.data());
+  intermediate_scoped_write->surface()->wait(begin_semaphores.size(),
+                                             begin_semaphores.data());
 
   RenderSurface(surface, src_rect, scaling,
                 is_downscale_or_identity_in_both_dimensions,
-                scoped_write->surface());
+                intermediate_scoped_write->surface());
 
   if (request->has_blit_request()) {
-    BlendBitmapOverlays(scoped_write->surface()->getCanvas(),
+    BlendBitmapOverlays(intermediate_scoped_write->surface()->getCanvas(),
                         request->blit_request());
   }
 
-  auto source_image = scoped_write->surface()->makeImageSnapshot();
-  if (!source_image) {
-    DLOG(ERROR) << "failed to retrieve source_image.";
+  auto intermediate_image =
+      intermediate_scoped_write->surface()->makeImageSnapshot();
+  if (!intermediate_image) {
+    DLOG(ERROR) << "failed to retrieve `intermediate_image`.";
     return;
   }
 
@@ -1030,19 +1051,27 @@ void SkiaOutputSurfaceImplOnGpu::CopyOutputNV12(
       plane_access_datas[1].scoped_write->surface(), nullptr, nullptr};
 
   // The region to be populated in caller's textures is derived from blit
-  // request's |destination_region_offset|, and from COR's |result_selection|.
-  // If we have a blit request, use it. Otherwise, use an
+  // request's |destination_region_offset()|, and from COR's
+  // |result_selection()|. If we have a blit request, use it. Otherwise, use an
   // empty rect (which means that entire image will be used as the target of the
   // blit - this will not result in rescaling since w/o blit request present,
-  // the image size matches the |result_selection|).
-  SkRect dst_region =
+  // the destination image size matches the |geometry.result_selection|).
+  const SkRect dst_region =
       request->has_blit_request()
           ? gfx::RectToSkRect(
                 gfx::Rect(request->blit_request().destination_region_offset(),
-                          destination_size))
+                          intermediate_dst_size))
           : SkRect::MakeEmpty();
-  skia::BlitRGBAToYUVA(source_image.get(), plane_surfaces.data(), yuva_info,
-                       dst_region);
+
+  // We should clear destination if BlitRequest asked to letterbox everything
+  // outside of intended destination region:
+  const bool clear_destination =
+      request->has_blit_request()
+          ? request->blit_request().letterboxing_behavior() ==
+                LetterboxingBehavior::kLetterbox
+          : false;
+  skia::BlitRGBAToYUVA(intermediate_image.get(), plane_surfaces.data(),
+                       yuva_info, dst_region, clear_destination);
 
   bool should_submit = false;
 
@@ -1059,12 +1088,12 @@ void SkiaOutputSurfaceImplOnGpu::CopyOutputNV12(
     }
   }
 
-  representation->SetCleared();
+  intermediate_representation->SetCleared();
 
   should_submit |= !end_semaphores.empty();
 
-  if (!FlushSurface(scoped_write->surface(), end_semaphores,
-                    scoped_write->TakeEndState())) {
+  if (!FlushSurface(intermediate_scoped_write->surface(), end_semaphores,
+                    intermediate_scoped_write->TakeEndState())) {
     // TODO(penghuang): handle vulkan device lost.
     FailedSkiaFlush("CopyOutputNV12 dest_surface->flush()");
     return;

components/viz/service/frame_sinks/video_capture/frame_sink_video_capturer_impl.cc

@@ -37,6 +37,8 @@
 #include "mojo/public/cpp/bindings/self_owned_receiver.h"
 #include "third_party/abseil-cpp/absl/types/optional.h"
 #include "ui/gfx/color_space.h"
+#include "ui/gfx/geometry/point.h"
+#include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/geometry/size.h"
 
 using media::VideoCaptureOracle;
@@ -111,6 +113,46 @@ CopyOutputRequest::ResultFormat VideoPixelFormatToCopyOutputRequestFormat(
   }
 }
 
+bool IsCompatibleWithFormat(const gfx::Rect& rect,
+                            media::VideoPixelFormat format) {
+  DCHECK(format == media::PIXEL_FORMAT_I420 ||
+         format == media::PIXEL_FORMAT_NV12 ||
+         format == media::PIXEL_FORMAT_ARGB);
+  if (format == media::PIXEL_FORMAT_ARGB) {
+    // No special requirements:
+    return true;
+  }
+
+  return rect.origin().x() % 2 == 0 && rect.origin().y() % 2 == 0 &&
+         rect.width() % 2 == 0 && rect.height() % 2 == 0;
+}
+
+// Given a |visible_rect| representing visible rectangle of some video frame,
+// calculates a centered rectangle that fits entirely within |visible_rect| and
+// has the same aspect ratio as |source_size|, taking into account
+// |pixel_format|.
+gfx::Rect GetContentRectangle(const gfx::Rect& visible_rect,
+                              const gfx::Size& source_size,
+                              media::VideoPixelFormat pixel_format) {
+  DCHECK(pixel_format == media::PIXEL_FORMAT_I420 ||
+         pixel_format == media::PIXEL_FORMAT_NV12 ||
+         pixel_format == media::PIXEL_FORMAT_ARGB);
+
+  if (pixel_format == media::PIXEL_FORMAT_I420 ||
+      pixel_format == media::PIXEL_FORMAT_NV12) {
+    return media::ComputeLetterboxRegionForI420(visible_rect, source_size);
+  } else {
+    DCHECK_EQ(media::PIXEL_FORMAT_ARGB, pixel_format);
+    const gfx::Rect content_rect =
+        media::ComputeLetterboxRegion(visible_rect, source_size);
+
+    // The media letterboxing computation explicitly allows for off-by-one
+    // errors due to computation, so we address those here.
+    return content_rect.ApproximatelyEqual(visible_rect, 1) ? visible_rect
+                                                            : content_rect;
+  }
+}
+
 }  // namespace
 
 // static
@@ -308,6 +350,10 @@ void FrameSinkVideoCapturerImpl::SetResolutionConstraints(
     bool use_fixed_aspect_ratio) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
+  DVLOG(2) << __func__ << ": min_size=" << min_size.ToString()
+           << ", max_size=" << max_size.ToString()
+           << ", use_fixed_aspect_ratio=" << use_fixed_aspect_ratio;
+
   TRACE_EVENT_INSTANT2("gpu.capture", "SetResolutionConstraints",
                        TRACE_EVENT_SCOPE_THREAD, "min_size.width",
                        min_size.width(), "min_size.height", min_size.height());
@@ -695,6 +741,7 @@ void FrameSinkVideoCapturerImpl::MaybeCaptureFrame(
   // TODO(https://crbug.com/1300943): we should likely just get the frame
   // region from the last aggregated surface.
   if (!compositor_frame_region.Contains(capture_region)) {
+    DVLOG(3) << __func__ << ": skipping capture!";
     MaybeScheduleRefreshFrame();
     return;
   }
@@ -707,9 +754,23 @@ void FrameSinkVideoCapturerImpl::MaybeCaptureFrame(
 
   // Reserve a buffer from the pool for the next frame.
   const OracleFrameNumber oracle_frame_number = oracle_->next_frame_number();
+
+  // Size of the video frames that we are supposed to produce. Depends on the
+  // pixel format and the capture size as determined by the oracle (which in
+  // turn depends on the capture constraints).
   const gfx::Size capture_size =
       AdjustSizeForPixelFormat(oracle_->capture_size());
 
+  // Size of the source that we are capturing:
+  const gfx::Size source_size = oracle_->source_size();
+  DCHECK_EQ(capture_region.size(), source_size);
+  DCHECK(!source_size.IsEmpty());
+
+  DVLOG(3) << __func__
+           << ": compositor_frame_region=" << compositor_frame_region.ToString()
+           << ", capture_region=" << capture_region.ToString()
+           << ", capture_size=" << capture_size.ToString();
+
   const bool can_resurrect_content = CanResurrectFrame(capture_size);
   scoped_refptr<VideoFrame> frame;
   if (can_resurrect_content) {
@@ -757,6 +818,13 @@ void FrameSinkVideoCapturerImpl::MaybeCaptureFrame(
     return;
   }
 
+  // If frame was resurrected / allocated from the pool, its visible rectangle
+  // should match what we requested:
+  DCHECK_EQ(frame->visible_rect().size(), capture_size);
+  // The pool should return a frame with visible rectangle that is compatible
+  // with the capture format.
+  DCHECK(IsCompatibleWithFormat(frame->visible_rect(), pixel_format_));
+
   // Record a trace event if the capture pipeline is redlining, but capture will
   // still proceed.
   if (utilization >= 1.0) {
@@ -800,23 +868,24 @@ void FrameSinkVideoCapturerImpl::MaybeCaptureFrame(
                            "frame_number", capture_frame_number, "trigger",
                            VideoCaptureOracle::EventAsString(event));
 
-  const gfx::Size& source_size = oracle_->source_size();
-  DCHECK(!source_size.IsEmpty());
-  gfx::Rect content_rect;
-  if (pixel_format_ == media::PIXEL_FORMAT_I420 ||
-      pixel_format_ == media::PIXEL_FORMAT_NV12) {
-    content_rect = media::ComputeLetterboxRegionForI420(frame->visible_rect(),
-                                                        source_size);
-  } else {
-    DCHECK_EQ(media::PIXEL_FORMAT_ARGB, pixel_format_);
-    content_rect =
-        media::ComputeLetterboxRegion(frame->visible_rect(), source_size);
-    // The media letterboxing computation explicitly allows for off-by-one
-    // errors due to computation, so we address those here.
-    if (content_rect.ApproximatelyEqual(frame->visible_rect(), 1)) {
-      content_rect = frame->visible_rect();
-    }
-  }
+  // `content_rect` is the region of the `frame` that we would like to populate.
+  // We know our source is of size `source_size`, and we have
+  // `frame->visible_rect()` to fill out - find the largest centered rectangle
+  // that will fit within the frame and maintains the aspect ratio of the
+  // source.
+  // TODO(https://crbug.com/1323342): currently, both the frame's visible
+  // rectangle and source size are controlled by oracle
+  // (`frame->visible_rect().size() == `capture_size`). Oracle also knows if we
+  // need to maintain fixed aspect ratio, so it should compute both the
+  // `capture_size` and `content_rect` for us, thus ensuring that letterboxing
+  // happens only when it needs to (i.e. when we allocate a frame and know that
+  // aspect ratio does not have to be maintained, we should use a size that we
+  // know would not require letterboxing).
+  const gfx::Rect content_rect =
+      GetContentRectangle(frame->visible_rect(), source_size, pixel_format_);
+  DVLOG(3) << __func__ << ": content_rect=" << content_rect.ToString()
+           << ", source_size=" << source_size.ToString()
+           << ", frame=" << frame->AsHumanReadableString();
 
   // Determine what rectangular region has changed since the last captured
   // frame.
@@ -900,7 +969,6 @@ void FrameSinkVideoCapturerImpl::MaybeCaptureFrame(
     return;
   }
 
-  DCHECK(capture_region.size() == source_size);
   if (absl::holds_alternative<RegionCaptureCropId>(target_->sub_target)) {
     const float scale_factor = frame_metadata.device_scale_factor;
     metadata.region_capture_rect =
@@ -927,7 +995,12 @@ void FrameSinkVideoCapturerImpl::MaybeCaptureFrame(
     std::array<gpu::MailboxHolder, 3> mailbox_holders = {
         request_properties.frame->mailbox_holder(0),
         request_properties.frame->mailbox_holder(1), gpu::MailboxHolder{}};
-    blit_request = BlitRequest(content_rect.origin(), mailbox_holders);
+
+    // TODO(https://crbug.com/775740): change the capturer to only request the
+    // parts of the frame that have changed whenever possible.
+    blit_request =
+        BlitRequest(content_rect.origin(), LetterboxingBehavior::kLetterbox,
+                    mailbox_holders);
 
     // We haven't captured the frame yet, but let's pretend that we did for the
     // sake of blend information computation. We will be asking for an entire
@@ -1021,6 +1094,7 @@ void FrameSinkVideoCapturerImpl::DidCopyFrame(
 
   scoped_refptr<media::VideoFrame>& frame = properties.frame;
   const gfx::Rect& content_rect = properties.content_rect;
+
   if (log_to_webrtc_ && consumer_) {
     std::string format = "";
     std::string strides = "";
@@ -1131,6 +1205,13 @@ void FrameSinkVideoCapturerImpl::DidCopyFrame(
   }
 
   if (frame) {
+    // The result may be smaller than what was requested, if unforeseen
+    // clamping to the source boundaries occurred by the executor of the
+    // CopyOutputRequest. However, the result should never contain more than
+    // what was requested.
+    DCHECK_LE(result->size().width(), content_rect.width());
+    DCHECK_LE(result->size().height(), content_rect.height());
+
     if (!frame->HasGpuMemoryBuffer()) {
       // For GMB-backed video frames, overlays were already applied by
       // CopyOutputRequest API. For in-memory frames, apply overlays here:
@@ -1145,15 +1226,21 @@ void FrameSinkVideoCapturerImpl::DidCopyFrame(
       }
     }
 
-    // The result may be smaller than what was requested, if unforeseen
-    // clamping to the source boundaries occurred by the executor of the
-    // CopyOutputRequest. However, the result should never contain more than
-    // what was requested.
-    DCHECK_LE(result->size().width(), content_rect.width());
-    DCHECK_LE(result->size().height(), content_rect.height());
-    media::LetterboxVideoFrame(
-        frame.get(), gfx::Rect(content_rect.origin(),
-                               AdjustSizeForPixelFormat(result->size())));
+    const gfx::Rect result_rect =
+        gfx::Rect(content_rect.origin(), result->size());
+    DCHECK(IsCompatibleWithFormat(result_rect, pixel_format_));
+
+    DVLOG(3) << __func__ << ": result->size()=" << result->size().ToString()
+             << ", content_rect=" << content_rect.ToString()
+             << ", result_rect=" << result_rect.ToString()
+             << ", frame=" << frame->AsHumanReadableString();
+
+    if (frame->visible_rect() != result_rect && !frame->HasGpuMemoryBuffer()) {
+      // If there are parts of the frame that are visible but we have not wrote
+      // into them, letterbox them. This is not needed for GMB-backed frames as
+      // the letterboxing happens on GPU.
+      media::LetterboxVideoFrame(frame.get(), result_rect);
+    }
 
     if (ShouldMark(*frame, properties.content_version)) {
       MarkFrame(frame, properties.content_version);

components/viz/service/frame_sinks/video_capture/frame_sink_video_capturer_impl_unittest.cc

@@ -442,8 +442,9 @@ MATCHER_P2(IsLetterboxedFrame, color, content_rect, "") {
 
   const VideoFrame& frame = *arg;
   const gfx::Rect kContentRect = content_rect;
-  const auto IsLetterboxedPlane = [&frame, kContentRect](int plane,
-                                                         uint8_t component) {
+
+  const auto IsLetterboxedPlane = [&frame, kContentRect, result_listener](
+                                      int plane, uint8_t component) {
     gfx::Rect content_rect_copy = kContentRect;
     if (plane != VideoFrame::kYPlane) {
       content_rect_copy = gfx::Rect(
@@ -455,10 +456,19 @@ MATCHER_P2(IsLetterboxedFrame, color, content_rect, "") {
       for (int col = 0; col < frame.row_bytes(plane); ++col) {
         if (content_rect_copy.Contains(gfx::Point(col, row))) {
           if (p[col] != component) {
+            *result_listener << " where pixel at (" << col << ", " << row
+                             << ") should be inside content rectangle and the "
+                                "component should match 0x"
+                             << std::hex << component << " but is 0x"
+                             << std::hex << static_cast<unsigned int>(p[col]);
             return false;
           }
         } else {  // Letterbox border around content.
           if (plane == VideoFrame::kYPlane && p[col] != 0x00) {
+            *result_listener << " where pixel at (" << col << ", " << row
+                             << ") should be outside content rectangle and the "
+                                "component should match 0x00 but is 0x"
+                             << std::hex << static_cast<unsigned int>(p[col]);
             return false;
           }
         }

media/base/video_util.h

@@ -84,6 +84,9 @@ MEDIA_EXPORT gfx::Rect ComputeLetterboxRegion(const gfx::Rect& bounds,
 // have color distortions around the edges in a letterboxed video frame. Note
 // that, in cases where ComputeLetterboxRegion() would return a 1x1-sized Rect,
 // this function could return either a 0x0-sized Rect or a 2x2-sized Rect.
+// Note that calling this function with `bounds` that already have the aspect
+// ratio of `content` is not guaranteed to be a no-op (for context, see
+// https://crbug.com/1323367).
 MEDIA_EXPORT gfx::Rect ComputeLetterboxRegionForI420(const gfx::Rect& bounds,
                                                      const gfx::Size& content);
 

media/video/renderable_gpu_memory_buffer_video_frame_pool.cc

@@ -179,6 +179,8 @@ bool FrameResources::Initialize() {
     return false;
   }
 
+  gpu_memory_buffer_->SetColorSpace(color_space_);
+
   // Bind SharedImages to each plane.
   constexpr size_t kNumPlanes = 2;
   constexpr gfx::BufferPlane kPlanes[kNumPlanes] = {gfx::BufferPlane::Y,

skia/ext/rgba_to_yuva.cc

@@ -4,24 +4,45 @@
 
 #include "skia/ext/rgba_to_yuva.h"
 
+#include <array>
+
 #include "base/logging.h"
 #include "base/notreached.h"
+#include "third_party/skia/include/core/SkBlendMode.h"
 #include "third_party/skia/include/core/SkCanvas.h"
+#include "third_party/skia/include/core/SkClipOp.h"
+#include "third_party/skia/include/core/SkColor.h"
 #include "third_party/skia/include/core/SkColorFilter.h"
 #include "third_party/skia/include/core/SkPaint.h"
+#include "third_party/skia/include/core/SkRect.h"
 #include "third_party/skia/include/effects/SkColorMatrix.h"
 
 namespace skia {
 
+namespace {
+
+SkRect GetSubsampledRect(const SkRect& rect,
+                         const std::array<float, 2>& subsampling_factors) {
+  return SkRect::MakeXYWH(rect.x() * subsampling_factors[0],
+                          rect.y() * subsampling_factors[1],
+                          rect.width() * subsampling_factors[0],
+                          rect.height() * subsampling_factors[1]);
+}
+
+}  // namespace
+
 void BlitRGBAToYUVA(SkImage* src_image,
                     SkSurface* dst_surfaces[SkYUVAInfo::kMaxPlanes],
                     const SkYUVAInfo& dst_yuva_info,
-                    const SkRect& dst_region) {
+                    const SkRect& dst_region,
+                    bool clear_destination) {
+  // Rectangle representing the entire destination image:
+  const SkRect dst_image_rect = SkRect::Make(dst_yuva_info.dimensions());
   const SkRect src_rect = SkRect::Make(src_image->bounds());
-  const SkRect dst_rect =
-      dst_region.isEmpty()
-          ? SkRect::MakeSize(SkSize::Make(dst_yuva_info.dimensions()))
-          : dst_region;
+  // Region of destination image that is supposed to be populated:
+  const SkRect dst_rect = dst_region.isEmpty() ? dst_image_rect : dst_region;
+
+  DCHECK(dst_image_rect.contains(dst_rect));
 
   // Permutation matrices to select the appropriate YUVA channels for each
   // output plane.
@@ -50,6 +71,8 @@ void BlitRGBAToYUVA(SkImage* src_image,
 
   // Blit each plane.
   for (int plane = 0; plane < dst_yuva_info.numPlanes(); ++plane) {
+    SkCanvas* plane_canvas = dst_surfaces[plane]->getCanvas();
+
     SkColorMatrix color_matrix = rgb_to_yuv_matrix;
     color_matrix.postConcat(permutation_matrices[plane]);
 
@@ -67,21 +90,29 @@ void BlitRGBAToYUVA(SkImage* src_image,
     // Subsampling factors are determined by the ratios of the entire image's
     // width & height to the dimensions of the passed in surfaces (which should
     // also span the entire logical image):
-    float subsampling_factors[2] = {
+    std::array<float, 2> subsampling_factors = {
         static_cast<float>(dst_surfaces[plane]->width()) /
             dst_yuva_info.dimensions().width(),
         static_cast<float>(dst_surfaces[plane]->height()) /
             dst_yuva_info.dimensions().height(),
     };
-    SkRect plane_dst_rect =
-        SkRect::MakeXYWH(dst_rect.x() * subsampling_factors[0],
-                         dst_rect.y() * subsampling_factors[1],
-                         dst_rect.width() * subsampling_factors[0],
-                         dst_rect.height() * subsampling_factors[1]);
 
-    dst_surfaces[plane]->getCanvas()->drawImageRect(
-        src_image, src_rect, plane_dst_rect, sampling_options, &paint,
-        SkCanvas::kFast_SrcRectConstraint);
+    if (clear_destination && dst_image_rect != dst_rect) {
+      // If we were told to clear the destination prior to blitting and we know
+      // the blit won't populate the entire destination image, issue the draw
+      // call that fills the destination with black and takes into account the
+      // color conversion needed.
+      SkPaint clear_paint(paint);
+      clear_paint.setColor(SK_ColorBLACK);
+
+      plane_canvas->drawPaint(clear_paint);
+    }
+
+    const SkRect plane_dst_rect =
+        GetSubsampledRect(dst_rect, subsampling_factors);
+    plane_canvas->drawImageRect(src_image, src_rect, plane_dst_rect,
+                                sampling_options, &paint,
+                                SkCanvas::kFast_SrcRectConstraint);
   }
 }
 

skia/ext/rgba_to_yuva.h

@@ -6,6 +6,7 @@
 #define SKIA_EXT_RGBA_TO_YUVA_H_
 
 #include "third_party/skia/include/core/SkImage.h"
+#include "third_party/skia/include/core/SkRect.h"
 #include "third_party/skia/include/core/SkSurface.h"
 #include "third_party/skia/include/core/SkYUVAInfo.h"
 
@@ -16,11 +17,13 @@ namespace skia {
 // `dst_yuva_info`. `dst_yuva_info` describes the entire destination image - the
 // results of the blit operation will be placed in its subregion, described by
 // `dst_region`. If a default-constructed `dst_region` is passed in, the entire
-// destination image will be written to.
+// destination image will be written to. If `clear_destination` is true, the
+// entire destination image will be cleared with black before the blit.
 SK_API void BlitRGBAToYUVA(SkImage* src_image,
                            SkSurface* dst_surfaces[SkYUVAInfo::kMaxPlanes],
                            const SkYUVAInfo& dst_yuva_info,
-                           const SkRect& dst_region = SkRect::MakeEmpty());
+                           const SkRect& dst_region = SkRect::MakeEmpty(),
+                           bool clear_destination = false);
 
 }  // namespace skia