Commit fd8bbf20 authored by Brian Ho's avatar Brian Ho Committed by Commit Bot

viz: Respect color space for SkiaRenderer copies

When fulfulling RGBA_BITMAP copy requests, SkiaRenderer does not
pass a color space to asyncRescaleAndReadPixels which according to
the Skia documentation [1], means that Skia will default to a sRGB
conversion. However, when the color space isn't sRGB (e.g.
HDR videos), the non-sRGB color space is still passed via
ReadPixelsContext to initialize the SkBitmap [2]. Thus, even though
the image was converted to sRGB in the read, the SkBitmap reports
that it is in a non-sRGB color space.

This CL fixes this issue by explicitly providing a SkColorSpace to
asyncRescaleAndReadPixels via a conversion from gfx::ColorSpace.
Additionally, if the gfx::ColorSpace cannot be converted to an
SkColorSpace (e.g. PIECEWISE_HDR), fallback to sRGB. This fallback
fixes a null pointer dereference in Chrome OS [3], and for more
context, see the CL for a similar fix in GLRenderer [4].

[1] https://source.chromium.org/chromium/chromium/src/+/master:third_party/skia/include/core/SkImageInfo.h;l=296;drc=a450ab794839b63f9c2ba982f738945f90e9fee2
[2] https://source.chromium.org/chromium/chromium/src/+/master:components/viz/service/display_embedder/skia_output_surface_impl_on_gpu.cc;l=214;drc=a450ab794839b63f9c2ba982f738945f90e9fee2
[3] https://source.chromium.org/chromium/chromium/src/+/master:components/viz/service/frame_sinks/video_capture/frame_sink_video_capturer_impl.cc;l=753;drc=e0789c0a2326279a3bac70012ca3edd3e46b5aa1
[4] https://chromium-review.googlesource.com/c/chromium/src/+/2464100

