dont pixel snap raster cache if SUPPORT_FRACTIONAL_TRANSLATION is enabled #34002

jonahwilliams · 2022-06-13T18:05:25Z

Required to support flutter/flutter#103909

When drawing from the raster cache, the engine unconditionally snaps to physical pixels. If we want to enable SUPPORT_FRACTIONAL_TRANSLATION , this will be incorrect.

Remove the round out when SUPPORT_FRACTIONAL_TRANSLATION , change several APIs that accept SKIRect to SkRect

…bled

dnfield

LGTM but I'd give @flar a chance to look at this too before landing.

jonahwilliams · 2022-06-13T19:12:47Z

I think this is the last piece before we can start opting in the desktop targets, after the next flutter roll allows me to "fix" the last google3 test that fails without pixel snapping

flar · 2022-06-13T20:49:30Z

flow/raster_cache.h

-    return bounds;
+    return SkRect::MakeLTRB(bounds.fLeft, bounds.fTop, bounds.fRight,
+                            bounds.fBottom);
+#else


These 2 branches share a few lines of code so the ifndef could be reduced a bit.

Even more because I believe that SkRect has a "round out to SkRect" method which can be executed on "itself" which means only 1 line of code needs to be inside the ifndef.

But the math isn't really complete here.

When computing the size of the cache image we need something along the lines of roundOut, but it isn't being done. More comments in the Rasterize method...

Perhaps this method should perform no roundOut in any case and let the calling method figure out the right way to handle non-integer bounds?

flar · 2022-06-13T20:53:49Z

flow/raster_cache.cc

  TRACE_EVENT0("flutter", "RasterCachePopulate");
-  SkIRect cache_rect = RasterCache::GetDeviceBounds(logical_rect, ctm);
+
+  SkRect cache_rect = RasterCache::GetDeviceBounds(logical_rect, ctm);


Below we pass width/height to MakeN32Premul. That method takes an integer width/height. I'm not sure why that is compiling, is it performing a silent conversion? Using what method? floor? round? ceil?

We have a couple of options here:

roundOut and use those bounds and offset

ceil the width and height and use the sub-pixel offset to render into that cache image

something where we store the sub-pixel rounding that was used for the offset so that we can adjust the location of the cache image properly in Draw?

Ahh shoot, I didn't check and assumed this took an SkScalar. Its probably just truncating?

I think we should probably round out to get the size of the texture, and then when drawing it use the original float offset, right?

flar · 2022-06-13T21:59:56Z

flow/raster_cache.h

    SkIRect bounds;
    device_rect.roundOut(&bounds);
    return SkRect::MakeLTRB(bounds.fLeft, bounds.fTop, bounds.fRight,
                            bounds.fBottom);


This code is equivalent to device_rect.roundOut(&deviceRect); return device_rect; which makes the return statements the same in both cases.

Basically, I believe that the only modification to this method needed is:

#ifndef ... device_rect.roundOut(&device_rect); #endif

flar

I believe this change is now compatible with our current implementation, but I mention a few opportunities for improvement.

One suggestion is simply reducing the lines of code.

The other suggestion is just pointing out that the existing code (and this patch) both do the rounding inaccurately in the case of IntCTM, but since that isn't a regression from the current code, that potential change should probably be left to its own PR so that it can be reverted separately from this change if we get it wrong.

flar · 2022-06-13T22:09:05Z

flow/raster_cache.cc

  SkCanvas* canvas = surface->getCanvas();
  canvas->clear(SK_ColorTRANSPARENT);
-  canvas->translate(-cache_rect.left(), -cache_rect.top());
+  canvas->translate(-dest_rect.left(), -dest_rect.top());


We still have a rounding problem when using ICTMs, but it is the same rounding problem that we are already living with.

I describe the current rounding problem here: #33981 (comment)

flar · 2022-06-13T22:09:14Z

flow/raster_cache.cc

+  SkRect dest_rect = RasterCache::GetDeviceBounds(logical_rect, ctm);
+  // we always round out here so that the texture is integer sized.
+  SkIRect cache_rect;
+  dest_rect.roundOut(&cache_rect);


This can choose a cache image size that is 1 pixel larger than needed in the non-ICTM case because our translation will be by dest_rect.left/top which may move it closer to the origin of the cache image by enough to not need that last pixel on the right and bottom. A minor issue, but could be fixed by using the ceil of the dest_rect width and height instead of the height of the cache_rect.

With ICTM, dest_rect is all integers anyway so the ceil of its w/h are just the w/h.

Without ICTM, the w/h indicate the number of pixels from the left/topmost coordinate in the drawing which are mapped to 0,0 in the cache image by the translate below and so the ceil of the w/h represent the number of whole pixels which it will take up.

Ahh good point, fixed

…N is enabled (flutter/engine#34002)

…bled (flutter#34002)

dont pixel snap raster cache if SUPPORT_FRACTIONAL_TRANSLATION is ena…

4872807

…bled

This comment was marked as outdated.

Sign in to view

flutter-dashboard bot added the needs tests label Jun 13, 2022

Jonah Williams added 3 commits June 13, 2022 11:06

Merge branch 'main' into no_snap_raster

da0b45d

++

e17f18e

++

538f398

jonahwilliams removed the needs tests label Jun 13, 2022

jonahwilliams requested a review from dnfield June 13, 2022 18:33

dnfield requested a review from flar June 13, 2022 18:45

dnfield approved these changes Jun 13, 2022

View reviewed changes

++

ac902b1

flar suggested changes Jun 13, 2022

View reviewed changes

round for for texture size

292dbed

jonahwilliams requested a review from flar June 13, 2022 21:37

flar reviewed Jun 13, 2022

View reviewed changes

flar approved these changes Jun 13, 2022

View reviewed changes

Jonah Williams added 2 commits June 13, 2022 15:17

++

d1598e8

Ceil on width/height

6e86adb

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 merged commit edc2aa4 into flutter:main Jun 13, 2022

engine-flutter-autoroll mentioned this pull request Jun 14, 2022

Roll Flutter Engine from 6670f92130d9 to edc2aa4e894b (14 revisions) flutter/flutter#105934

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 14, 2022

edc2aa4 dont pixel snap raster cache if SUPPORT_FRACTIONAL_TRANSLATIO…

4ca884b

…N is enabled (flutter/engine#34002)

jonahwilliams deleted the no_snap_raster branch June 14, 2022 01:25

houhuayong pushed a commit to houhuayong/engine that referenced this pull request Jun 21, 2022

dont pixel snap raster cache if SUPPORT_FRACTIONAL_TRANSLATION is ena…

0f482c2

…bled (flutter#34002)

dont pixel snap raster cache if SUPPORT_FRACTIONAL_TRANSLATION is enabled #34002

dont pixel snap raster cache if SUPPORT_FRACTIONAL_TRANSLATION is enabled #34002

Uh oh!

Conversation

jonahwilliams commented Jun 13, 2022

Uh oh!

This comment was marked as outdated.

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Jun 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants