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

1 files changed, 33 insertions, 112 deletions

diff --git a/libs/hwui/Readback.cpp b/libs/hwui/Readback.cpp
index 80f2b5714659..2a488378e3a8 100644
--- a/libs/hwui/Readback.cpp
+++ b/libs/hwui/Readback.cpp
@@ -66,13 +66,10 @@ CopyResult Readback::copySurfaceInto(Surface& surface, const Rect& srcRect, SkBi
 
     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 @@ CopyResult Readback::copyHWBitmapInto(Bitmap* hwBitmap, SkBitmap* bitmap) {
     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 @@ CopyResult Readback::copyLayerInto(DeferredLayerUpdater* deferredLayer, SkBitmap
     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 @@ CopyResult Readback::copyImageInto(const sk_sp<SkImage>& image,
         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 @@ CopyResult Readback::copyImageInto(const sk_sp<SkImage>& image,
 
 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 @@ bool Readback::copyLayerInto(Layer* layer, const SkRect* srcRect, const SkRect*
      * 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;
-
-        // 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 (!skiapipeline::LayerDrawable::DrawLayer(mRenderThread.getGrContext(),
+                                                tmpSurface->getCanvas(), layer, srcRect, dstRect,
+                                                false)) {
+        ALOGW("Unable to draw content from GPU into the provided bitmap");
+        return false;
+    }
+
+    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 */