Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@knopp
Copy link
Member

@knopp knopp commented Jun 11, 2022

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

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@zanderso zanderso requested review from flar and jonahwilliams June 11, 2022 14:11
Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jonahwilliams jonahwilliams added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jun 13, 2022
@fluttergithubbot fluttergithubbot merged commit 7aa8484 into flutter:main Jun 13, 2022
@flar
Copy link
Contributor

flar commented Jun 13, 2022

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 correct best solution.

@knopp
Copy link
Member Author

knopp commented Jun 13, 2022

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?

@jonahwilliams
Copy link
Contributor

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.

@flar
Copy link
Contributor

flar commented Jun 13, 2022

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.

@flar
Copy link
Contributor

flar commented Jun 13, 2022

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?

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.

@knopp
Copy link
Member Author

knopp commented Jun 13, 2022

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?

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 #ifndef SUPPORT_FRACTIONAL_TRANSLATION :)

@jonahwilliams
Copy link
Contributor

The issue for that is flutter/flutter#103909 😄

@flar
Copy link
Contributor

flar commented Jun 13, 2022

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.

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 13, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 13, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 13, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 14, 2022
@knopp knopp deleted the layers_with_raster_cache_use_integer_ctm branch June 14, 2022 17:03
houhuayong pushed a commit to houhuayong/engine that referenced this pull request Jun 21, 2022
CaseyHillers pushed a commit to CaseyHillers/engine that referenced this pull request Jun 29, 2022
jonahwilliams pushed a commit to jonahwilliams/engine that referenced this pull request Jun 29, 2022
jonahwilliams pushed a commit to jonahwilliams/engine that referenced this pull request Jun 29, 2022
jonahwilliams pushed a commit to jonahwilliams/engine that referenced this pull request Jun 29, 2022
jonahwilliams pushed a commit to jonahwilliams/engine that referenced this pull request Jun 29, 2022
CaseyHillers pushed a commit that referenced this pull request Jun 30, 2022
* All layers with raster cache should use integer CTM (#33981)

* Update checkerboard_layertree_unittests.cc

Co-authored-by: Matej Knopp <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ImageFiltered Render Issue

4 participants