Merge "SkiaRE: Use RGB_888x for isOpaque" into sc-dev

4 files changed, 98 insertions, 27 deletions

diff --git a/libs/renderengine/skia/AutoBackendTexture.cpp b/libs/renderengine/skia/AutoBackendTexture.cpp
index 8ae69de7f3..8356005911 100644
--- a/libs/renderengine/skia/AutoBackendTexture.cpp
+++ b/libs/renderengine/skia/AutoBackendTexture.cpp
@@ -87,8 +87,15 @@ sk_sp<SkImage> AutoBackendTexture::makeImage(ui::Dataspace dataspace, SkAlphaTyp
         mUpdateProc(mImageCtx, context);
     }
 
+    auto colorType = mColorType;
+    if (alphaType == kOpaque_SkAlphaType) {
+        if (colorType == kRGBA_8888_SkColorType) {
+            colorType = kRGB_888x_SkColorType;
+        }
+    }
+
     sk_sp<SkImage> image =
-            SkImage::MakeFromTexture(context, mBackendTexture, kTopLeft_GrSurfaceOrigin, mColorType,
+            SkImage::MakeFromTexture(context, mBackendTexture, kTopLeft_GrSurfaceOrigin, colorType,
                                      alphaType, toSkColorSpace(dataspace), releaseImageProc, this);
     if (image.get()) {
         // The following ref will be counteracted by releaseProc, when SkImage is discarded.
diff --git a/libs/renderengine/skia/AutoBackendTexture.h b/libs/renderengine/skia/AutoBackendTexture.h
index 3133de60b5..a9e8430d66 100644
--- a/libs/renderengine/skia/AutoBackendTexture.h
+++ b/libs/renderengine/skia/AutoBackendTexture.h
@@ -65,6 +65,8 @@ public:
             return mTexture->getOrCreateSurface(dataspace, context);
         }
 
+        SkColorType colorType() const { return mTexture->mColorType; }
+
         DISALLOW_COPY_AND_ASSIGN(LocalRef);
 
     private:
diff --git a/libs/renderengine/skia/SkiaGLRenderEngine.cpp b/libs/renderengine/skia/SkiaGLRenderEngine.cpp
index 47c330ffb1..2d80c46461 100644
--- a/libs/renderengine/skia/SkiaGLRenderEngine.cpp
+++ b/libs/renderengine/skia/SkiaGLRenderEngine.cpp
@@ -971,11 +971,28 @@ status_t SkiaGLRenderEngine::drawLayers(const DisplaySettings& display,
                                                       false);
             }
 
-            sk_sp<SkImage> image =
-                    imageTextureRef->makeImage(layerDataspace,
-                                               item.usePremultipliedAlpha ? kPremul_SkAlphaType
-                                                                          : kUnpremul_SkAlphaType,
-                                               grContext);
+            // isOpaque means we need to ignore the alpha in the image,
+            // replacing it with the alpha specified by the LayerSettings. See
+            // https://developer.android.com/reference/android/view/SurfaceControl.Builder#setOpaque(boolean)
+            // The proper way to do this is to use an SkColorType that ignores
+            // alpha, like kRGB_888x_SkColorType, and that is used if the
+            // incoming image is kRGBA_8888_SkColorType. However, the incoming
+            // image may be kRGBA_F16_SkColorType, for which there is no RGBX
+            // SkColorType, or kRGBA_1010102_SkColorType, for which we have
+            // kRGB_101010x_SkColorType, but it is not yet supported as a source
+            // on the GPU. (Adding both is tracked in skbug.com/12048.) In the
+            // meantime, we'll use a workaround that works unless we need to do
+            // any color conversion. The workaround requires that we pretend the
+            // image is already premultiplied, so that we do not premultiply it
+            // before applying SkBlendMode::kPlus.
+            const bool useIsOpaqueWorkaround = item.isOpaque &&
+                    (imageTextureRef->colorType() == kRGBA_1010102_SkColorType ||
+                     imageTextureRef->colorType() == kRGBA_F16_SkColorType);
+            const auto alphaType = useIsOpaqueWorkaround ? kPremul_SkAlphaType
+                    : item.isOpaque                      ? kOpaque_SkAlphaType
+                    : item.usePremultipliedAlpha         ? kPremul_SkAlphaType
+                                                         : kUnpremul_SkAlphaType;
+            sk_sp<SkImage> image = imageTextureRef->makeImage(layerDataspace, alphaType, grContext);
 
             auto texMatrix = getSkM44(item.textureTransform).asM33();
             // textureTansform was intended to be passed directly into a shader, so when
@@ -1004,27 +1021,7 @@ status_t SkiaGLRenderEngine::drawLayers(const DisplaySettings& display,
                 shader = image->makeShader(SkSamplingOptions(), matrix);
             }
 
