Use colorFilters when rendering to an sRGB bitmap.

Bug: 62347704
Test: CtsUiRenderingTestCases, CtsGraphicsTestCases, hwui_unit_tests
Change-Id: I3e237b64cd92217b02d4995bdd695a28d3f393ee

7 files changed, 69 insertions, 63 deletions

diff --git a/core/jni/android/graphics/Picture.cpp b/core/jni/android/graphics/Picture.cpp
index bfb25113a7e9..7b381b41bde5 100644
--- a/core/jni/android/graphics/Picture.cpp
+++ b/core/jni/android/graphics/Picture.cpp
@@ -44,7 +44,7 @@ Canvas* Picture::beginRecording(int width, int height) {
     mWidth = width;
     mHeight = height;
     SkCanvas* canvas = mRecorder->beginRecording(SkIntToScalar(width), SkIntToScalar(height));
-    return Canvas::create_canvas(canvas, Canvas::XformToSRGB::kDefer);
+    return Canvas::create_canvas(canvas);
 }
 
 void Picture::endRecording() {
diff --git a/core/jni/android/graphics/pdf/PdfDocument.cpp b/core/jni/android/graphics/pdf/PdfDocument.cpp
index abc3599a48c1..e2dc52b70fb6 100644
--- a/core/jni/android/graphics/pdf/PdfDocument.cpp
+++ b/core/jni/android/graphics/pdf/PdfDocument.cpp
@@ -21,7 +21,6 @@
 
 #include "CreateJavaOutputStreamAdaptor.h"
 
-#include "SkColorSpaceXformCanvas.h"
 #include "SkDocument.h"
 #include "SkPicture.h"
 #include "SkPictureRecorder.h"
@@ -95,10 +94,7 @@ public:
 
             SkCanvas* canvas = document->beginPage(page->mWidth, page->mHeight,
                     &(page->mContentRect));
-            std::unique_ptr<SkCanvas> toSRGBCanvas =
-                    SkCreateColorSpaceXformCanvas(canvas, SkColorSpace::MakeSRGB());
-
-            toSRGBCanvas->drawPicture(page->mPicture);
+            canvas->drawPicture(page->mPicture);
 
             document->endPage();
         }
