From 7c42bef70e102f873950550393c6951ddae5975f Mon Sep 17 00:00:00 2001 From: Derek Sollenberger Date: Tue, 23 Feb 2021 13:01:39 -0500 Subject: Use stack based cleanup for canvas save/restore calls. Using this approach enables future CLs to add quick reject logic to drawing layers in future CLs and more cleanly manages the save/restore lifetimes. Test: librendererengine_test Bug: 181028875 Change-Id: I061a6986d0a2d3735d35b1c065a1bd41c147a41b --- libs/renderengine/skia/SkiaGLRenderEngine.cpp | 29 ++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/libs/renderengine/skia/SkiaGLRenderEngine.cpp b/libs/renderengine/skia/SkiaGLRenderEngine.cpp index 327b04c699..769951f00f 100644 --- a/libs/renderengine/skia/SkiaGLRenderEngine.cpp +++ b/libs/renderengine/skia/SkiaGLRenderEngine.cpp @@ -556,6 +556,26 @@ void SkiaGLRenderEngine::initCanvas(SkCanvas* canvas, const DisplaySettings& dis canvas->translate(-display.clip.left, -display.clip.top); } +class AutoSaveRestore { +public: + AutoSaveRestore(SkCanvas* canvas) : mCanvas(canvas) { mSaveCount = canvas->save(); } + ~AutoSaveRestore() { restore(); } + void replace(SkCanvas* canvas) { + mCanvas = canvas; + mSaveCount = canvas->save(); + } + void restore() { + if (mCanvas) { + mCanvas->restoreToCount(mSaveCount); + mCanvas = nullptr; + } + } + +private: + SkCanvas* mCanvas; + int mSaveCount; +}; + status_t SkiaGLRenderEngine::drawLayers(const DisplaySettings& display, const std::vector& layers, const sp& buffer, @@ -650,7 +670,7 @@ status_t SkiaGLRenderEngine::drawLayers(const DisplaySettings& display, } } - canvas->save(); + AutoSaveRestore surfaceAutoSaveRestore(canvas); // Clear the entire canvas with a transparent black to prevent ghost images. canvas->clear(SK_ColorTRANSPARENT); initCanvas(canvas, display); @@ -705,7 +725,7 @@ status_t SkiaGLRenderEngine::drawLayers(const DisplaySettings& display, // assign dstCanvas to canvas and ensure that the canvas state is up to date canvas = dstCanvas; - canvas->save(); + surfaceAutoSaveRestore.replace(canvas); initCanvas(canvas, display); LOG_ALWAYS_FATAL_IF(activeSurface->getCanvas()->getSaveCount() != @@ -717,7 +737,7 @@ status_t SkiaGLRenderEngine::drawLayers(const DisplaySettings& display, activeSurface = dstSurface; } - canvas->save(); + SkAutoCanvasRestore layerAutoSaveRestore(canvas, true); if (CC_UNLIKELY(mCapture->isCaptureRunning())) { // Record the name of the layer if the capture is running. std::stringstream layerSettings; @@ -888,9 +908,8 @@ status_t SkiaGLRenderEngine::drawLayers(const DisplaySettings& display, } canvas->drawRect(bounds, paint); } - canvas->restore(); } - canvas->restore(); + surfaceAutoSaveRestore.restore(); mCapture->endCapture(); { ATRACE_NAME("flush surface"); -- cgit v1.2.3-59-g8ed1b From 4c331c851d8cf81837e66423426e33baab469edb Mon Sep 17 00:00:00 2001 From: Derek Sollenberger Date: Tue, 23 Feb 2021 13:09:50 -0500 Subject: Avoid unnecessary computation for shadows and clipping for rounded layers Shadow layers did not need to generate the shader as it was not used so avoid that overhead. Setting a roundrect clip and then drawing a single rect of the same size is always less efficient than just drawing the roundrect directly. Test: librenderengine_test Bug: 181028875 Change-Id: I4e333f34b886f630773e4d660973bf8aa4585635 --- libs/renderengine/skia/SkiaGLRenderEngine.cpp | 28 ++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/libs/renderengine/skia/SkiaGLRenderEngine.cpp b/libs/renderengine/skia/SkiaGLRenderEngine.cpp index 769951f00f..5ceae63334 100644 --- a/libs/renderengine/skia/SkiaGLRenderEngine.cpp +++ b/libs/renderengine/skia/SkiaGLRenderEngine.cpp @@ -785,6 +785,18 @@ status_t SkiaGLRenderEngine::drawLayers(const DisplaySettings& display, } } + // Shadows are assumed to live only on their own layer - it's not valid + // to draw the boundary rectangles when there is already a caster shadow + // TODO(b/175915334): consider relaxing this restriction to enable more flexible + // composition - using a well-defined invalid color is long-term less error-prone. + if (layer->shadow.length > 0) { + const auto rect = layer->geometry.roundedCornersRadius > 0 + ? getSkRect(layer->geometry.roundedCornersCrop) + : bounds; + drawShadow(canvas, rect, layer->geometry.roundedCornersRadius, layer->shadow); + continue; + } + const ui::Dataspace targetDataspace = mUseColorManagement ? (needsLinearEffect(layer->colorTransform, layer->sourceDataspace, display.outputDataspace) @@ -892,20 +904,10 @@ status_t SkiaGLRenderEngine::drawLayers(const DisplaySettings& display, paint.setColorFilter(filter); - if (layer->shadow.length > 0) { - const auto rect = layer->geometry.roundedCornersRadius > 0 - ? getSkRect(layer->geometry.roundedCornersCrop) - : bounds; - drawShadow(canvas, rect, layer->geometry.roundedCornersRadius, layer->shadow); + if (layer->geometry.roundedCornersRadius > 0) { + paint.setAntiAlias(true); + canvas->drawRRect(getRoundedRect(layer), paint); } else { - // Shadows are assumed to live only on their own layer - it's not valid - // to draw the boundary retangles when there is already a caster shadow - // TODO(b/175915334): consider relaxing this restriction to enable more flexible - // composition - using a well-defined invalid color is long-term less error-prone. - // Push the clipRRect onto the clip stack. Draw the image. Pop the clip. - if (layer->geometry.roundedCornersRadius > 0) { - canvas->clipRRect(getRoundedRect(layer), true); - } canvas->drawRect(bounds, paint); } } -- cgit v1.2.3-59-g8ed1b From e2fe78c03bbcf4064da812271b2e0181f86bcf19 Mon Sep 17 00:00:00 2001 From: Derek Sollenberger Date: Tue, 23 Feb 2021 13:22:54 -0500 Subject: 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 --- libs/renderengine/skia/SkiaGLRenderEngine.cpp | 81 +++++++++++++++------------ libs/renderengine/skia/SkiaGLRenderEngine.h | 7 ++- 2 files changed, 49 insertions(+), 39 deletions(-) diff --git a/libs/renderengine/skia/SkiaGLRenderEngine.cpp b/libs/renderengine/skia/SkiaGLRenderEngine.cpp index 5ceae63334..9f40011411 100644 --- a/libs/renderengine/skia/SkiaGLRenderEngine.cpp +++ b/libs/renderengine/skia/SkiaGLRenderEngine.cpp @@ -454,11 +454,6 @@ static bool needsToneMapping(ui::Dataspace sourceDataspace, ui::Dataspace destin 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& 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 @@ void SkiaGLRenderEngine::unbindExternalTextureBuffer(uint64_t bufferId) { sk_sp SkiaGLRenderEngine::createRuntimeEffectShader(sk_sp 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 @@ status_t SkiaGLRenderEngine::drawLayers(const DisplaySettings& display, } } + const ui::Dataspace dstDataspace = + mUseColorManagement ? display.outputDataspace : ui::Dataspace::UNKNOWN; sk_sp 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 @@ status_t SkiaGLRenderEngine::drawLayers(const DisplaySettings& display, SkPaint paint; sk_sp 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 displayColorTransform; + if (display.colorTransform != mat4()) { + displayColorTransform = SkColorFilters::Matrix(toSkColorMatrix(display.colorTransform)); + } + for (const auto& layer : layers) { ATRACE_NAME("DrawLayer"); @@ -797,15 +801,22 @@ status_t SkiaGLRenderEngine::drawLayers(const DisplaySettings& display, 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 @@ status_t SkiaGLRenderEngine::drawLayers(const DisplaySettings& display, } sk_sp image = - imageTextureRef->getTexture()->makeImage(targetDataspace, + imageTextureRef->getTexture()->makeImage(layerDataspace, item.usePremultipliedAlpha ? kPremul_SkAlphaType : kUnpremul_SkAlphaType, @@ -880,12 +891,12 @@ status_t SkiaGLRenderEngine::drawLayers(const DisplaySettings& display, 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 @@ status_t SkiaGLRenderEngine::drawLayers(const DisplaySettings& display, .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 filter = - SkColorFilters::Matrix(toSkColorMatrix(display.colorTransform)); - - paint.setColorFilter(filter); + paint.setColorFilter(displayColorTransform); if (layer->geometry.roundedCornersRadius > 0) { paint.setAntiAlias(true); diff --git a/libs/renderengine/skia/SkiaGLRenderEngine.h b/libs/renderengine/skia/SkiaGLRenderEngine.h index 5779ae679b..ad26206a8f 100644 --- a/libs/renderengine/skia/SkiaGLRenderEngine.h +++ b/libs/renderengine/skia/SkiaGLRenderEngine.h @@ -92,11 +92,12 @@ private: void initCanvas(SkCanvas* canvas, const DisplaySettings& display); void drawShadow(SkCanvas* canvas, const SkRect& casterRect, float casterCornerRadius, const ShadowSettings& shadowSettings); - // If mUseColorManagement is correct and layer needsLinearEffect, it returns a linear runtime - // shader. Otherwise it returns the input shader. + // If requiresLinearEffect is true or the layer has a stretchEffect a new shader is returned. + // Otherwise it returns the input shader. sk_sp createRuntimeEffectShader(sk_sp shader, const LayerSettings* layer, const DisplaySettings& display, - bool undoPremultipliedAlpha); + bool undoPremultipliedAlpha, + bool requiresLinearEffect); EGLDisplay mEGLDisplay; EGLContext mEGLContext; -- cgit v1.2.3-59-g8ed1b