Set the color space to sRGB on the Surface and remove colorFilter.
Also for a canvas wrapping a bitmap the colorspace of the bitmap
will be used to correctly blend content.
Test: CtsUiRenderingTestCases
Bug: 111436479
Change-Id: I63ad7a30605a7f725cc0ef4716d42ea978fb03e3
diff --git a/libs/hwui/Readback.cpp b/libs/hwui/Readback.cpp
index 80f2b57..2a48837 100644
--- a/libs/hwui/Readback.cpp
+++ b/libs/hwui/Readback.cpp
@@ -66,13 +66,10 @@
sk_sp<SkColorSpace> colorSpace =
DataSpaceToColorSpace(static_cast<android_dataspace>(surface.getBuffersDataSpace()));
- sk_sp<SkColorFilter> colorSpaceFilter;
- if (colorSpace && !colorSpace->isSRGB()) {
- colorSpaceFilter = SkToSRGBColorFilter::Make(colorSpace);
- }
sk_sp<SkImage> image = SkImage::MakeFromAHardwareBuffer(
- reinterpret_cast<AHardwareBuffer*>(sourceBuffer.get()), kPremul_SkAlphaType);
- return copyImageInto(image, colorSpaceFilter, texTransform, srcRect, bitmap);
+ reinterpret_cast<AHardwareBuffer*>(sourceBuffer.get()),
+ kPremul_SkAlphaType, colorSpace);
+ return copyImageInto(image, texTransform, srcRect, bitmap);
}
CopyResult Readback::copyHWBitmapInto(Bitmap* hwBitmap, SkBitmap* bitmap) {
@@ -83,20 +80,7 @@
transform.loadScale(1, -1, 1);
transform.translate(0, -1);
- // TODO: Try to take and reuse the image inside HW bitmap with "hwBitmap->makeImage".
- // TODO: When this was attempted, it resulted in instability.
- sk_sp<SkColorFilter> colorSpaceFilter;
- sk_sp<SkColorSpace> colorSpace = hwBitmap->info().refColorSpace();
- if (colorSpace && !colorSpace->isSRGB()) {
- colorSpaceFilter = SkToSRGBColorFilter::Make(colorSpace);
- }
- sk_sp<SkImage> image = SkImage::MakeFromAHardwareBuffer(
- reinterpret_cast<AHardwareBuffer*>(hwBitmap->graphicBuffer()), kPremul_SkAlphaType);
-
- // HW Bitmap currently can only attach to a GraphicBuffer with PIXEL_FORMAT_RGBA_8888 format
- // and SRGB color space. ImageDecoder can create a new HW Bitmap with non-SRGB color space: for
- // example see android.graphics.cts.BitmapColorSpaceTest#testEncodeP3hardware test.
- return copyImageInto(image, colorSpaceFilter, transform, srcRect, bitmap);
+ return copyImageInto(hwBitmap->makeImage(), transform, srcRect, bitmap);
}
CopyResult Readback::copyLayerInto(DeferredLayerUpdater* deferredLayer, SkBitmap* bitmap) {
@@ -118,8 +102,7 @@
return copyResult;
}
-CopyResult Readback::copyImageInto(const sk_sp<SkImage>& image,
- sk_sp<SkColorFilter>& colorSpaceFilter, Matrix4& texTransform,
+CopyResult Readback::copyImageInto(const sk_sp<SkImage>& image, Matrix4& texTransform,
const Rect& srcRect, SkBitmap* bitmap) {
if (Properties::getRenderPipelineType() == RenderPipelineType::SkiaGL) {
mRenderThread.requireGlContext();
@@ -157,11 +140,7 @@
return copyResult;
}
- // See Readback::copyLayerInto for an overview of color space conversion.
- // HW Bitmap are allowed to be in a non-SRGB color space (for example coming from ImageDecoder).
- // For Surface and HW Bitmap readback flows we pass colorSpaceFilter, which does the conversion.
- // TextureView readback is using Layer::setDataSpace, which creates a SkColorFilter internally.
- Layer layer(mRenderThread.renderState(), colorSpaceFilter, 255, SkBlendMode::kSrc);
+ Layer layer(mRenderThread.renderState(), nullptr, 255, SkBlendMode::kSrc);
bool disableFilter = MathUtils::areEqual(skiaSrcRect.width(), skiaDestRect.width()) &&
MathUtils::areEqual(skiaSrcRect.height(), skiaDestRect.height());
layer.setForceFilter(!disableFilter);
@@ -177,38 +156,6 @@
bool Readback::copyLayerInto(Layer* layer, const SkRect* srcRect, const SkRect* dstRect,
SkBitmap* bitmap) {
- /*
- * In the past only TextureView readback was setting the temporary surface color space to null.
- * Now all 3 readback flows are drawing into a SkSurface with null color space.
- * At readback there are 3 options to convert the source image color space to the destination
- * color space requested in "bitmap->info().colorSpace()":
- * 1. Set color space for temporary surface render target to null (disables color management),
- * colorspace tag from source SkImage is ignored by Skia,
- * convert SkImage to SRGB at draw time with SkColorFilter/SkToSRGBColorFilter,
- * do a readback from temporary SkSurface to a temporary SRGB SkBitmap "bitmap2",
- * read back from SRGB "bitmap2" into non-SRGB "bitmap" which will do a CPU color conversion.
- *
- * 2. Set color space for temporary surface render target to SRGB (not nullptr),
- * colorspace tag on the source SkImage is used by Skia to enable conversion,
- * convert SkImage to SRGB at draw time with drawImage (no filters),
- * do a readback from temporary SkSurface, which will do a color conversion from SRGB to
- * bitmap->info().colorSpace() on the CPU.
- *
- * 3. Set color space for temporary surface render target to bitmap->info().colorSpace(),
- * colorspace tag on the source SkImage is used by Skia to enable conversion,
- * convert SkImage to bitmap->info().colorSpace() at draw time with drawImage (no filters),
- * do a readback from SkSurface, which will not do any color conversion, because
- * surface was created with the same color space as the "bitmap".
- *
- * Option 1 is used for all readback flows.
- * Options 2 and 3 are new, because skia added support for non-SRGB render targets without
- * linear blending.
- * TODO: evaluate if options 2 or 3 for color space conversion are better.
- */
-
- // drop the colorSpace from the temporary surface.
- SkImageInfo surfaceInfo = bitmap->info().makeColorSpace(nullptr);
-
/* This intermediate surface is present to work around a bug in SwiftShader that
* prevents us from reading the contents of the layer's texture directly. The
* workaround involves first rendering that texture into an intermediate buffer and
@@ -217,70 +164,44 @@
* with reading incorrect data from EGLImage backed SkImage (likely a driver bug).
*/
sk_sp<SkSurface> tmpSurface = SkSurface::MakeRenderTarget(mRenderThread.getGrContext(),
- SkBudgeted::kYes, surfaceInfo);
+ SkBudgeted::kYes, bitmap->info());
+ // if we can't generate a GPU surface that matches the destination bitmap (e.g. 565) then we
+ // attempt to do the intermediate rendering step in 8888
if (!tmpSurface.get()) {
- surfaceInfo = surfaceInfo.makeColorType(SkColorType::kN32_SkColorType);
+ SkImageInfo tmpInfo = bitmap->info().makeColorType(SkColorType::kN32_SkColorType);
tmpSurface = SkSurface::MakeRenderTarget(mRenderThread.getGrContext(), SkBudgeted::kYes,
- surfaceInfo);
+ tmpInfo);
if (!tmpSurface.get()) {
- ALOGW("Unable to readback GPU contents into the provided bitmap");
+ ALOGW("Unable to generate GPU buffer in a format compatible with the provided bitmap");
return false;
}
}
- if (skiapipeline::LayerDrawable::DrawLayer(mRenderThread.getGrContext(),
- tmpSurface->getCanvas(), layer, srcRect, dstRect,
- false)) {
- // If bitmap->info().colorSpace() is non-SRGB, convert the data from SRGB to non-SRGB on
- // CPU. We can't just pass bitmap->info() to SkSurface::readPixels, because "tmpSurface" has
- // disabled color conversion.
- SkColorSpace* destColorSpace = bitmap->info().colorSpace();
- SkBitmap tempSRGBBitmap;
- SkBitmap tmpN32Bitmap;
- SkBitmap* bitmapInSRGB;
- if (destColorSpace && !destColorSpace->isSRGB()) {
- tempSRGBBitmap.allocPixels(bitmap->info().makeColorSpace(SkColorSpace::MakeSRGB()));
- bitmapInSRGB = &tempSRGBBitmap; // Need to convert latter from SRGB to non-SRGB.
- } else {
- bitmapInSRGB = bitmap; // No need for color conversion - write directly into output.
- }
- bool success = false;
+ if (!skiapipeline::LayerDrawable::DrawLayer(mRenderThread.getGrContext(),
+ tmpSurface->getCanvas(), layer, srcRect, dstRect,
+ false)) {
+ ALOGW("Unable to draw content from GPU into the provided bitmap");
+ return false;
+ }
- // TODO: does any of the readbacks below clamp F16 exSRGB?
- // Readback into a SRGB SkBitmap.
- if (tmpSurface->readPixels(bitmapInSRGB->info(), bitmapInSRGB->getPixels(),
- bitmapInSRGB->rowBytes(), 0, 0)) {
- success = true;
- } else {
- // if we fail to readback from the GPU directly (e.g. 565) then we attempt to read into
- // 8888 and then convert that into the destination format before giving up.
- SkImageInfo bitmapInfo =
- SkImageInfo::MakeN32(bitmap->width(), bitmap->height(), bitmap->alphaType(),
- SkColorSpace::MakeSRGB());
- if (tmpN32Bitmap.tryAllocPixels(bitmapInfo) &&
- tmpSurface->readPixels(bitmapInfo, tmpN32Bitmap.getPixels(),
- tmpN32Bitmap.rowBytes(), 0, 0)) {
- success = true;
- bitmapInSRGB = &tmpN32Bitmap;
- }
- }
-
- if (success) {
- if (bitmapInSRGB != bitmap) {
- // Convert from SRGB to non-SRGB color space if needed. Convert from N32 to
- // destination bitmap color format if needed.
- if (!bitmapInSRGB->readPixels(bitmap->info(), bitmap->getPixels(),
- bitmap->rowBytes(), 0, 0)) {
- return false;
- }
- }
- bitmap->notifyPixelsChanged();
- return true;
+ if (!tmpSurface->readPixels(*bitmap, 0, 0)) {
+ // if we fail to readback from the GPU directly (e.g. 565) then we attempt to read into
+ // 8888 and then convert that into the destination format before giving up.
+ SkBitmap tmpBitmap;
+ SkImageInfo tmpInfo = bitmap->info().makeColorType(SkColorType::kN32_SkColorType);
+ if (bitmap->info().colorType() == SkColorType::kN32_SkColorType ||
+ !tmpBitmap.tryAllocPixels(tmpInfo) ||
+ !tmpSurface->readPixels(tmpBitmap, 0, 0) ||
+ !tmpBitmap.readPixels(bitmap->info(), bitmap->getPixels(),
+ bitmap->rowBytes(), 0, 0)) {
+ ALOGW("Unable to convert content into the provided bitmap");
+ return false;
}
}
- return false;
+ bitmap->notifyPixelsChanged();
+ return true;
}
} /* namespace uirenderer */