SkiaRE: Use RGB_888x for isOpaque
Bug: 181130369
Test: manual
Test: librenderengine_test
The old method of blending with black doesn't work if we need to do
color conversion. The Skia pipeline premultiplies the shader prior to
blending. We can claim the image is premul to avoid that, but if we need
to do color conversion, we'll unpremultiply and convert, leading to the
current bug.
Instead, use SkColorTypes that ignore the alpha bits where possible. At
the moment, this only works for kRGBA_8888_SkColorType. There are two
other input types that we might see: kRGBA_1010102_SkColorType and
kRGBA_F16_SkColorType. While the former has an "ignore alpha" version,
it is not yet supported as a source to the GPU. The latter does not yet
have an "ignore alpha" version. Adding support for both is tracked in
skbug.com/12048. In the meantime, keep the old behavior for the
unsupported types, rather than breaking them even worse.
Change-Id: I7b6c4ff92a39a5e1c510a0a91117f55d7ce5115f
diff --git a/libs/renderengine/skia/AutoBackendTexture.cpp b/libs/renderengine/skia/AutoBackendTexture.cpp
index 8ae69de..8356005 100644
--- a/libs/renderengine/skia/AutoBackendTexture.cpp
+++ b/libs/renderengine/skia/AutoBackendTexture.cpp
@@ -87,8 +87,15 @@
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 3133de6..a9e8430 100644
--- a/libs/renderengine/skia/AutoBackendTexture.h
+++ b/libs/renderengine/skia/AutoBackendTexture.h
@@ -65,6 +65,8 @@
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 9f6ecaa..3270df1 100644
--- a/libs/renderengine/skia/SkiaGLRenderEngine.cpp
+++ b/libs/renderengine/skia/SkiaGLRenderEngine.cpp
@@ -965,11 +965,28 @@
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
@@ -998,27 +1015,7 @@
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 1ca7a16..dfab6e8 100644
--- a/libs/renderengine/tests/RenderEngineTest.cpp
+++ b/libs/renderengine/tests/RenderEngineTest.cpp
@@ -54,6 +54,7 @@
virtual std::unique_ptr<renderengine::gl::GLESRenderEngine> createGLESRenderEngine() {
return nullptr;
}
+ virtual bool useColorManagement() const = 0;
};
class GLESRenderEngineFactory : public RenderEngineFactory {
@@ -82,6 +83,8 @@
.build();
return renderengine::gl::GLESRenderEngine::create(reCreationArgs);
}
+
+ bool useColorManagement() const override { return false; }
};
class GLESCMRenderEngineFactory : public RenderEngineFactory {
@@ -110,6 +113,8 @@
.build();
return renderengine::gl::GLESRenderEngine::create(reCreationArgs);
}
+
+ bool useColorManagement() const override { return true; }
};
class SkiaGLESRenderEngineFactory : public RenderEngineFactory {
@@ -130,9 +135,16 @@
.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 @@
.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 @@
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 @@
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