Started https://pinpoint-dot-chromeperf.appspot.com/job/16ebc832c40000 to capture traces of r543290 and r543291, with "Frame Viewer" tracing categories (blink,cc,gpu,renderer.scheduler,v8,toplevel,disabled-by-default-cc.debug,disabled-by-default-cc.debug.picture,disabled-by-default-cc.debug.display_items).
Reduced test case, based on http://jsfiddle.net/3yDKh/16. Needs --enable-prefer-compositing-to-lcd-text on low-dpi machine to reproduce.
Ideally, none of the moving elements should be invalidated because they are composited. On SPv1, we invalidate the first box (whose margin-top changes). On SPv175, we invalidate all of the boxes.
The over-invalidation is caused by paint-offset change in sub-pixels.
For SPv1, we calculate visual rect in backing object coordinates which doesn't include backing object's sub-pixel accumulation. We add sub-pixel accumulation to the visual rect when we issue raster invalidation, and sub-pixel accumulation change of a composited layer doesn't trigger raster invalidation.
For SPv175, composited layer's sub-pixel accumulation is baked in paint offset of all descendants (unless the subpixel accumulation is discarded because of complex transform, etc.), so change of sub-pixel accumulation of the composited layer will cause paint offset change of all descendants.
I'm not sure the correct/best way to handle paint offset change in sub-pixels. Fully invalidate (which is the current status of SPv175) is not acceptable because that slows down every movement animation of composited layers. Can we just ignore it for composited layers? It seems that change of sub-pixel accumulation can cause change of visual size of an sub-pixel sized object due to top-left corner and bottom-right corner can be pixel-snapped in different directions. However, this seems actually ignored for composited layers in SPv1. Perhaps we can just let SPv175 behaves like SPv1 at first?
Wdyt?
Cc: alancutter@chromium.org Summary: [SPv175] Repaint storm during animation with subpixel lengths (was: thread_GPU_cpu_time_per_frame seems like a real spv175 regression)
Discussed the issue with trchen@ on Friday. The root cause seems that animation in subpixels is incompatible with pixel snapping, for composited animation. For example, a box with subpixel location and size can change pixel-snapped size when animating its location with subpixels, causing compositing-only animation not applicable.
Thinking of the following solutions:
1. Fix the root cause(?): round some interpolated values during animation.
Pros:
The root cause?
Cons:
Needs to change css animation code
What if the start and end values are with subpixels?
Doesn't work for javascript animations
2. Like SPv1, accept imperfection of pixel-snapping for composited layers with subpixel accumulation. That is, ignore pixel-snapped size change of a composited layer contents during animation:
- visual rects are calculated with composited layer's subpixel accumulation excluded;
- don't call EnclosingIntRect in PaintInvalidator::MapLocalRectToVisualRectInBacking() (unless some special cases)
- add composited layer's subpixel accumulation to visual rects when we issue raster invalidations and send the visual rects to cc.
Pros:
Compatible with SPv1
Can fix the regression quickly
Cons (all existing in SPv1):
Imperfection of pixel-snapping
Needs to expose subpixel accumulation to platform.
I'm inclined to approach 2. Wdyt?
We still need to find a good solution for SPv2.
I think equivalent CSS and Javascript animations invoke identical layout and paint behaviour when possible so I'm in favour of option 2 despite not fully understanding the paint concepts involved.
Also: compositing and subpixel positioning are fundamentally at odds. This is because
compositing is a texture caching strategy. "Subpixel" positioning is a technique
to simulate some aspects of higher resolution screens by blending and rounding in clever
ways. Invalidating the texture and re-computing optimal positions on subpixel change
works, but that defeats the performance advantage of compositing. This is the reason
for the special-case code mentioned in comment 12.
This is the same class of issue that leads to performance/quality tradeoffs such as that
involved in the discussions of the behavior of will-change: transform w.r.t. raster
behavior.
However, note that there are multiple fast paths here:
(1) change of composited transform
(2) change of composited layer subpixel position
The code linked to in comment 12 is for use-case (2).
I had read #c12 code before making https://chromium-review.googlesource.com/c/chromium/src/+/1023606 but didn't notice it's limited direct composited layers. Is it to avoid the chicken-and-egg issue or we just want to limit to direct composited layers? My CL can be simplified if we still limit to direct composited layers.
Re #c16 No. I think a whole blurry layer is much worse than some non-noticeable pixel-snapping differences of sub-pixel-sized boxes in the sub-pixel-positioned composited layer.
Oh right, now I recall that subpixel accumulation is explicitly excluded from the paint offset transform.
Anyway, I think comment 9 option (2), restricted to directly composited, would be good.
This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop)
For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
About the CLs:
- r553479: the original CL which caused null pointer crashes;
- r554065: revert of r553479
- r554079: reland of r553479 with the null pointer issue fixed.
Only r554079 needs merge into M67.
Justifications:
- The bug is a major performance regression of SlimmingPaintV175 (enabled in M67). The CL will fix the regression.
- The CL looks big, but many lines are just for plumbing and for limiting the real functional code to safe cases only. Compared to r553479, r554079 also adds code to avoid possible failing cases.
- The same mechanism has been verified in SlimmingPaintV1 (before M67) in many milestones.
Comment 1 by wangxianzhu@chromium.org
, Apr 14 2018