[chromium] Partially filled pixels do not occlude pixels below them.
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.
Created attachment 123183 [details] Patch Fail at rebasing. Better version.
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?
(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.
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?
Created attachment 123193 [details] Patch For jamesr review Please see comment #1 re: FloatRect.cpp
Sorry collision with your comments, will address them first.
(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?
Created attachment 123225 [details] Patch
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
(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.
Created attachment 123309 [details] Patch
Comment on attachment 123309 [details] Patch R=me
Comment on attachment 123309 [details] Patch Clearing flags on attachment: 123309 Committed r105561: <http://trac.webkit.org/changeset/105561>
All reviewed patches have been landed. Closing bug.