Bug: 1138518
Change-Id: I4ccd5b6e9af75caff917fa851281eae7ee8f10cc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2472437Reviewed-by: default avatarJonathan Backer <backer@chromium.org>
Commit-Queue: Brian Ho <hob@chromium.org>
Cr-Commit-Position: refs/heads/master@{#817140}
parent 6f0d6777
...@@ -813,14 +813,22 @@ bool SkiaOutputSurfaceImplOnGpu::CopyOutput( ...@@ -813,14 +813,22 @@ bool SkiaOutputSurfaceImplOnGpu::CopyOutput(
CopyOutputRequest::ResultFormat::RGBA_BITMAP) { CopyOutputRequest::ResultFormat::RGBA_BITMAP) {
// Perform swizzle during readback. // Perform swizzle during readback.
const bool skbitmap_is_bgra = (kN32_SkColorType == kBGRA_8888_SkColorType); const bool skbitmap_is_bgra = (kN32_SkColorType == kBGRA_8888_SkColorType);
// If we can't convert |color_space| to a SkColorSpace
// (e.g. PIECEWISE_HDR), request a sRGB destination color space for the
// copy result instead.
gfx::ColorSpace dest_color_space = color_space;
sk_sp<SkColorSpace> sk_color_space = color_space.ToSkColorSpace();
if (!sk_color_space) {
dest_color_space = gfx::ColorSpace::CreateSRGB();
}
SkImageInfo dst_info = SkImageInfo::Make( SkImageInfo dst_info = SkImageInfo::Make(
geometry.result_selection.width(), geometry.result_selection.height(), geometry.result_selection.width(), geometry.result_selection.height(),
skbitmap_is_bgra ? kBGRA_8888_SkColorType : kRGBA_8888_SkColorType, skbitmap_is_bgra ? kBGRA_8888_SkColorType : kRGBA_8888_SkColorType,
kPremul_SkAlphaType); kPremul_SkAlphaType, sk_color_space);
std::unique_ptr<ReadPixelsContext> context = std::unique_ptr<ReadPixelsContext> context =
std::make_unique<ReadPixelsContext>(std::move(request), std::make_unique<ReadPixelsContext>(std::move(request),
geometry.result_selection, geometry.result_selection,
color_space, weak_ptr_); dest_color_space, weak_ptr_);
// Skia readback could be synchronous. Incremement counter in case // Skia readback could be synchronous. Incremement counter in case
// ReadbackCompleted is called immediately. // ReadbackCompleted is called immediately.
num_readbacks_pending_++; num_readbacks_pending_++;
......
...@@ -194,4 +194,80 @@ TEST_F(SkiaOutputSurfaceImplTest, SupportsColorSpaceChange) { ...@@ -194,4 +194,80 @@ TEST_F(SkiaOutputSurfaceImplTest, SupportsColorSpaceChange) {
} }
} }
// Tests that the destination color space is preserved across a CopyOutput for
// ColorSpaces supported by SkColorSpace.
TEST_F(SkiaOutputSurfaceImplTest, CopyOutputBitmapSupportedColorSpace) {
output_surface_->Reshape(kSurfaceRect.size(), 1, gfx::ColorSpace(),
gfx::BufferFormat::RGBX_8888, /*use_stencil=*/false);
constexpr gfx::Rect output_rect(0, 0, 10, 10);
const gfx::ColorSpace color_space = gfx::ColorSpace(
gfx::ColorSpace::PrimaryID::BT709, gfx::ColorSpace::TransferID::LINEAR);
base::RunLoop run_loop;
std::unique_ptr<CopyOutputResult> result;
auto request = std::make_unique<CopyOutputRequest>(
CopyOutputRequest::ResultFormat::RGBA_BITMAP,
base::BindOnce(
[](std::unique_ptr<CopyOutputResult>* result_out,
base::OnceClosure quit_closure,
std::unique_ptr<CopyOutputResult> tmp_result) {
*result_out = std::move(tmp_result);
std::move(quit_closure).Run();
},
&result, run_loop.QuitClosure()));
request->set_result_task_runner(
TestGpuServiceHolder::GetInstance()->gpu_thread_task_runner());
copy_output::RenderPassGeometry geometry;
geometry.result_bounds = output_rect;
geometry.result_selection = output_rect;
geometry.sampling_bounds = output_rect;
geometry.readback_offset = gfx::Vector2d(0, 0);
PaintRootRenderPass(kSurfaceRect, base::DoNothing::Once());
output_surface_->CopyOutput(AggregatedRenderPassId{0}, geometry, color_space,
std::move(request));
output_surface_->SwapBuffersSkipped();
run_loop.Run();
EXPECT_EQ(color_space, result->GetRGBAColorSpace());
}
// Tests that copying from a source with a color space that can't be converted
// to a SkColorSpace will fallback to a transform to sRGB.
TEST_F(SkiaOutputSurfaceImplTest, CopyOutputBitmapUnsupportedColorSpace) {
output_surface_->Reshape(kSurfaceRect.size(), 1, gfx::ColorSpace(),
gfx::BufferFormat::RGBX_8888, /*use_stencil=*/false);
constexpr gfx::Rect output_rect(0, 0, 10, 10);
const gfx::ColorSpace color_space = gfx::ColorSpace::CreatePiecewiseHDR(
gfx::ColorSpace::PrimaryID::BT2020, 0.5, 1.5);
base::RunLoop run_loop;
std::unique_ptr<CopyOutputResult> result;
auto request = std::make_unique<CopyOutputRequest>(
CopyOutputRequest::ResultFormat::RGBA_BITMAP,
base::BindOnce(
[](std::unique_ptr<CopyOutputResult>* result_out,
base::OnceClosure quit_closure,
std::unique_ptr<CopyOutputResult> tmp_result) {
*result_out = std::move(tmp_result);
std::move(quit_closure).Run();
},
&result, run_loop.QuitClosure()));
request->set_result_task_runner(
TestGpuServiceHolder::GetInstance()->gpu_thread_task_runner());
copy_output::RenderPassGeometry geometry;
geometry.result_bounds = output_rect;
geometry.result_selection = output_rect;
geometry.sampling_bounds = output_rect;
geometry.readback_offset = gfx::Vector2d(0, 0);
PaintRootRenderPass(kSurfaceRect, base::DoNothing::Once());
output_surface_->CopyOutput(AggregatedRenderPassId{0}, geometry, color_space,
std::move(request));
output_surface_->SwapBuffersSkipped();
run_loop.Run();
EXPECT_EQ(gfx::ColorSpace::CreateSRGB(), result->GetRGBAColorSpace());
}
} // namespace viz } // namespace viz
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment