From 4876de16e34622634266d09522c9153c78c7c2fb Mon Sep 17 00:00:00 2001 From: Chris Craik Date: Thu, 25 Feb 2016 16:54:08 -0800 Subject: [PATCH] Properly reject empty unclipped savelayers bug:27225580 bug:27281241 Empty unclipped savelayers (clipped at defer time, often by dirty rect) were resulting in invalid layer clear rectangles. Simplify by just rejecting these unclipped savelayers entirely at defer. Also, use repaint rect as base clip for constructed ops within LayerBuilder. Change-Id: I5c466199e85201a2f68f5cdc60b29187c849961b --- libs/hwui/BakedOpRenderer.cpp | 2 +- libs/hwui/BakedOpState.cpp | 22 ++++++--- libs/hwui/BakedOpState.h | 4 +- libs/hwui/FrameBuilder.cpp | 53 +++++++++++--------- libs/hwui/LayerBuilder.cpp | 6 +-- libs/hwui/LayerBuilder.h | 2 +- libs/hwui/tests/unit/FrameBuilderTests.cpp | 56 ++++++++++++++++++++++ 7 files changed, 108 insertions(+), 37 deletions(-) diff --git a/libs/hwui/BakedOpRenderer.cpp b/libs/hwui/BakedOpRenderer.cpp index 35c8f6b52d382..5f689b48a8447 100644 --- a/libs/hwui/BakedOpRenderer.cpp +++ b/libs/hwui/BakedOpRenderer.cpp @@ -347,7 +347,7 @@ void BakedOpRenderer::dirtyRenderTarget(const Rect& uiDirty) { if (valid) { dirty = android::Rect(uiDirty.left, uiDirty.top, uiDirty.right, uiDirty.bottom); } else { - dirty = android::Rect(0, 0, + dirty = android::Rect( mRenderTarget.offscreenBuffer->viewportWidth, mRenderTarget.offscreenBuffer->viewportHeight); } diff --git a/libs/hwui/BakedOpState.cpp b/libs/hwui/BakedOpState.cpp index a542c26b41b84..682bd045098de 100644 --- a/libs/hwui/BakedOpState.cpp +++ b/libs/hwui/BakedOpState.cpp @@ -21,6 +21,15 @@ namespace android { namespace uirenderer { +static int computeClipSideFlags(const Rect& clip, const Rect& bounds) { + int clipSideFlags = 0; + if (clip.left > bounds.left) clipSideFlags |= OpClipSideFlags::Left; + if (clip.top > bounds.top) clipSideFlags |= OpClipSideFlags::Top; + if (clip.right < bounds.right) clipSideFlags |= OpClipSideFlags::Right; + if (clip.bottom < bounds.bottom) clipSideFlags |= OpClipSideFlags::Bottom; + return clipSideFlags; +} + ResolvedRenderState::ResolvedRenderState(LinearAllocator& allocator, Snapshot& snapshot, const RecordedOp& recordedOp, bool expandForStroke) { // resolvedMatrix = parentMatrix * localMatrix @@ -55,10 +64,7 @@ ResolvedRenderState::ResolvedRenderState(LinearAllocator& allocator, Snapshot& s clippedBounds.setEmpty(); } else { // Not rejected! compute true clippedBounds and clipSideFlags - if (clipRect.left > clippedBounds.left) clipSideFlags |= OpClipSideFlags::Left; - if (clipRect.top > clippedBounds.top) clipSideFlags |= OpClipSideFlags::Top; - if (clipRect.right < clippedBounds.right) clipSideFlags |= OpClipSideFlags::Right; - if (clipRect.bottom < clippedBounds.bottom) clipSideFlags |= OpClipSideFlags::Bottom; + clipSideFlags = computeClipSideFlags(clipRect, clippedBounds); clippedBounds.doIntersect(clipRect); } } @@ -69,11 +75,13 @@ ResolvedRenderState::ResolvedRenderState(LinearAllocator& allocator, Snapshot& s , clippedBounds(clipState->rect) , clipSideFlags(OpClipSideFlags::Full) {} -ResolvedRenderState::ResolvedRenderState(const ClipRect* viewportRect, const Rect& dstRect) +ResolvedRenderState::ResolvedRenderState(const ClipRect* clipRect, const Rect& dstRect) : transform(Matrix4::identity()) - , clipState(viewportRect) + , clipState(clipRect) , clippedBounds(dstRect) - , clipSideFlags(OpClipSideFlags::None) {} + , clipSideFlags(computeClipSideFlags(clipRect->rect, dstRect)) { + clippedBounds.doIntersect(clipRect->rect); +} } // namespace uirenderer } // namespace android diff --git a/libs/hwui/BakedOpState.h b/libs/hwui/BakedOpState.h index 5a5845af81b96..4365ef870ddaa 100644 --- a/libs/hwui/BakedOpState.h +++ b/libs/hwui/BakedOpState.h @@ -175,8 +175,8 @@ private: , projectionPathMask(snapshot.projectionPathMask) , op(shadowOpPtr) {} - BakedOpState(const ClipRect* viewportRect, const Rect& dstRect, const RecordedOp& recordedOp) - : computedState(viewportRect, dstRect) + BakedOpState(const ClipRect* clipRect, const Rect& dstRect, const RecordedOp& recordedOp) + : computedState(clipRect, dstRect) , alpha(1.0f) , roundRectClipState(nullptr) , projectionPathMask(nullptr) diff --git a/libs/hwui/FrameBuilder.cpp b/libs/hwui/FrameBuilder.cpp index 4f51036b336ed..04de98afe85c7 100644 --- a/libs/hwui/FrameBuilder.cpp +++ b/libs/hwui/FrameBuilder.cpp @@ -782,39 +782,46 @@ void FrameBuilder::deferBeginUnclippedLayerOp(const BeginUnclippedLayerOp& op) { boundsTransform.mapRect(dstRect); dstRect.doIntersect(mCanvasState.currentSnapshot()->getRenderTargetClip()); - // Allocate a holding position for the layer object (copyTo will produce, copyFrom will consume) - OffscreenBuffer** layerHandle = mAllocator.create(nullptr); + if (dstRect.isEmpty()) { + // Unclipped layer rejected - push a null op, so next EndUnclippedLayerOp is ignored + currentLayer().activeUnclippedSaveLayers.push_back(nullptr); + } else { + // Allocate a holding position for the layer object (copyTo will produce, copyFrom will consume) + OffscreenBuffer** layerHandle = mAllocator.create(nullptr); - /** - * First, defer an operation to copy out the content from the rendertarget into a layer. - */ - auto copyToOp = mAllocator.create_trivial(op, layerHandle); - BakedOpState* bakedState = BakedOpState::directConstruct(mAllocator, - &(currentLayer().viewportClip), dstRect, *copyToOp); - currentLayer().deferUnmergeableOp(mAllocator, bakedState, OpBatchType::CopyToLayer); + /** + * First, defer an operation to copy out the content from the rendertarget into a layer. + */ + auto copyToOp = mAllocator.create_trivial(op, layerHandle); + BakedOpState* bakedState = BakedOpState::directConstruct(mAllocator, + &(currentLayer().repaintClip), dstRect, *copyToOp); + currentLayer().deferUnmergeableOp(mAllocator, bakedState, OpBatchType::CopyToLayer); - /** - * Defer a clear rect, so that clears from multiple unclipped layers can be drawn - * both 1) simultaneously, and 2) as long after the copyToLayer executes as possible - */ - currentLayer().deferLayerClear(dstRect); + /** + * Defer a clear rect, so that clears from multiple unclipped layers can be drawn + * both 1) simultaneously, and 2) as long after the copyToLayer executes as possible + */ + currentLayer().deferLayerClear(dstRect); - /** - * And stash an operation to copy that layer back under the rendertarget until - * a balanced EndUnclippedLayerOp is seen - */ - auto copyFromOp = mAllocator.create_trivial(op, layerHandle); - bakedState = BakedOpState::directConstruct(mAllocator, - &(currentLayer().viewportClip), dstRect, *copyFromOp); - currentLayer().activeUnclippedSaveLayers.push_back(bakedState); + /** + * And stash an operation to copy that layer back under the rendertarget until + * a balanced EndUnclippedLayerOp is seen + */ + auto copyFromOp = mAllocator.create_trivial(op, layerHandle); + bakedState = BakedOpState::directConstruct(mAllocator, + &(currentLayer().repaintClip), dstRect, *copyFromOp); + currentLayer().activeUnclippedSaveLayers.push_back(bakedState); + } } void FrameBuilder::deferEndUnclippedLayerOp(const EndUnclippedLayerOp& /* ignored */) { LOG_ALWAYS_FATAL_IF(currentLayer().activeUnclippedSaveLayers.empty(), "no layer to end!"); BakedOpState* copyFromLayerOp = currentLayer().activeUnclippedSaveLayers.back(); - currentLayer().deferUnmergeableOp(mAllocator, copyFromLayerOp, OpBatchType::CopyFromLayer); currentLayer().activeUnclippedSaveLayers.pop_back(); + if (copyFromLayerOp) { + currentLayer().deferUnmergeableOp(mAllocator, copyFromLayerOp, OpBatchType::CopyFromLayer); + } } } // namespace uirenderer diff --git a/libs/hwui/LayerBuilder.cpp b/libs/hwui/LayerBuilder.cpp index c5d7191f1b624..bc39621f2cb21 100644 --- a/libs/hwui/LayerBuilder.cpp +++ b/libs/hwui/LayerBuilder.cpp @@ -199,10 +199,10 @@ LayerBuilder::LayerBuilder(uint32_t width, uint32_t height, : width(width) , height(height) , repaintRect(repaintRect) + , repaintClip(repaintRect) , offscreenBuffer(renderNode ? renderNode->getLayer() : nullptr) , beginLayerOp(beginLayerOp) - , renderNode(renderNode) - , viewportClip(Rect(width, height)) {} + , renderNode(renderNode) {} // iterate back toward target to see if anything drawn since should overlap the new op // if no target, merging ops still iterate to find similar batch to insert after @@ -260,7 +260,7 @@ void LayerBuilder::flushLayerClears(LinearAllocator& allocator) { Matrix4::identity(), nullptr, paint, verts, vertCount); BakedOpState* bakedState = BakedOpState::directConstruct(allocator, - &viewportClip, bounds, *op); + &repaintClip, bounds, *op); deferUnmergeableOp(allocator, bakedState, OpBatchType::Vertices); } } diff --git a/libs/hwui/LayerBuilder.h b/libs/hwui/LayerBuilder.h index 99968e1750c8f..4a7ca2de9b1b5 100644 --- a/libs/hwui/LayerBuilder.h +++ b/libs/hwui/LayerBuilder.h @@ -109,10 +109,10 @@ public: const uint32_t width; const uint32_t height; const Rect repaintRect; + const ClipRect repaintClip; OffscreenBuffer* offscreenBuffer; const BeginLayerOp* beginLayerOp; const RenderNode* renderNode; - const ClipRect viewportClip; // list of deferred CopyFromLayer ops, to be deferred upon encountering EndUnclippedLayerOps std::vector activeUnclippedSaveLayers; diff --git a/libs/hwui/tests/unit/FrameBuilderTests.cpp b/libs/hwui/tests/unit/FrameBuilderTests.cpp index f49dd3f6bc818..f86898fd669ad 100644 --- a/libs/hwui/tests/unit/FrameBuilderTests.cpp +++ b/libs/hwui/tests/unit/FrameBuilderTests.cpp @@ -651,6 +651,62 @@ TEST(FrameBuilder, saveLayerUnclipped_mergedClears) { << "Expect 4 copyTos, 4 copyFroms, 1 clear SimpleRects, and 1 rect."; } +TEST(FrameBuilder, saveLayerUnclipped_clearClip) { + class SaveLayerUnclippedClearClipTestRenderer : public TestRendererBase { + public: + void onCopyToLayerOp(const CopyToLayerOp& op, const BakedOpState& state) override { + EXPECT_EQ(0, mIndex++); + } + void onSimpleRectsOp(const SimpleRectsOp& op, const BakedOpState& state) override { + EXPECT_EQ(1, mIndex++); + ASSERT_NE(nullptr, op.paint); + EXPECT_EQ(SkXfermode::kClear_Mode, PaintUtils::getXfermodeDirect(op.paint)); + EXPECT_EQ(Rect(50, 50, 150, 150), state.computedState.clippedBounds) + << "Expect dirty rect as clip"; + ASSERT_NE(nullptr, state.computedState.clipState); + EXPECT_EQ(Rect(50, 50, 150, 150), state.computedState.clipState->rect); + EXPECT_EQ(ClipMode::Rectangle, state.computedState.clipState->mode); + } + void onRectOp(const RectOp& op, const BakedOpState& state) override { + EXPECT_EQ(2, mIndex++); + } + void onCopyFromLayerOp(const CopyFromLayerOp& op, const BakedOpState& state) override { + EXPECT_EQ(3, mIndex++); + } + }; + + auto node = TestUtils::createNode(0, 0, 200, 200, + [](RenderProperties& props, RecordingCanvas& canvas) { + // save smaller than clip, so we get unclipped behavior + canvas.saveLayerAlpha(10, 10, 190, 190, 128, (SaveFlags::Flags)(0)); + canvas.drawRect(0, 0, 200, 200, SkPaint()); + canvas.restore(); + }); + + // draw with partial screen dirty, and assert we see that rect later + FrameBuilder frameBuilder(sEmptyLayerUpdateQueue, SkRect::MakeLTRB(50, 50, 150, 150), 200, 200, + TestUtils::createSyncedNodeList(node), sLightGeometry, nullptr); + SaveLayerUnclippedClearClipTestRenderer renderer; + frameBuilder.replayBakedOps(renderer); + EXPECT_EQ(4, renderer.getIndex()); +} + +TEST(FrameBuilder, saveLayerUnclipped_reject) { + auto node = TestUtils::createNode(0, 0, 200, 200, + [](RenderProperties& props, RecordingCanvas& canvas) { + // unclipped savelayer + rect both in area that won't intersect with dirty + canvas.saveLayerAlpha(100, 100, 200, 200, 128, (SaveFlags::Flags)(0)); + canvas.drawRect(100, 100, 200, 200, SkPaint()); + canvas.restore(); + }); + + // draw with partial screen dirty that doesn't intersect with savelayer + FrameBuilder frameBuilder(sEmptyLayerUpdateQueue, SkRect::MakeWH(100, 100), 200, 200, + TestUtils::createSyncedNodeList(node), sLightGeometry, nullptr); + FailRenderer renderer; + frameBuilder.replayBakedOps(renderer); +} + /* saveLayerUnclipped { saveLayer { saveLayerUnclipped { rect } } } will play back as: * - startTemporaryLayer, onCopyToLayer, onSimpleRects, onRect, onCopyFromLayer, endLayer * - startFrame, onCopyToLayer, onSimpleRects, drawLayer, onCopyFromLayer, endframe