Bug 76658 – [chromium] Partially filled pixels do not occlude pixels below them.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dana Jansens]

[chromium] Partially filled pixels do not occlude pixels below them.

[Dana Jansens]

Created attachment 123181 [details]
Patch

Quads are being marked as occluding all the area under their transformed quad rect, but the area is being rounded /out/ to the nearest ints. This is bad because it includes partial pixels not covered by the quad.

Includes a layout test that demonstrates the issue and fails with the current code. Solution is to add a rect to the Region that rounded /in/ from the transformed quad rect. (Or to make a FloatRegion one day.)

I'd like to stick the enclosedIntRect() into FloatRect.cpp if that would be appropriate/reviewable since it's used anywhere that tracks or deals with transformed rects and accumulated opaque areas.

[Dana Jansens]

Created attachment 123183 [details]
Patch

Fail at rebasing. Better version.

[Adrienne Walker]

Comment on attachment 123181 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123181&action=review

Is this hitting the case where the layer is an axis aligned int rect, but there's some scale on the contents that causes quads to be on non-integer boundaries?

> Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:94
> +            opaqueCoverageThusFar.unite(enclosedIntRect(quadRect));

quadRect is an IntRect.  Did you mean to apply enclosedIntRect to the result of mapRect?

[Dana Jansens]

(In reply to comment #3)
> (From update of attachment 123181 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123181&action=review
> 
> Is this hitting the case where the layer is an axis aligned int rect, but there's some scale on the contents that causes quads to be on non-integer boundaries?

Yes exactly.

> > Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:94
> > +            opaqueCoverageThusFar.unite(enclosedIntRect(quadRect));
> 
> quadRect is an IntRect.  Did you mean to apply enclosedIntRect to the result of mapRect?

Yeh that was the rebase fail.

[James Robinson]

Comment on attachment 123183 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123183&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:67
> +    float left = ceilf(rect.x());
> +    float top = ceilf(rect.y());

most rect logic in WebKit tries to stick to x/y/maxX/maxY instead of left/top

> Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:69
> +    float width = std::max(0.0f, floorf(rect.maxX()) - left);

WK style is to add a 'using namespace std' near the top of the file and then make this call max()

if you call it as max<float>() then you can have 0 be the first parameter

does this do the right thing when the whole rectangle is in the -x/-y quadrant?

> LayoutTests/compositing/culling/tile-occlusion-boundaries.html:9
> +      background-color:red;

we try to construct pixel tests such that green mean pass and red means fail, can you tweak this so you see only green if it passes and something else if it fails?

[Dana Jansens]

Created attachment 123193 [details]
Patch

For jamesr review

Please see comment #1 re: FloatRect.cpp

[Dana Jansens]

Sorry collision with your comments, will address them first.

[Dana Jansens]

(In reply to comment #5)
> (From update of attachment 123183 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123183&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:67
> > +    float left = ceilf(rect.x());
> > +    float top = ceilf(rect.y());
> 
> most rect logic in WebKit tries to stick to x/y/maxX/maxY instead of left/top

done.

> > Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:69
> > +    float width = std::max(0.0f, floorf(rect.maxX()) - left);
> 
> WK style is to add a 'using namespace std' near the top of the file and then make this call max()

done.

> if you call it as max<float>() then you can have 0 be the first parameter

thanks.

> does this do the right thing when the whole rectangle is in the -x/-y quadrant?

good check. it does. also when it crosses 0.

  float x = ceilf(-11.6); // -11
  float maxx = floorf(-7.4); // -8
  std::cout << maxx - x << "\n";
gives: 3 = -8 - -11 = -8 + 11

> > LayoutTests/compositing/culling/tile-occlusion-boundaries.html:9
> > +      background-color:red;
> 
> we try to construct pixel tests such that green mean pass and red means fail, can you tweak this so you see only green if it passes and something else if it fails?

Made the rect green by adding a small green jpg (so ToT can tell it's opaque). I can make the bad pixels red but this requires using more layers underneath and aligning with their boundaries. If our decision on how to assign border texels changes it would render the test silently useless so I'd prefer to make it expose blue behind the NCCH. Sound reasonable?

[Dana Jansens]

Created attachment 123225 [details]
Patch

[James Robinson]

Comment on attachment 123225 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123225&action=review

> LayoutTests/compositing/culling/tile-occlusion-boundaries.html:22
> +<img class="scaled" src="../resources/green.jpg"></img>

instead of checking in a new file, can you re-use an existing one or generate an image programatically with canvas and toImageURL()? we have two files called "green.jpg" and one file called "green.png" in LayoutTests already

> LayoutTests/compositing/culling/tile-occlusion-boundaries.html:25
> +The image sits such that it completely fills a tile in the white background layer. However some of the edge pixels of the box are only partially filled, so they end up being composited non-opaque.

unfortunately all this visible text means that we'll need different pixel baselines for every platform. can you put this in an HTML comment like "<!-- text text text -->" so developers can see the descriptive text but have it not be in the pixel output?

It's also not completely clear what the pass/fail criteria is here. Can you put that first, and then the description later? I think the pass criteria here is something like "You should see a green rectangle with no blue fringes" or something of that nature

[Dana Jansens]

(In reply to comment #10)
> instead of checking in a new file, can you re-use an existing one or generate an image programatically with canvas and toImageURL()? we have two files called "green.jpg" and one file called "green.png" in LayoutTests already

done. reusing green.jpg.  css data would not be an <img src> so won't be caught as opaque in tot yet.

> > LayoutTests/compositing/culling/tile-occlusion-boundaries.html:25
> > +The image sits such that it completely fills a tile in the white background layer. However some of the edge pixels of the box are only partially filled, so they end up being composited non-opaque.
> 
> unfortunately all this visible text means that we'll need different pixel baselines for every platform. can you put this in an HTML comment like "<!-- text text text -->" so developers can see the descriptive text but have it not be in the pixel output?
> 
> It's also not completely clear what the pass/fail criteria is here. Can you put that first, and then the description later? I think the pass criteria here is something like "You should see a green rectangle with no blue fringes" or something of that nature

done and done.

[Dana Jansens]

Created attachment 123309 [details]
Patch

[James Robinson]

Comment on attachment 123309 [details]
Patch

R=me

[WebKit Review Bot]

Comment on attachment 123309 [details]
Patch

Clearing flags on attachment: 123309

Committed r105561: <http://trac.webkit.org/changeset/105561>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.