[webkit-reviews] review denied: [Bug 76349] [chromium] Allow modification of size of partially occluded quads during culling to reduce pixel overdraw. : [Attachment 123729] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied W. James MacLean
<wjmaclean at chromium.org>'s request for review:
Bug 76349: [chromium] Allow modification of size of partially occluded quads
during culling to reduce pixel overdraw.
https://bugs.webkit.org/show_bug.cgi?id=76349

Attachment 123729: Patch
https://bugs.webkit.org/attachment.cgi?id=123729&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=123729&action=review


> Source/WebCore/platform/graphics/chromium/cc/CCDrawQuad.h:62
> +	   m_quadVisibleRect = quadVisibleRect;
> +	   m_quadVisibleRect.intersect(m_quadRect);

this sort of logic belongs in the .cpp, and the behavior should be documented
in the header since this isn't a simple setter

> Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:107
> +	   IntRect transformedQuadRect(enclosedIntRect(floatTransformedRect));
> +
> +	   IntRect transformedVisibleQuadRect =
rectSubtractRegion(opaqueCoverageThusFar, transformedQuadRect);
> +	   bool keepQuad = !transformedVisibleQuadRect.isEmpty();

this looks wrong. consider quad A which is opaque and extends from (5, 5) to
(10, 10) and quad B which is behind quad A and extends from (4.1, 4.1) to
(10.9, 10.9). this logic would calculate (5, 5, 10, 10) for quad B's
transformedQuadRect, since it uses enclosedIntRect(), and keepQuad would be set
to false and we wouldn't draw it, even though we should!. keep in mind that
since this is all per render surface the whole thing could be greatly scaled up
later on and produce very visible errors.

i think that in order to cull correctly and only deal with integer quads/rects,
we need to be conservative. that means to decide if a given quad is culled
inflate to integer boundaries with enclosingIntRect() and then when marking a
quad as opaque for further culling deflate with enclosedIntRect().

even if my analysis is wrong, please add some test coverage for this.

> Source/WebKit/chromium/tests/CCQuadCullerTest.cpp:41
> +#define MakeTileQuad(s, r)  \
> +    CCTileDrawQuad::create((s), (r), 1, IntPoint(1, 1), IntSize(100, 100),
0, false, false, false, false, false)

why isn't this a function? we very rarely use macros for things like this, and
if we did it would be NAMED_LIKE_THIS()