Merge "SkiaRE: Use RGB_888x for isOpaque" into sc-dev
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 47c330f..2d80c46 100644
--- a/libs/renderengine/skia/SkiaGLRenderEngine.cpp
+++ b/libs/renderengine/skia/SkiaGLRenderEngine.cpp
@@ -971,11 +971,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
@@ -1004,27 +1021,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