[BlinkGenPropertyTrees] compositing/overflow/nested-render-surfaces-with-rotation.html is failing |
|||||||
Issue descriptionThis test is failing when run with --enable-blink-gen-property-trees due to incorrect clipping: virtual/prefer_compositing_to_lcd_text/compositing/overflow/nested-render-surfaces-with-rotation.html
,
Sep 14
I'm leaving the team, thus re-assigning.
,
Sep 17
,
Sep 17
,
Sep 17
Reduced test case:
<!DOCTYPE html>
<div style="width: 300px; height: 300px; background: blue; overflow: hidden">
<div style="transform: rotate(45deg); opacity: 0.5">
<div style="width: 300px; height: 300px; background: yellow; overflow: hidden; will-change: transform">
</div>
</div>
</div>
,
Sep 18
I'm trying the #c2 method. It can create almost correct results, but also creates some bad effects along the edge of the layers like extra blurring, background bleeding, sometimes more pixelating. Still debugging.
,
Sep 18
I just tried a simple method by just setting the output_clip of the new effect node to context_.current.clip in PaintPropertyTreeBuilder::UpdateEffect(), and it seems to work well. Though this will break interleaving clip/effect, but this proves that simply setting output_clip works in non-interleaving cases without the extra mask layers. Will investigate how to make this work for both interleaving and non-interleaving effects/clips.
,
Sep 19
By comparing the cc property trees and cc layers produced with and without explicit output clip, I think the bad effects along the edge of layers are caused by the order of applying the clip mask and the opacity effect (which causes bleeding), especially when they are in different transform spaces (which causes blurriness). Looking into the details of the cc property trees and the cc layers, and trying to adjust the order. Perhaps the border radius test failures are also related to this.
,
Sep 21
Another test case. Not sure if this is directly related, but the result is really bad and it needs a fix:
<!DOCTYPE html>
<div style="margin: 100px; transform: rotate(45deg); overflow: hidden;
width: 200px; height: 200px; border: 20px solid green; overflow: hidden">
<div style="transform: translateX(-50px); will-change: transform;
width: 400px; height: 400px; background: red">
</div>
</div>
</div>
,
Sep 21
The following test case shows 1px clip error that causes failure of compositing/overflow/nested-render-surfaces-with-rotation.html (the left border of the clipping container is covered by the content):
<!DOCTYPE html>
<div style="margin: 100px; width: 200px; transform: rotate(45deg); opacity: 0.9">
<div style="width: 200px; height: 200px; border: 1px solid black; overflow: scroll">
<div style="transform: translateX(-10px); will-change: transform">
<div style="width: 300px; height: 300px; background: yellow"></div>
</div>
</div>
</div>
,
Sep 26
Just found that the output clip CL (https://chromium-review.googlesource.com/c/chromium/src/+/1239418) fixed the overflowing bug of virtual/prefer_compositing_to_lcd_text/compositing/overflow/nested-render-surfaces-with-rotation.html: the rotated contents no longer overflow the clips in large. There is just 1 pixel overflow on the left edge of the inner clip, which can be reproduced with the test case in #c10. https://chromium-review.googlesource.com/c/chromium/src/+/1244104 (WIP) will fix the failure of the test case in #c9. Not sure if the output clip CL can fix all rotated clip issues with rotated clips except the cases in #c9 and #c10.
,
Sep 26
We still have overflowing problem in the following case:
<!DOCTYPE html>
<div style="width: 300px; height: 300px; background: blue; overflow: hidden">
<div style="transform: rotate(45deg)">
<div style="opacity: 0.5">
<div style="width: 300px; height: 300px; background: red; overflow: hidden; will-change: transform">
<div style="position: fixed"></div>
</div>
</div>
</div>
</div>
It's simplified based on compositing/overflow/nested-render-surfaces-with-rotation.html by
1. forcing the inner clipping container composited;
2. separating transform and opacity divs between the outer and inner clipping containers.
The output clip CL fixed virtual/prefer_compositing_to_lcd_text/compositing/overflow/nested-render-surfaces-with-rotation.html because the parent of the inner clipping container has both transform and opacity which contains the fixed position descendant thus has explicit output clip.
,
Sep 26
The 1px clip error issue is interesting. For example,
<!DOCTYPE html>
<div id="a" style="margin: 3px; width: 200px; transform: rotate(0.12deg); opacity: 0.9">
<div id="b" style="width: 200px; height: 200px; border: 1px solid black; will-change: transform; overflow: hidden">
<div id="c" style="transform: translateX(-1px); will-change: transform; width: 300px; height: 300px; background: yellow"></div>
</div>
</div>
the left edge of the clip has 1px error to the left if I use the above margin and rotate values, but is good if I change the margin to 4px, or the rotate to 0.11deg. There seems some error of pixel snapping, but I can't find it from the diffs between the verbose outputs of BGPT and non-BGPT. For non-BGPT, the error might also exists but is hidden by the child containment layer that BGPT doesn't have.
,
Sep 26
More interesting observations of the 1px clip error issue:
The following test case shows that the clip error is actually 1 physical pixel instead of css pixel:
<!DOCTYPE html>
<div style="transform: scale(5); transform-origin: 0 0">
<div id="a" style="margin: 5px; transform: rotate(0.12deg); opacity: 0.9">
<div id="b" style="width: 100px; height: 100px; border: 1px solid black; will-change: transform; overflow: hidden">
<div id="c" style="outline: 2px solid red; will-change: transform; width: 300px; height: 300px; background: yellow"></div>
</div>
</div>
</div>
In devtools, using keyboard up/down arrow to adjust the margin value of "a", you'll see the red pixels appear/disappear "randomly".
I guess that the "random" 1 pixel overflow is caused by cc impl-side code because it is physical.
There seems also some kind of under invalidation when the margin changes between 1px and 11px.
,
Sep 26
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/534a77d736cc20e640ba713c5e5eca40bace6366 commit 534a77d736cc20e640ba713c5e5eca40bace6366 Author: Xianzhu Wang <wangxianzhu@chromium.org> Date: Wed Sep 26 23:06:53 2018 More cases of explicit output clip of effect nodes Now exclude cases of using null output clip for an effect node if the out-of-flow descenant's containing block has the same clip as the current clip. This lets PaintChunksToCcLayers and PropertyTreeManager go through the more optimized path for more cases. Bug: 879173 Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel Change-Id: I5c9e0006dc678c36b752823293f08355a194578b Reviewed-on: https://chromium-review.googlesource.com/1246686 Reviewed-by: Philip Rogers <pdr@chromium.org> Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org> Cr-Commit-Position: refs/heads/master@{#594512} [add] https://crrev.com/534a77d736cc20e640ba713c5e5eca40bace6366/third_party/WebKit/LayoutTests/compositing/overflow/clip-rotate-opacity-fixed-expected.html [add] https://crrev.com/534a77d736cc20e640ba713c5e5eca40bace6366/third_party/WebKit/LayoutTests/compositing/overflow/clip-rotate-opacity-fixed.html [modify] https://crrev.com/534a77d736cc20e640ba713c5e5eca40bace6366/third_party/blink/renderer/core/paint/paint_property_tree_builder.cc [modify] https://crrev.com/534a77d736cc20e640ba713c5e5eca40bace6366/third_party/blink/renderer/core/paint/paint_property_tree_builder_test.cc
,
Sep 26
I just talked to enne about #14 and they think it could be a skia bug. Can you get the skpictures with and without the red line and compare?
,
Sep 26
#c15 CL creates output clip for #c12 case and fixes it for BGPT. Remaining issues are: 1. 1px clip error at top or left (test cases in #c10, #c13 and #c14). 2. wrong clip on interleaving effect and clip with rotated transforms (no test case for now. All other wrong clips about effect and rotated clip have been fixed by the output clip CLs) 3. wrong clip for the simple case in #c9 due to missing effect node and render surface. The remaining cases either fail with minor visual effect (1 physical pixel) or are rare cases (not tested by existing layout tests). Still investigating.
,
Sep 26
Re #c16: the skpictures in the picture layers/tiles are all the same in non-BGPTand BGPT, and look correct. The clip is outside of the layers. I'm trying to find where the clip is applied.
,
Sep 26
The problem is at https://cs.chromium.org/chromium/src/cc/trees/draw_property_utils.cc?rcl=baa0cf415e9bf820e21841745e1cf01c72860e83&l=919 where a clip rect like 0.999999,1.000000 100.000000x100.000000 is converted to 0,1 101x100. Though the above issue is just some computation error about integral clips, I wonder how cc should deal with non-integral clips. If a non-integral clip is expanded to integer, the edge pixels will mistakenly fully cover the underlying surface's pixels. Is child containment layer for the purpose? If yes, we should have some counterpart (which seems missing for now) in BGPT.
,
Oct 2
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/296fedf8021a31cf0afa3e07409a460c4512d99e commit 296fedf8021a31cf0afa3e07409a460c4512d99e Author: Xianzhu Wang <wangxianzhu@chromium.org> Date: Tue Oct 02 19:30:42 2018 [PE] Ignore floating point error for clip rects A clip rect mapped through multiple transform spaces can be (1.99999, 2.00001, 10.999998, 20.000002). Previously we use ToEnclosingRect to convert it to an integral rect and will result a rect that has 1 pixel bigger in some directions, causing incorrect clipping result. Now add gfx::ToEnclosingIgnoringError to convert a near integral gfx::RectF to the nearest integral gfx::Rect. Bug: 879173 Cq-Include-Trybots: luci.chromium.try:linux-blink-gen-property-trees;luci.chromium.try:linux_layout_tests_slimming_paint_v2;luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel Change-Id: Id7ab68c04f8f1efed1dfe1810b82dd24d1a41ea5 Reviewed-on: https://chromium-review.googlesource.com/1252143 Reviewed-by: Ian Vollick <vollick@chromium.org> Reviewed-by: Ali Juma <ajuma@chromium.org> Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org> Cr-Commit-Position: refs/heads/master@{#595947} [modify] https://crrev.com/296fedf8021a31cf0afa3e07409a460c4512d99e/cc/trees/draw_property_utils.cc [modify] https://crrev.com/296fedf8021a31cf0afa3e07409a460c4512d99e/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-gen-property-trees [modify] https://crrev.com/296fedf8021a31cf0afa3e07409a460c4512d99e/ui/gfx/geometry/rect_conversions.cc [modify] https://crrev.com/296fedf8021a31cf0afa3e07409a460c4512d99e/ui/gfx/geometry/rect_conversions.h [modify] https://crrev.com/296fedf8021a31cf0afa3e07409a460c4512d99e/ui/gfx/geometry/rect_unittest.cc
,
Oct 2
|
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by pdr@chromium.org
, Aug 30