Support result_selection with Skia readback

Add support for CopyOutputRequest::result_selection when using
SkiaRenderer with the Skia readback API. Previously the field was
ignored in SkiaOutputSurfaceImplOnGpu. The skia readback API doesn't
support specifying a clip rect in the destination pixel space, so this
is a bit of a workaround with the potential for some rounding errors.

This CL also adds SkiaReadbackPixelTest. It draw a frame containing a
source image with a CopyOutputRequest. The pixels that are read back are
compared against reference PNGs. The test is based on
GLRendererCopierPixelTest and uses the same source and result PNGs.

Bug: 1047217
Change-Id: I9f97cacafff15833d6ce97d36c69c70b55d09479
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2033624
Commit-Queue: kylechar <kylechar@chromium.org>
Reviewed-by: Jonathan Backer <backer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#739127}

components/viz/service/BUILD.gn

@@ -438,6 +438,7 @@ viz_source_set("unit_tests") {
     "display/layer_quad_unittest.cc",
     "display/renderer_pixeltest.cc",
     "display/shader_unittest.cc",
+    "display/skia_readback_pixeltest.cc",
     "display/software_renderer_unittest.cc",
     "display/surface_aggregator_pixeltest.cc",
     "display/surface_aggregator_unittest.cc",

components/viz/service/display/skia_readback_pixeltest.cc

+// Copyright 2020 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.
+
+#include <memory>
+#include <tuple>
+
+#include "base/bind.h"
+#include "base/command_line.h"
+#include "base/containers/span.h"
+#include "base/files/file_path.h"
+#include "base/path_service.h"
+#include "base/run_loop.h"
+#include "build/build_config.h"
+#include "cc/base/switches.h"
+#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/copy_output_request.h"
+#include "components/viz/common/frame_sinks/copy_output_result.h"
+#include "components/viz/common/frame_sinks/copy_output_util.h"
+#include "components/viz/service/display_embedder/skia_output_surface_impl.h"
+#include "components/viz/test/paths.h"
+#include "gpu/command_buffer/client/shared_image_interface.h"
+#include "gpu/command_buffer/common/shared_image_usage.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/skia/include/core/SkBitmap.h"
+#include "third_party/skia/include/core/SkImageInfo.h"
+#include "ui/gfx/color_space.h"
+#include "ui/gfx/geometry/rect.h"
+#include "ui/gfx/geometry/size.h"
+
+namespace viz {
+namespace {
+
+// The size of the source texture or framebuffer.
+constexpr gfx::Size kSourceSize = gfx::Size(240, 120);
+
+base::FilePath GetTestFilePath(const base::FilePath::CharType* basename) {
+  base::FilePath test_dir;
+  base::PathService::Get(Paths::DIR_TEST_DATA, &test_dir);
+  return test_dir.Append(base::FilePath(basename));
+}
+
+SharedQuadState* CreateSharedQuadState(RenderPass* render_pass,
+                                       const gfx::Rect& rect) {
+  const gfx::Rect layer_rect = rect;
+  const gfx::Rect visible_layer_rect = rect;
+  const gfx::Rect clip_rect = rect;
+  SharedQuadState* shared_state = render_pass->CreateAndAppendSharedQuadState();
+  shared_state->SetAll(gfx::Transform(), layer_rect, visible_layer_rect,
+                       gfx::RRectF(), clip_rect, /*is_clipped=*/false,
+                       /*are_contents_opaque=*/false, /*opacity=*/1.0f,
+                       SkBlendMode::kSrcOver,
+                       /*sorting_context_id=*/0);
+  return shared_state;
+}
+
+base::span<const uint8_t> MakePixelSpan(const SkBitmap& bitmap) {
+  return base::make_span(static_cast<const uint8_t*>(bitmap.getPixels()),
+                         bitmap.computeByteSize());
+}
+
+void DeleteSharedImage(scoped_refptr<ContextProvider> context_provider,
+                       gpu::Mailbox mailbox,
+                       const gpu::SyncToken& sync_token,
+                       bool is_lost) {
+  DCHECK(context_provider);
+  gpu::SharedImageInterface* sii = context_provider->SharedImageInterface();
+  DCHECK(sii);
+  sii->DestroySharedImage(sync_token, mailbox);
+}
+
+}  // namespace
+
+class SkiaReadbackPixelTest : public cc::PixelTest,
+                              public testing::WithParamInterface<bool> {
+ public:
+  // TODO(kylechar): Add parameter to also test RGBA_TEXTURE when it's
+  // supported with the Skia readback API.
+  CopyOutputResult::Format RequestFormat() const {
+    return CopyOutputResult::Format::RGBA_BITMAP;
+  }
+  bool ScaleByHalf() const { return GetParam(); }
+
+  void SetUp() override {
+    SetUpSkiaRenderer(false);
+
+    ASSERT_TRUE(cc::ReadPNGFile(
+        GetTestFilePath(FILE_PATH_LITERAL("16_color_rects.png")),
+        &source_bitmap_));
+    source_bitmap_.setImmutable();
+  }
+
+  // Returns filepath for expected output PNG.
+  base::FilePath GetExpectedPath() const {
+    return GetTestFilePath(
+        ScaleByHalf() ? FILE_PATH_LITERAL("half_of_one_of_16_color_rects.png")
+                      : FILE_PATH_LITERAL("one_of_16_color_rects.png"));
+  }
+
+  // TODO(kylechar): Create an OOP-R style GPU resource with no GL dependencies.
+  ResourceId CreateGpuResource(const gfx::Size& size,
+                               ResourceFormat format,
+                               base::span<const uint8_t> pixels) {
+    gpu::SharedImageInterface* sii =
+        child_context_provider_->SharedImageInterface();
+    DCHECK(sii);
+    gpu::Mailbox mailbox =
+        sii->CreateSharedImage(format, size, gfx::ColorSpace(),
+                               gpu::SHARED_IMAGE_USAGE_DISPLAY, pixels);
+    gpu::SyncToken sync_token = sii->GenUnverifiedSyncToken();
+
+    TransferableResource gl_resource = TransferableResource::MakeGL(
+        mailbox, GL_LINEAR, GL_TEXTURE_2D, sync_token, size,
+        /*is_overlay_candidate=*/false);
+    gl_resource.format = format;
+    auto release_callback = SingleReleaseCallback::Create(
+        base::BindOnce(&DeleteSharedImage, child_context_provider_, mailbox));
+    return child_resource_provider_->ImportResource(
+        gl_resource, std::move(release_callback));
+  }
+
+  // Creates a RenderPass that embeds a single quad containing |source_bitmap_|.
+  std::unique_ptr<RenderPass> GenerateRootRenderPass() {
+    ResourceFormat format =
+        (source_bitmap_.info().colorType() == kBGRA_8888_SkColorType)
+            ? ResourceFormat::BGRA_8888
+            : ResourceFormat::RGBA_8888;
+    ResourceId resource_id =
+        CreateGpuResource(kSourceSize, format, MakePixelSpan(source_bitmap_));
+
+    std::unordered_map<ResourceId, ResourceId> resource_map =
+        cc::SendResourceAndGetChildToParentMap(
+            {resource_id}, this->resource_provider_.get(),
+            this->child_resource_provider_.get(),
+            this->child_context_provider_.get());
+    ResourceId mapped_resource_id = resource_map[resource_id];
+
+    const gfx::Rect output_rect(kSourceSize);
+    std::unique_ptr<RenderPass> pass = RenderPass::Create();
+    pass->SetNew(/*render_pass_id=*/1, output_rect, output_rect,
+                 gfx::Transform());
+
+    SharedQuadState* sqs = CreateSharedQuadState(pass.get(), output_rect);
+
+    auto* quad = pass->CreateAndAppendDrawQuad<TileDrawQuad>();
+    quad->SetNew(sqs, output_rect, output_rect, /*needs_blending=*/false,
+                 mapped_resource_id, gfx::RectF(output_rect), kSourceSize,
+                 /*contents_premultiplied=*/true, /*nearest_neighbor=*/true,
+                 /*force_anti_aliasing_off=*/false);
+    return pass;
+  }
+
+ protected:
+  SkBitmap source_bitmap_;
+};
+
+// Test that SkiaRenderer readback works correctly. This test will use the
+// default readback implementation for the platform, which is either the legacy
+// GLRendererCopier or the new Skia readback API.
+TEST_P(SkiaReadbackPixelTest, ExecutesCopyRequest) {
+  // Generates a RenderPass which contains one quad that spans the full output.
+  // The quad has our source image, so the framebuffer should contain the source
+  // image after DrawFrame().
+  std::unique_ptr<RenderPass> pass = GenerateRootRenderPass();
+
+  std::unique_ptr<CopyOutputResult> result;
+  base::RunLoop loop;
+  auto request = std::make_unique<CopyOutputRequest>(
+      RequestFormat(),
+      base::BindOnce(
+          [](std::unique_ptr<CopyOutputResult>* result_out,
+             base::OnceClosure quit_closure,
+             std::unique_ptr<CopyOutputResult> result_from_copier) {
+            *result_out = std::move(result_from_copier);
+            std::move(quit_closure).Run();
+          },
+          &result, loop.QuitClosure()));
+
+  // In order to test coordinate calculations the tests will issue copy requests
+  // for a small region just to the right and below the center of the entire
+  // source texture/framebuffer.
+  gfx::Rect result_selection(kSourceSize.width() / 2, kSourceSize.height() / 2,
+                             kSourceSize.width() / 4, kSourceSize.height() / 4);
+
+  // Build CopyOutputRequest based on test parameters.
+  if (ScaleByHalf()) {
+    result_selection =
+        gfx::ScaleToEnclosingRect(gfx::Rect(result_selection), 0.5f);
+    request->SetUniformScaleRatio(2, 1);
+  }
+  request->set_result_selection(result_selection);
+  pass->copy_requests.push_back(std::move(request));
+
+  RenderPassList pass_list;
+  pass_list.push_back(std::move(pass));
+
+  renderer_->DecideRenderPassAllocationsForFrame(pass_list);
+  renderer_->DrawFrame(&pass_list, 1.0f, kSourceSize);
+
+  loop.Run();
+
+  // Check that a result was produced and is of the expected rect/size.
+  ASSERT_TRUE(result);
+  ASSERT_FALSE(result->IsEmpty());
+  EXPECT_EQ(result_selection, result->rect());
+
+  // Examine the image in the |result|, and compare it to the baseline PNG file.
+  const SkBitmap actual = result->AsSkBitmap();
+
+  base::FilePath expected_path = GetExpectedPath();
+  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
+          cc::switches::kCCRebaselinePixeltests)) {
+    EXPECT_TRUE(cc::WritePNGFile(actual, expected_path, false));
+  }
+
+  if (!cc::MatchesPNGFile(actual, expected_path,
+                          cc::ExactPixelComparator(false))) {
+    LOG(ERROR) << "Entire source: " << cc::GetPNGDataUrl(source_bitmap_);
+    ADD_FAILURE();
+  }
+}
+
+INSTANTIATE_TEST_SUITE_P(All,
+                         SkiaReadbackPixelTest,
+                         // Result scaling: Scale by half?
+                         testing::Values(true, false));
+
+}  // namespace viz

