Cleanup Skia RE display and layer colorTransforms
In the general case we don't set the display's colorMatrix on the paint
if we know it is the identity matrix. We also now abort early if
the layer has no alpha and no colortransform or colorspace conversion
will change that.
The layer's colortransform is now respected regardless of whether or
not color management is enabled for the RE instance
Also includes some small code refactors to avoid computing the same
values in multiple places.
Test: librenderengine_test
Bug: 180712498
Bug: 181028875
Change-Id: I6e236efc5987d00cb464e8798c48f2b3b21635c5
diff --git a/libs/renderengine/skia/SkiaGLRenderEngine.cpp b/libs/renderengine/skia/SkiaGLRenderEngine.cpp
index 5ceae63..9f40011 100644
--- a/libs/renderengine/skia/SkiaGLRenderEngine.cpp
+++ b/libs/renderengine/skia/SkiaGLRenderEngine.cpp
@@ -454,11 +454,6 @@
sourceTransfer != destTransfer;
}
-static bool needsLinearEffect(const mat4& colorTransform, ui::Dataspace sourceDataspace,
- ui::Dataspace destinationDataspace) {
- return colorTransform != mat4() || needsToneMapping(sourceDataspace, destinationDataspace);
-}
-
void SkiaGLRenderEngine::cacheExternalTextureBuffer(const sp<GraphicBuffer>& buffer) {
// Only run this if RE is running on its own thread. This way the access to GL
// operations is guaranteed to be happening on the same thread.
@@ -491,14 +486,19 @@
sk_sp<SkShader> SkiaGLRenderEngine::createRuntimeEffectShader(sk_sp<SkShader> shader,
const LayerSettings* layer,
const DisplaySettings& display,
- bool undoPremultipliedAlpha) {
+ bool undoPremultipliedAlpha,
+ bool requiresLinearEffect) {
if (layer->stretchEffect.hasEffect()) {
// TODO: Implement
}
- if (mUseColorManagement &&
- needsLinearEffect(layer->colorTransform, layer->sourceDataspace, display.outputDataspace)) {
- LinearEffect effect = LinearEffect{.inputDataspace = layer->sourceDataspace,
- .outputDataspace = display.outputDataspace,
+ if (requiresLinearEffect) {
+ const ui::Dataspace inputDataspace =
+ mUseColorManagement ? layer->sourceDataspace : ui::Dataspace::UNKNOWN;
+ const ui::Dataspace outputDataspace =
+ mUseColorManagement ? display.outputDataspace : ui::Dataspace::UNKNOWN;
+
+ LinearEffect effect = LinearEffect{.inputDataspace = inputDataspace,
+ .outputDataspace = outputDataspace,
.undoPremultipliedAlpha = undoPremultipliedAlpha};
auto effectIter = mRuntimeEffects.find(effect);
@@ -630,11 +630,10 @@
}
}
+ const ui::Dataspace dstDataspace =
+ mUseColorManagement ? display.outputDataspace : ui::Dataspace::UNKNOWN;
sk_sp<SkSurface> dstSurface =
- surfaceTextureRef->getTexture()->getOrCreateSurface(mUseColorManagement
- ? display.outputDataspace
- : ui::Dataspace::UNKNOWN,
- grContext.get());
+ surfaceTextureRef->getTexture()->getOrCreateSurface(dstDataspace, grContext.get());
SkCanvas* dstCanvas = mCapture->tryCapture(dstSurface.get());
if (dstCanvas == nullptr) {
@@ -691,13 +690,18 @@
SkPaint paint;
sk_sp<SkShader> shader =
SkShaders::Color(SkColor4f{.fR = 0., .fG = 0., .fB = 0., .fA = 1.0},
- toSkColorSpace(mUseColorManagement ? display.outputDataspace
- : ui::Dataspace::UNKNOWN));
+ toSkColorSpace(dstDataspace));
paint.setShader(shader);
clearRegion.setRects(skRects, numRects);
canvas->drawRegion(clearRegion, paint);
}
+ // setup color filter if necessary
+ sk_sp<SkColorFilter> displayColorTransform;
+ if (display.colorTransform != mat4()) {
+ displayColorTransform = SkColorFilters::Matrix(toSkColorMatrix(display.colorTransform));
+ }
+
for (const auto& layer : layers) {
ATRACE_NAME("DrawLayer");
@@ -797,15 +801,22 @@
continue;
}
- const ui::Dataspace targetDataspace = mUseColorManagement
- ? (needsLinearEffect(layer->colorTransform, layer->sourceDataspace,
- display.outputDataspace)
- // If we need to map to linear space, then mark the source image with the
- // same colorspace as the destination surface so that Skia's color
- // management is a no-op.
- ? display.outputDataspace
- : layer->sourceDataspace)
- : ui::Dataspace::UNKNOWN;
+ const bool requiresLinearEffect = layer->colorTransform != mat4() ||
+ (mUseColorManagement &&
+ needsToneMapping(layer->sourceDataspace, display.outputDataspace));
+
+ // quick abort from drawing the remaining portion of the layer
+ if (layer->alpha == 0 && !requiresLinearEffect &&
+ (!displayColorTransform || displayColorTransform->isAlphaUnchanged())) {
+ continue;
+ }
+
+ // If we need to map to linear space or color management is disabled, then mark the source
+ // image with the same colorspace as the destination surface so that Skia's color
+ // management is a no-op.
+ const ui::Dataspace layerDataspace = (!mUseColorManagement || requiresLinearEffect)
+ ? dstDataspace
+ : layer->sourceDataspace;
SkPaint paint;
if (layer->source.buffer.buffer) {
@@ -824,7 +835,7 @@
}
sk_sp<SkImage> image =
- imageTextureRef->getTexture()->makeImage(targetDataspace,
+ imageTextureRef->getTexture()->makeImage(layerDataspace,
item.usePremultipliedAlpha
? kPremul_SkAlphaType
: kUnpremul_SkAlphaType,
@@ -880,12 +891,12 @@
if (item.isOpaque) {
shader = SkShaders::Blend(SkBlendMode::kPlus, shader,
SkShaders::Color(SkColors::kBlack,
- toSkColorSpace(targetDataspace)));
+ toSkColorSpace(layerDataspace)));
}
- paint.setShader(
- createRuntimeEffectShader(shader, layer, display,
- !item.isOpaque && item.usePremultipliedAlpha));
+ paint.setShader(createRuntimeEffectShader(shader, layer, display,
+ !item.isOpaque && item.usePremultipliedAlpha,
+ requiresLinearEffect));
paint.setAlphaf(layer->alpha);
} else {
ATRACE_NAME("DrawColor");
@@ -894,15 +905,13 @@
.fG = color.g,
.fB = color.b,
.fA = layer->alpha},
- toSkColorSpace(targetDataspace));
+ toSkColorSpace(layerDataspace));
paint.setShader(createRuntimeEffectShader(shader, layer, display,
- /* undoPremultipliedAlpha */ false));
+ /* undoPremultipliedAlpha */ false,
+ requiresLinearEffect));
}
- sk_sp<SkColorFilter> filter =
- SkColorFilters::Matrix(toSkColorMatrix(display.colorTransform));
-
- paint.setColorFilter(filter);
+ paint.setColorFilter(displayColorTransform);
if (layer->geometry.roundedCornersRadius > 0) {
paint.setAntiAlias(true);