Bug 76349 – [chromium] Allow modification of size of partially occluded quads during culling to reduce pixel overdraw.

• Component: New Bugs
• Status: RESOLVED FIXED

[W. James MacLean]

[chromium] Allow modification of size of partially occluded quads. [not for review, yet]

[W. James MacLean]

Created attachment 122573 [details]
Patch

[W. James MacLean]

Comment on attachment 122573 [details]
Patch

This patch is not quite done yet. I would appreciate comments on the approach, specifically w.r.t. to the two issues mentioned below.

1) It seems ugly having to have the special modification for CCTileDrawQuad, yet it fails without it. When we re-size the quad, we need to say where the new quad starts in the texture ...

We can't just do this via the clip rect, as this is shared with all quads in the layer.

2) The code specific to CCTileDrawQuad in CCQuadCuller seems to invoke some sort of pointer error when running CCQuadCullerTest.verifyCullCenterTileOnly (which is the only CCQuadCuller unit test that will invoke the quad resizing logic). When run, it gives the following (truncated) error dump:

[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from CCQuadCullerTest
[ RUN      ] CCQuadCullerTest.verifyCullCenterTileOnly
*** glibc detected *** out/Debug/webkit_unit_tests: munmap_chunk(): invalid pointer: 0x000000000242c640 ***
======= Backtrace: =========
/lib/libc.so.6(+0x775b6)[0x7f90bc03f5b6]
.
.
.

183:0115/163925:532185838963:ERROR:process_util_posix.cc(138)] Received signal 6
	base::debug::StackTrace::StackTrace() [0x7f90c4675492]
	base::(anonymous namespace)::StackDumpSignalHandler() [0x7f90c46d2cfd]
	0x7f90bbffbaf0
	0x7f90bbffba75
	0x7f90bbfff5c0
	0x7f90bc0354fb
	0x7f90bc03f5b6
	WTF::deleteOwnedPtr<>() [0x7f90c56b9567]
	WTF::OwnPtr<>::~OwnPtr() [0x7f90c56b954d]
	WTF::VectorDestructor<>::destruct() [0x7f90c56b9466]
	WTF::VectorTypeOperations<>::destruct() [0x7f90c56b9360]
	WTF::Vector<>::shrink() [0x7f90c56b90ef]
	WTF::Vector<>::~Vector() [0x7f90c56b8e09]
	(anonymous namespace)::CCQuadCullerTest_verifyCullCenterTileOnly_Test::TestBody() [0x7f90c56b720c]
	testing::internal::HandleSehExceptionsInMethodIfSupported<>() [0x7f90c5a6af05]

[W. James MacLean]

Created attachment 122624 [details]
Patch

[W. James MacLean]

Comment on attachment 122624 [details]
Patch

Fixed logic error in test for bounds = region (was failing for 'L'-shaped regions).

Note: this patch should work OK for all quad types except perhaps CustomDrawQuads and RenderSurfaceDraw quads - I haven't been able to test with those yet.

[Dana Jansens]

Comment on attachment 122624 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:63
> +    rectRegion.subtract(Region(boundsRect));

You don't need to Region() as the constructor is not explicit. Another review asked me to just pass the IntRect in this case before.

This is rectRegion.subtract(rectRegion.bounds()). When will this return anything but an empty set?

> Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:65
> +    if (boundsRect.isEmpty() || rectRegion.isEmpty())

boundsRect is the containing bounds of rectRegion. Is not one empty if and only if the other is empty? I feel like maybe there are some tests missing?

> Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:93
> +            if (drawQuad->material() == CCDrawQuad::TiledContent) {

virtual function?

[W. James MacLean]

(In reply to comment #5)
> (From update of attachment 122624 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122624&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:63
> > +    rectRegion.subtract(Region(boundsRect));
> 
> You don't need to Region() as the constructor is not explicit. Another review asked me to just pass the IntRect in this case before.
> 
> This is rectRegion.subtract(rectRegion.bounds()). When will this return anything but an empty set?
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:65
> > +    if (boundsRect.isEmpty() || rectRegion.isEmpty())
> 
> boundsRect is the containing bounds of rectRegion. Is not one empty if and only if the other is empty? I feel like maybe there are some tests missing?

rectRegion, at this point, may not be rectangular (we've subtracted off the intersection between it and the occluded region). Perhaps needs a new name.

At any rate, I've changed the logic (subtracted things in the wrong order), based on the code you quote above.

> > Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:93
> > +            if (drawQuad->material() == CCDrawQuad::TiledContent) {
> 
> virtual function?

I think these were explicitly avoided during the original design of the DrawQuad classes (see LayerRendererChromium::drawQuad()), so I don't want to introduce them here until Enne has looked at it.

[W. James MacLean]

Created attachment 122641 [details]
Patch

[W. James MacLean]

Comment on attachment 122641 [details]
Patch


Fixed the unit test - we were deriving from CCDrawQuad, but setting the materials to be TiledContent ... bad idea if you pass it to code that believes what it's told by material().

[Adrienne Walker]

Comment on attachment 122641 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:97
> +            // FIXME: We shouldn't need to handle special quad cases here. Should
> +            // CCTileDrawQuad somehow do this computation internally and automatically,
> +            // by assuming changes to it's drawQuad always remap the texture offset?

Another option might be to store the visible rect separately from quad rect on CCDrawQuad, and just have the specialized quad drawing functions handle it, since they're already special-cased by quad type.

[W. James MacLean]

(In reply to comment #9)
> (From update of attachment 122641 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122641&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:97
> > +            // FIXME: We shouldn't need to handle special quad cases here. Should
> > +            // CCTileDrawQuad somehow do this computation internally and automatically,
> > +            // by assuming changes to it's drawQuad always remap the texture offset?
> 
> Another option might be to store the visible rect separately from quad rect on CCDrawQuad, and just have the specialized quad drawing functions handle it, since they're already special-cased by quad type.

Excellent idea! Will revise and re-submit shortly ...

[W. James MacLean]

Created attachment 122955 [details]
Patch

[WebKit Review Bot]

Comment on attachment 122955 [details]
Patch

Attachment 122955 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11283321

New failing tests:
compositing/color-matching/image-color-matching.html

[Adrienne Walker]

Comment on attachment 122955 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:539
> +    const IntRect& tileRect = quad->quadVisibleRect();

What about edge AA? Perhaps if the visible rect edge isn't the same as the quad rect edge, then you need to turn off edge AA for that edge? Or, if there's any edge AA, then use the quad rect?

I mean, it's the case that you don't set a visible rect if the layer isn't axis aligned, and we also don't have edge AA either when that happens, but that feels like coincidence (that could change in the future), rather than an explicit decision.

> Source/WebCore/platform/graphics/chromium/cc/CCTileDrawQuad.h:42
> +    void setTextureOffset(const IntPoint& newOffset) { m_textureOffset = newOffset; }

nit: This isn't used anywhere.

[W. James MacLean]

Comment on attachment 122955 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:539
>> +    const IntRect& tileRect = quad->quadVisibleRect();
> 
> What about edge AA? Perhaps if the visible rect edge isn't the same as the quad rect edge, then you need to turn off edge AA for that edge? Or, if there's any edge AA, then use the quad rect?
> 
> I mean, it's the case that you don't set a visible rect if the layer isn't axis aligned, and we also don't have edge AA either when that happens, but that feels like coincidence (that could change in the future), rather than an explicit decision.

Question: at present we're always making a vertical or horizontal cut to get the visible rect, so one edge will line up (leave it as-is?), one edge won't (turn off edgeAA if it used to be on?), but the remaining edges line up, but are *shorter* than the original edge (here leave edgeAA on if it was originally on, I guess, as these shorter edges still line up with where they use to?).

I agree it could change in the future, although we could perhaps guard against that by using an ASSERT. That said, I'm happy to figure out the logic to test each edge if it has edgeAA, and turn it off if the edge has 'moved'.

I'd rather not fall back to just using the whole rect if we can avoid it.

Does that seem reasonable?

>> Source/WebCore/platform/graphics/chromium/cc/CCTileDrawQuad.h:42
>> +    void setTextureOffset(const IntPoint& newOffset) { m_textureOffset = newOffset; }
> 
> nit: This isn't used anywhere.

Thanks, missed removing this.

> Source/WebKit/chromium/tests/CCQuadCullerTest.cpp:29
> +#include "cc/CCTileDrawQuad.h"

This needs to go too ...

[Adrienne Walker]

(In reply to comment #14)
> (From update of attachment 122955 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122955&action=review
> 
> >> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:539
> >> +    const IntRect& tileRect = quad->quadVisibleRect();
> > 
> > What about edge AA? Perhaps if the visible rect edge isn't the same as the quad rect edge, then you need to turn off edge AA for that edge? Or, if there's any edge AA, then use the quad rect?
> > 
> > I mean, it's the case that you don't set a visible rect if the layer isn't axis aligned, and we also don't have edge AA either when that happens, but that feels like coincidence (that could change in the future), rather than an explicit decision.
> 
> Question: at present we're always making a vertical or horizontal cut to get the visible rect, so one edge will line up (leave it as-is?), one edge won't (turn off edgeAA if it used to be on?), but the remaining edges line up, but are *shorter* than the original edge (here leave edgeAA on if it was originally on, I guess, as these shorter edges still line up with where they use to?).
>
> I agree it could change in the future, although we could perhaps guard against that by using an ASSERT. That said, I'm happy to figure out the logic to test each edge if it has edgeAA, and turn it off if the edge has 'moved'.

Sure, that seems like a reasonable approach.

[W. James MacLean]

Created attachment 123128 [details]
Patch

[W. James MacLean]

Added disabling of AA on edges that differ between quadRect and quadVisibleRect.

[WebKit Review Bot]

Comment on attachment 123128 [details]
Patch

Attachment 123128 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11253516

New failing tests:
compositing/color-matching/image-color-matching.html

[Adrienne Walker]

(In reply to comment #18)
> (From update of attachment 123128 [details])
> Attachment 123128 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/11253516
> 
> New failing tests:
> compositing/color-matching/image-color-matching.html

Can you fix this? There's a nice big horizontal ~1px black line across the bottom set of images with your patch applied.

[W. James MacLean]

(In reply to comment #19)
> (In reply to comment #18)
> > (From update of attachment 123128 [details] [details])
> > Attachment 123128 [details] [details] did not pass chromium-ews (chromium-xvfb):
> > Output: http://queues.webkit.org/results/11253516
> > 
> > New failing tests:
> > compositing/color-matching/image-color-matching.html
> 
> Can you fix this? There's a nice big horizontal ~1px black line across the bottom set of images with your patch applied.

Debugging it right now :-)

[W. James MacLean]

(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #18)
> > > (From update of attachment 123128 [details] [details] [details])
> > > Attachment 123128 [details] [details] [details] did not pass chromium-ews (chromium-xvfb):
> > > Output: http://queues.webkit.org/results/11253516
> > > 
> > > New failing tests:
> > > compositing/color-matching/image-color-matching.html
> > 
> > Can you fix this? There's a nice big horizontal ~1px black line across the bottom set of images with your patch applied.
> 
> Debugging it right now :-)

Fix found, waiting for 76658 to clear.

[W. James MacLean]

Created attachment 123547 [details]
Patch

[W. James MacLean]

This patch should fix the failing layout test.

[Adrienne Walker]

Comment on attachment 123547 [details]
Patch

LGTM.

[James Robinson]

I can't tell from the title, ChangeLog, or bug comments what this patch is actually trying to accomplish. What problem is this solving? What functionality does it enable?

[W. James MacLean]

(In reply to comment #25)
> I can't tell from the title, ChangeLog, or bug comments what this patch is actually trying to accomplish. What problem is this solving? What functionality does it enable?

At present, the draw-culler either culls a quad because it is completely occluded, or it draws the entire quad (even if it is partially occluded). This patch allows us to draw only the visible part of partially occluded quads. It seems to help quite a bit in reducing overdraw.

[W. James MacLean]

Created attachment 123729 [details]
Patch

[James Robinson]

Comment on attachment 123729 [details]
Patch

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()

[W. James MacLean]

Created attachment 123978 [details]
Patch

[W. James MacLean]

(In reply to comment #28)
> (From update of attachment 123729 [details])
> 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

Done.

> > 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.

Your analysis is correct. Fixed, and two new tests added.

> > 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()

Fixed.

[James Robinson]

Comment on attachment 123978 [details]
Patch

R=me, thanks for the additional tests.

[WebKit Review Bot]

Comment on attachment 123978 [details]
Patch

Clearing flags on attachment: 123978

Committed r106076: <http://trac.webkit.org/changeset/106076>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.