-
Notifications
You must be signed in to change notification settings - Fork 6k
All layers with raster cache should use integer CTM #33981
All layers with raster cache should use integer CTM #33981
Conversation
jonahwilliams
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
This fix was merged before I had a chance to respond. I disagree with the assessment above. I agree that we had a bounds consistency issue that needed to be fixed, but I disagree that applying the Integer CTM in all cases was the |
|
When would this not be a correct solution? Given that raster cache is always painted on integral coordinates, and that during diff phase it's not known whether raster cache is used, it seems to me that the only way to have consistency between passes is to always snap to integers. Am I missing something here? |
|
The long term solution here feels like we shouldn't pixel snap, but for correctness I don't understand how we could avoid the pixel snap in the short term. Inconsistency between where the raster cached image is drawn versus the original image is also a source of fidelity issues that we need to account for. |
|
Another way to have consistency is to pad the bonds to account for the discrepancy. Integer CTM is a cheap solution, but we're leaving fidelity on the floor when we use it everywhere. I think I see what is causing the cache draws to happen at an integer location and it's another thing that I think needs to be fixed about the cache drawing. The same method is used for computing the bounds of the cache image as is used for considering where to place it. sub-pixel precision can be maintained in the latter without forcing Integer CTM, but it isn't accounted for currently and we slam the cache entry to a integer position to account for what could be considered bad math here. For dlPicture and skPicture layers for which it was written these things don't matter because those layers were using Integer CTM for performance anyway. But, when we started using it for other layers that transform their cache images before they reach the screen, these deficiencies were no longer appropriate, but they were inherited from the conditions that only apply to the first 2 layers that used caching. As your comment that just popped up as I was writing this says, perhaps this is too big a ball of yarn to unravel for now and we should just go with the easy fix (Integer CTM) for now and devise a longer term plan for getting rid of CTM slamming or doing it with more fidelity. |
I changed the wording in that first comment to "best". It solves the problem. It has drawbacks, but the problem went away so "correct" is a relative term here. |
Once raster cache can paint on sub-pixel coordinates I'll be more than happy to provide PR that removes all traces of |
|
The issue for that is flutter/flutter#103909 😄 |
|
But it isn't just whether or not RC can paint on non-integer bounds - we are rounding poorly! We aren't even doing integer bounds right, but we keep relying on it. Consider how we determine the sub-pixel position of the rendering and how we correlate that with its final rendering position. It's only really a good match for the original CTM under which it was cached and integer offsets from that CTM. Subpixel offsets from the original CTM move the location of the pixels inconsistently with how the item would have been rendered. Normally you'd want the cache to show the pixels +/- 0.5 pixels off, but the way the computation is done we're showing it +/- a whole pixel off. Consider a rendering that transforms to (X1 + .99)=>(X2). We rendering that starting from 0 which means it was only .01 pixels from moving to a new location before it was snap shotted. Now we render it with a .01 offset and it maps to (X1 +1) and so we render the cached image at (X1+1). The contents jumped an entire pixel just as soon as it started moving and stayed there until we reached an offset of 1.01. This is all bad math either way. I suppose maybe it is consistent, but it is just bad rounding all the way around. |
* All layers with raster cache should use integer CTM (#33981) * Update checkerboard_layertree_unittests.cc Co-authored-by: Matej Knopp <[email protected]>
Layers that use raster cache should be painted at identical position regardless of whether they're raster cached or not. Otherwise the position calculated during Diff pass may be subtly wrong resulting in artefacts.
Also because rater cached layer is currently always painted on pixel boundaries, any layer using raster cache should snap the CTM to integer when painting uncached children to match the behavior.
Fixes flutter/flutter#105674
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
Pre-launch Checklist
writing and running engine tests.
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.