@@ -131,7 +127,7 @@ static jlong nativeStartPage(JNIEnv* env, jobject thiz, jlong documentPtr,
     PdfDocument* document = reinterpret_cast<PdfDocument*>(documentPtr);
     SkCanvas* canvas = document->startPage(pageWidth, pageHeight,
             contentLeft, contentTop, contentRight, contentBottom);
-    return reinterpret_cast<jlong>(Canvas::create_canvas(canvas, Canvas::XformToSRGB::kDefer));
+    return reinterpret_cast<jlong>(Canvas::create_canvas(canvas));
 }
 
 static void nativeFinishPage(JNIEnv* env, jobject thiz, jlong documentPtr) {
diff --git a/libs/hwui/SkiaCanvas.cpp b/libs/hwui/SkiaCanvas.cpp
index 9683d0614e26..d191b56768e8 100644
--- a/libs/hwui/SkiaCanvas.cpp
+++ b/libs/hwui/SkiaCanvas.cpp
@@ -25,7 +25,6 @@
 
 #include <SkCanvasStateUtils.h>
 #include <SkColorFilter.h>
-// TODO remove me!
 #include <SkColorSpaceXformCanvas.h>
 #include <SkDrawable.h>
 #include <SkDeque.h>
@@ -48,25 +47,33 @@ Canvas* Canvas::create_canvas(const SkBitmap& bitmap) {
     return new SkiaCanvas(bitmap);
 }
 
-Canvas* Canvas::create_canvas(SkCanvas* skiaCanvas, XformToSRGB xformToSRGB) {
-    return new SkiaCanvas(skiaCanvas, xformToSRGB);
+Canvas* Canvas::create_canvas(SkCanvas* skiaCanvas) {
+    return new SkiaCanvas(skiaCanvas);
 }
 
 SkiaCanvas::SkiaCanvas() {}
 
-SkiaCanvas::SkiaCanvas(SkCanvas* canvas, XformToSRGB xformToSRGB)
-    : mCanvas(canvas)
-{
-    LOG_ALWAYS_FATAL_IF(XformToSRGB::kImmediate == xformToSRGB);
-}
+SkiaCanvas::SkiaCanvas(SkCanvas* canvas) : mCanvas(canvas) {}
 
 SkiaCanvas::SkiaCanvas(const SkBitmap& bitmap) {
     sk_sp<SkColorSpace> cs = bitmap.refColorSpace();
     mCanvasOwned =
             std::unique_ptr<SkCanvas>(new SkCanvas(bitmap, SkCanvas::ColorBehavior::kLegacy));
-    mCanvasWrapper = SkCreateColorSpaceXformCanvas(mCanvasOwned.get(),
-            cs == nullptr ? SkColorSpace::MakeSRGB() : std::move(cs));
-    mCanvas = mCanvasWrapper.get();
+    if (cs.get() == nullptr || cs->isSRGB()) {
+        if(!uirenderer::Properties::isSkiaEnabled()) {
+            mCanvasWrapper = SkCreateColorSpaceXformCanvas(mCanvasOwned.get(), SkColorSpace::MakeSRGB());
+            mCanvas = mCanvasWrapper.get();
+        } else {
+            mCanvas = mCanvasOwned.get();
+        }
+    } else {
+        /** The wrapper is needed if we are drawing into a non-sRGB destination, since
+         *  we need to transform all colors (not just bitmaps via filters) into the
+         *  destination's colorspace.
+         */
+        mCanvasWrapper = SkCreateColorSpaceXformCanvas(mCanvasOwned.get(), std::move(cs));
+        mCanvas = mCanvasWrapper.get();
+    }
 }
 
 SkiaCanvas::~SkiaCanvas() {}
@@ -75,6 +82,7 @@ void SkiaCanvas::reset(SkCanvas* skiaCanvas) {
     if (mCanvas != skiaCanvas) {
         mCanvas = skiaCanvas;
         mCanvasOwned.reset();
+        mCanvasWrapper.reset();
     }
     mSaveStack.reset(nullptr);
     mHighContrastText = false;
@@ -88,13 +96,18 @@ void SkiaCanvas::setBitmap(const SkBitmap& bitmap) {
     sk_sp<SkColorSpace> cs = bitmap.refColorSpace();
     std::unique_ptr<SkCanvas> newCanvas =
             std::unique_ptr<SkCanvas>(new SkCanvas(bitmap, SkCanvas::ColorBehavior::kLegacy));
-    std::unique_ptr<SkCanvas> newCanvasWrapper = SkCreateColorSpaceXformCanvas(newCanvas.get(),
-            cs == nullptr ? SkColorSpace::MakeSRGB() : std::move(cs));
+    std::unique_ptr<SkCanvas> newCanvasWrapper;
+    if (cs.get() != nullptr && !cs->isSRGB()) {
+        newCanvasWrapper = SkCreateColorSpaceXformCanvas(newCanvas.get(), std::move(cs));
+    }
+    else if(!uirenderer::Properties::isSkiaEnabled()) {
+        newCanvasWrapper = SkCreateColorSpaceXformCanvas(newCanvas.get(), SkColorSpace::MakeSRGB());
+    }
 
     // deletes the previously owned canvas (if any)
     mCanvasOwned = std::move(newCanvas);
     mCanvasWrapper = std::move(newCanvasWrapper);
-    mCanvas = mCanvasWrapper.get();
+    mCanvas = mCanvasWrapper ? mCanvasWrapper.get() : mCanvasOwned.get();
 
     // clean up the old save stack
     mSaveStack.reset(nullptr);
@@ -531,13 +544,27 @@ void SkiaCanvas::drawVertices(const SkVertices* vertices, SkBlendMode mode, cons
 // Canvas draw operations: Bitmaps
 // ----------------------------------------------------------------------------
 
-inline static const SkPaint* addFilter(const SkPaint* origPaint, SkPaint* tmpPaint,
-        sk_sp<SkColorFilter> colorFilter) {
-    if (colorFilter) {
+const SkPaint* SkiaCanvas::addFilter(const SkPaint* origPaint, SkPaint* tmpPaint,
+        sk_sp<SkColorFilter> colorSpaceFilter) {
+    /* We don't apply the colorSpace filter if this canvas is already wrapped with
+     * a SkColorSpaceXformCanvas since it already takes care of converting the
+     * contents of the bitmap into the appropriate colorspace.  The mCanvasWrapper
+     * should only be used if this canvas is backed by a surface/bitmap that is known
+     * to have a non-sRGB colorspace.
+     */
+    if (!mCanvasWrapper && colorSpaceFilter) {
         if (origPaint) {
             *tmpPaint = *origPaint;
         }
-        tmpPaint->setColorFilter(colorFilter);
+
+        if (tmpPaint->getColorFilter()) {
+            tmpPaint->setColorFilter(SkColorFilter::MakeComposeFilter(
+                    tmpPaint->refColorFilter(), colorSpaceFilter));
+            LOG_ALWAYS_FATAL_IF(!tmpPaint->getColorFilter());
+        } else {
+            tmpPaint->setColorFilter(colorSpaceFilter);
+        }
+
         return tmpPaint;
     } else {
         return origPaint;
diff --git a/libs/hwui/SkiaCanvas.h b/libs/hwui/SkiaCanvas.h
index af2c23e4a2b7..6a01f964873b 100644
--- a/libs/hwui/SkiaCanvas.h
+++ b/libs/hwui/SkiaCanvas.h
@@ -37,12 +37,8 @@ public:
      *  @param canvas SkCanvas to handle calls made to this SkiaCanvas. Must
      *      not be NULL. This constructor does not take ownership, so the caller
      *      must guarantee that it remains valid while the SkiaCanvas is valid.
-     *  @param xformToSRGB Indicates if bitmaps should be xformed to the sRGB
-     *      color space before drawing.  This makes sense for software rendering.
-     *      For the picture case, it may make more sense to leave bitmaps as is,
-     *      and handle the xform when replaying the picture.
      */
-    explicit SkiaCanvas(SkCanvas* canvas, XformToSRGB xformToSRGB);
+    explicit SkiaCanvas(SkCanvas* canvas);
 
     virtual ~SkiaCanvas();
 
@@ -182,6 +178,9 @@ private:
     void drawPoints(const float* points, int count, const SkPaint& paint,
             SkCanvas::PointMode mode);
 
+    const SkPaint* addFilter(const SkPaint* origPaint, SkPaint* tmpPaint,
+            sk_sp<SkColorFilter> colorSpaceFilter);
+
     class Clip;
 
     std::unique_ptr<SkCanvas> mCanvasWrapper; // might own a wrapper on the canvas
diff --git a/libs/hwui/hwui/Canvas.h b/libs/hwui/hwui/Canvas.h
index ac8a08169997..90d98f22e0a5 100644
--- a/libs/hwui/hwui/Canvas.h
+++ b/libs/hwui/hwui/Canvas.h
@@ -98,15 +98,6 @@ public:
     static WARN_UNUSED_RESULT Canvas* create_recording_canvas(int width, int height,
             uirenderer::RenderNode* renderNode = nullptr);
 
-    enum class XformToSRGB {
-        // Transform any Bitmaps to the sRGB color space before drawing.
-        kImmediate,
-
-        // Draw the Bitmap as is.  This likely means that we are recording and that the
-        // transform can be handled at playback time.
-        kDefer,
-    };
-
     /**
      *  Create a new Canvas object which delegates to an SkCanvas.
      *
@@ -114,12 +105,10 @@ public:
      *      delegated to this object. This function will call ref() on the
      *      SkCanvas, and the returned Canvas will unref() it upon
      *      destruction.
-     *  @param xformToSRGB Indicates if bitmaps should be xformed to the sRGB
-     *      color space before drawing.
      *  @return new non-null Canvas Object.  The type of DisplayList produced by this canvas is
      *      determined based on  Properties::getRenderPipelineType().
      */
-    static Canvas* create_canvas(SkCanvas* skiaCanvas, XformToSRGB xformToSRGB);
+    static Canvas* create_canvas(SkCanvas* skiaCanvas);
 
     /**
      *  Provides a Skia SkCanvas interface that acts as a proxy to this Canvas.
diff --git a/libs/hwui/pipeline/skia/SkiaRecordingCanvas.cpp b/libs/hwui/pipeline/skia/SkiaRecordingCanvas.cpp
index bb4160779d57..4c1d67351161 100644
--- a/libs/hwui/pipeline/skia/SkiaRecordingCanvas.cpp
+++ b/libs/hwui/pipeline/skia/SkiaRecordingCanvas.cpp
@@ -150,8 +150,17 @@ inline static const SkPaint* bitmapPaint(const SkPaint* origPaint, SkPaint* tmpP
         if (origPaint) {
             *tmpPaint = *origPaint;
         }
+
+        sk_sp<SkColorFilter> filter;
+        if (colorFilter && tmpPaint->getColorFilter()) {
+            filter = SkColorFilter::MakeComposeFilter(tmpPaint->refColorFilter(), colorFilter);
+            LOG_ALWAYS_FATAL_IF(!filter);
+        } else {
+            filter = colorFilter;
+        }
+
         tmpPaint->setAntiAlias(false);
-        tmpPaint->setColorFilter(colorFilter);
+        tmpPaint->setColorFilter(filter);
         return tmpPaint;
     } else {
         return origPaint;
diff --git a/libs/hwui/tests/unit/SkiaCanvasTests.cpp b/libs/hwui/tests/unit/SkiaCanvasTests.cpp
index c048dda4a2e9..d84b83d3f2dc 100644
--- a/libs/hwui/tests/unit/SkiaCanvasTests.cpp
+++ b/libs/hwui/tests/unit/SkiaCanvasTests.cpp
@@ -45,8 +45,7 @@ OPENGL_PIPELINE_TEST(SkiaCanvasProxy, drawGlyphsViaPicture) {
         // record the same text draw into a SkPicture and replay it into a Recording canvas
         SkPictureRecorder recorder;
         SkCanvas* skCanvas = recorder.beginRecording(200, 200, NULL, 0);
-        std::unique_ptr<Canvas> pictCanvas(Canvas::create_canvas(skCanvas,
-                Canvas::XformToSRGB::kDefer));
+        std::unique_ptr<Canvas> pictCanvas(Canvas::create_canvas(skCanvas));
         TestUtils::drawUtf8ToCanvas(pictCanvas.get(), text, paint, 25, 25);
         sk_sp<SkPicture> picture = recorder.finishRecordingAsPicture();
 
@@ -65,7 +64,7 @@ OPENGL_PIPELINE_TEST(SkiaCanvasProxy, drawGlyphsViaPicture) {
 
 TEST(SkiaCanvas, drawShadowLayer) {
     auto surface = SkSurface::MakeRasterN32Premul(10, 10);
-    SkiaCanvas canvas(surface->getCanvas(), Canvas::XformToSRGB::kDefer);
+    SkiaCanvas canvas(surface->getCanvas());
 
     // clear to white
     canvas.drawColor(SK_ColorWHITE, SkBlendMode::kSrc);
@@ -108,27 +107,14 @@ TEST(SkiaCanvas, colorSpaceXform) {
     // The result should be less than fully red, since we convert to Adobe RGB at draw time.
     ASSERT_EQ(0xFF0000DC, *adobeSkBitmap.getAddr32(0, 0));
 
-    // Now try in kDefer mode.  This is a little strange given that, in practice, all software
-    // canvases are kImmediate.
-    SkCanvas skCanvas(skBitmap);
-    SkiaCanvas deferCanvas(&skCanvas, Canvas::XformToSRGB::kDefer);
-    deferCanvas.drawBitmap(*adobeBitmap, 0, 0, nullptr);
-    // The result should be as before, since we deferred the conversion to sRGB.
-    ASSERT_EQ(0xFF0000DC, *skBitmap.getAddr32(0, 0));
-
-    // Test picture recording.  We will kDefer the xform at recording time, but handle it when
-    // we playback to the software canvas.
+    // Test picture recording.
     SkPictureRecorder recorder;
     SkCanvas* skPicCanvas = recorder.beginRecording(1, 1, NULL, 0);
-    SkiaCanvas picCanvas(skPicCanvas, Canvas::XformToSRGB::kDefer);
+    SkiaCanvas picCanvas(skPicCanvas);
     picCanvas.drawBitmap(*adobeBitmap, 0, 0, nullptr);
     sk_sp<SkPicture> picture = recorder.finishRecordingAsPicture();
 
-    // Playback to a deferred canvas.  The result should be as before.
-    deferCanvas.asSkCanvas()->drawPicture(picture);
-    ASSERT_EQ(0xFF0000DC, *skBitmap.getAddr32(0, 0));
-
-    // Playback to an immediate canvas.  The result should be fully red.
+    // Playback to an software canvas.  The result should be fully red.
     canvas.asSkCanvas()->drawPicture(picture);
     ASSERT_EQ(0xFF0000FF, *skBitmap.getAddr32(0, 0));
 }
@@ -155,7 +141,7 @@ TEST(SkiaCanvas, captureCanvasState) {
     // Create a picture canvas.
     SkPictureRecorder recorder;
     SkCanvas* skPicCanvas = recorder.beginRecording(1, 1, NULL, 0);
-    SkiaCanvas picCanvas(skPicCanvas, Canvas::XformToSRGB::kDefer);
+    SkiaCanvas picCanvas(skPicCanvas);
     state = picCanvas.captureCanvasState();
 
     // Verify that we cannot get the CanvasState.