-            // Handle opaque images - it's a little nonstandard how we do this.
-            // Fundamentally we need to support SurfaceControl.Builder#setOpaque:
-            // https://developer.android.com/reference/android/view/SurfaceControl.Builder#setOpaque(boolean)
-            // The important language is that when isOpaque is set, opacity is not sampled from the
-            // alpha channel, but blending may still be supported on a transaction via setAlpha. So,
-            // here's the conundrum:
-            // 1. We can't force the SkImage alpha type to kOpaque_SkAlphaType, because it's treated
-            // as an internal hint - composition is undefined when there are alpha bits present.
-            // 2. We can try to lie about the pixel layout, but that only works for RGBA8888
-            // buffers, i.e., treating them as RGBx8888 instead. But we can't do the same for
-            // RGBA1010102 because RGBx1010102 is not supported as a pixel layout for SkImages. It's
-            // also not clear what to use for F16 either, and lying about the pixel layout is a bit
-            // of a hack anyways.
-            // 3. We can't change the blendmode to src, because while this satisfies the requirement
-            // for ignoring the alpha channel, it doesn't quite satisfy the blending requirement
-            // because src always clobbers the destination content.
-            //
-            // So, what we do here instead is an additive blend mode where we compose the input
-            // image with a solid black. This might need to be reassess if this does not support
-            // FP16 incredibly well, but FP16 end-to-end isn't well supported anyway at the moment.
-            if (item.isOpaque) {
+            if (useIsOpaqueWorkaround) {
                 shader = SkShaders::Blend(SkBlendMode::kPlus, shader,
                                           SkShaders::Color(SkColors::kBlack,
                                                            toSkColorSpace(layerDataspace)));
diff --git a/libs/renderengine/tests/RenderEngineTest.cpp b/libs/renderengine/tests/RenderEngineTest.cpp
index 1ca7a16fe0..dfab6e8cd2 100644
--- a/libs/renderengine/tests/RenderEngineTest.cpp
+++ b/libs/renderengine/tests/RenderEngineTest.cpp
@@ -54,6 +54,7 @@ public:
     virtual std::unique_ptr<renderengine::gl::GLESRenderEngine> createGLESRenderEngine() {
         return nullptr;
     }
+    virtual bool useColorManagement() const = 0;
 };
 
 class GLESRenderEngineFactory : public RenderEngineFactory {
@@ -82,6 +83,8 @@ public:
                         .build();
         return renderengine::gl::GLESRenderEngine::create(reCreationArgs);
     }
+
+    bool useColorManagement() const override { return false; }
 };
 
 class GLESCMRenderEngineFactory : public RenderEngineFactory {
@@ -110,6 +113,8 @@ public:
                         .build();
         return renderengine::gl::GLESRenderEngine::create(reCreationArgs);
     }
+
+    bool useColorManagement() const override { return true; }
 };
 
 class SkiaGLESRenderEngineFactory : public RenderEngineFactory {
@@ -130,9 +135,16 @@ public:
                         .setSupportsBackgroundBlur(true)
                         .setContextPriority(renderengine::RenderEngine::ContextPriority::MEDIUM)
                         .setRenderEngineType(type())
+                        // FIXME (b/189935602): This version is currently color managed.
+                        // We should change it and fix the tests that fail.
+                        //.setUseColorManagerment(false)
                         .build();
         return renderengine::skia::SkiaGLRenderEngine::create(reCreationArgs);
     }
+
+    // FIXME (b/189935602): This version is currently color managed.
+    // We should change it and fix the tests that fail.
+    bool useColorManagement() const override { return true; }
 };
 
 class SkiaGLESCMRenderEngineFactory : public RenderEngineFactory {
@@ -157,6 +169,8 @@ public:
                         .build();
         return renderengine::skia::SkiaGLRenderEngine::create(reCreationArgs);
     }
+
+    bool useColorManagement() const override { return true; }
 };
 
 class RenderEngineTest : public ::testing::TestWithParam<std::shared_ptr<RenderEngineFactory>> {
@@ -295,6 +309,7 @@ public:
                 const uint8_t expected[4] = {r, g, b, a};
                 bool equal = colorCompare(src, expected);
                 EXPECT_TRUE(equal)
+                        << GetParam()->name().c_str() << ": "
                         << "pixel @ (" << region.left + i << ", " << region.top + j << "): "
                         << "expected (" << static_cast<uint32_t>(r) << ", "
                         << static_cast<uint32_t>(g) << ", " << static_cast<uint32_t>(b) << ", "
@@ -2015,6 +2030,56 @@ TEST_P(RenderEngineTest, testDisableBlendingBuffer) {
     expectBufferColor(rect, 0, 128, 0, 128);
 }
 
+TEST_P(RenderEngineTest, test_isOpaque) {
+    initializeRenderEngine();
+
+    const auto rect = Rect(0, 0, 1, 1);
+    const renderengine::DisplaySettings display{
+            .physicalDisplay = rect,
+            .clip = rect,
+            .outputDataspace = ui::Dataspace::DISPLAY_P3,
+    };
+
+    // Create an unpremul buffer that is green with no alpha. Using isOpaque
+    // should make the green show.
+    const auto buf = allocateSourceBuffer(1, 1);
+    {
+        uint8_t* pixels;
+        buf->getBuffer()->lock(GRALLOC_USAGE_SW_READ_OFTEN | GRALLOC_USAGE_SW_WRITE_OFTEN,
+                               reinterpret_cast<void**>(&pixels));
+        pixels[0] = 0;
+        pixels[1] = 255;
+        pixels[2] = 0;
+        pixels[3] = 0;
+        buf->getBuffer()->unlock();
+    }
+
+    const renderengine::LayerSettings greenLayer{
+            .geometry.boundaries = rect.toFloatRect(),
+            .source =
+                    renderengine::PixelSource{
+                            .buffer =
+                                    renderengine::Buffer{
+                                            .buffer = buf,
+                                            // Although the pixels are not
+                                            // premultiplied in practice, this
+                                            // matches the input we see.
+                                            .usePremultipliedAlpha = true,
+                                            .isOpaque = true,
+                                    },
+                    },
+            .alpha = 1.0f,
+    };
+
+    std::vector<const renderengine::LayerSettings*> layers{&greenLayer};
+    invokeDraw(display, layers);
+
+    if (GetParam()->useColorManagement()) {
+        expectBufferColor(rect, 117, 251, 76, 255);
+    } else {
+        expectBufferColor(rect, 0, 255, 0, 255);
+    }
+}
 } // namespace android
 
 // TODO(b/129481165): remove the #pragma below and fix conversion issues