components/viz/service/display_embedder/skia_output_surface_impl_on_gpu.cc

@@ -1155,14 +1155,12 @@ void SkiaOutputSurfaceImplOnGpu::CopyOutput(
       base::BindOnce([](std::vector<std::unique_ptr<SkDeferredDisplayList>>) {},
                      std::move(destroy_after_swap_)));
 
-  bool use_gl_renderer_copier = !is_using_vulkan() && !is_using_dawn() &&
-                                !features::IsUsingSkiaForGLReadback();
-  if (use_gl_renderer_copier)
+  if (use_gl_renderer_copier_)
     gpu::ContextUrl::SetActiveUrl(copier_active_url_);
 
   // Lazy initialize GLRendererCopier before draw because
   // DirectContextProvider ctor the backbuffer.
-  if (use_gl_renderer_copier && !copier_) {
+  if (use_gl_renderer_copier_ && !copier_) {
     if (!MakeCurrent(true /* need_fbo0 */))
       return;
     auto client = std::make_unique<DirectContextProviderDelegateImpl>(
@@ -1228,7 +1226,7 @@ void SkiaOutputSurfaceImplOnGpu::CopyOutput(
     surface->flush();
   }
 
-  if (use_gl_renderer_copier) {
+  if (use_gl_renderer_copier_) {
     surface->flush();
 
     GLuint gl_id = 0;
@@ -1279,18 +1277,40 @@ void SkiaOutputSurfaceImplOnGpu::CopyOutput(
   SkFilterQuality filter_quality = is_downscale_in_both_dimensions
                                        ? kMedium_SkFilterQuality
                                        : kHigh_SkFilterQuality;
-  SkIRect src_rect = SkIRect::MakeXYWH(
-      geometry.sampling_bounds.x(), geometry.sampling_bounds.y(),
-      geometry.sampling_bounds.width(), geometry.sampling_bounds.height());
+
+  // Compute |source_selection| as a workaround to support |result_selection|
+  // with Skia readback. |result_selection| is a clip rect specified in the
+  // destination pixel space. By transforming |result_selection| back to the
+  // source pixel space we can compute what rectangle to sample from.
+  //
+  // This might introduce some rounding error if destination pixel space is
+  // scaled up from the source pixel space. When scaling |result_selection| back
+  // down it might not be pixel aligned.
+  gfx::Rect source_selection = geometry.sampling_bounds;
+  if (request->has_result_selection()) {
+    gfx::Rect sampling_selection = request->result_selection();
+    if (request->is_scaled()) {
+      // Invert the scaling.
+      sampling_selection = copy_output::ComputeResultRect(
+          sampling_selection, request->scale_to(), request->scale_from());
+    }
+    sampling_selection.Offset(source_selection.OffsetFromOrigin());
+    source_selection.Intersect(sampling_selection);
+  }
+
+  SkIRect src_rect =
+      SkIRect::MakeXYWH(source_selection.x(), source_selection.y(),
+                        source_selection.width(), source_selection.height());
 
   if (request->result_format() ==
       CopyOutputRequest::ResultFormat::I420_PLANES) {
     std::unique_ptr<ReadPixelsContext> context =
-        std::make_unique<ReadPixelsContext>(
-            std::move(request), geometry.result_bounds, color_space, weak_ptr_);
+        std::make_unique<ReadPixelsContext>(std::move(request),
+                                            geometry.result_selection,
+                                            color_space, weak_ptr_);
     surface->asyncRescaleAndReadPixelsYUV420(
         kRec709_SkYUVColorSpace, SkColorSpace::MakeSRGB(), src_rect,
-        {geometry.result_bounds.width(), geometry.result_bounds.height()},
+        {geometry.result_selection.width(), geometry.result_selection.height()},
         SkSurface::RescaleGamma::kSrc, filter_quality, &OnYUVReadbackDone,
         context.release());
   } else if (request->result_format() ==
@@ -1298,12 +1318,13 @@ void SkiaOutputSurfaceImplOnGpu::CopyOutput(
     // Perform swizzle during readback.
     const bool skbitmap_is_bgra = (kN32_SkColorType == kBGRA_8888_SkColorType);
     SkImageInfo dst_info = SkImageInfo::Make(
-        geometry.result_bounds.width(), geometry.result_bounds.height(),
+        geometry.result_selection.width(), geometry.result_selection.height(),
         skbitmap_is_bgra ? kBGRA_8888_SkColorType : kRGBA_8888_SkColorType,
         kPremul_SkAlphaType);
     std::unique_ptr<ReadPixelsContext> context =
-        std::make_unique<ReadPixelsContext>(
-            std::move(request), geometry.result_bounds, color_space, weak_ptr_);
+        std::make_unique<ReadPixelsContext>(std::move(request),
+                                            geometry.result_selection,
+                                            color_space, weak_ptr_);
     surface->asyncRescaleAndReadPixels(
         dst_info, src_rect, SkSurface::RescaleGamma::kSrc, filter_quality,
         &OnRGBAReadbackDone, context.release());
@@ -1454,6 +1475,8 @@ bool SkiaOutputSurfaceImplOnGpu::Initialize() {
     if (!InitializeForGL())
       return false;
   }
+  use_gl_renderer_copier_ = !is_using_vulkan() && !is_using_dawn() &&
+                            !features::IsUsingSkiaForGLReadback();
   max_resource_cache_bytes_ =
       context_state_->gr_context()->getResourceCacheLimit();
   return true;

components/viz/service/display_embedder/skia_output_surface_impl_on_gpu.h

@@ -294,6 +294,7 @@ class SkiaOutputSurfaceImplOnGpu : public gpu::ImageTransportSurfaceDelegate,
   BufferPresentedCallback buffer_presented_callback_;
   ContextLostCallback context_lost_callback_;
   GpuVSyncCallback gpu_vsync_callback_;
+  bool use_gl_renderer_copier_ = false;
 
 #if defined(USE_OZONE)
   // This should outlive gl_surface_ and vulkan_surface_.