[chromium] Do partial-swap scissoring on quads during draw instead of on layers

• Component: New Bugs
• Status: RESOLVED FIXED

[Dana Jansens]

[chromium] Do partial-swap scissoring on quads during draw instead of on layers

[Dana Jansens]

Created attachment 153379 [details]
Patch

[Dana Jansens]

This needs a rebase, and I deleted a few scissor tests in CCLTHCommonTest that I would like to bring back in some form, but this is where I'm going with this change.

[Dana Jansens]

Created attachment 154763 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Dana Jansens]

Comment on attachment 154763 [details]
Patch

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

> Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:362
> +#if 0

woops.. missed 2 tests.

[Dana Jansens]

Created attachment 154803 [details]
Patch

[WebKit Review Bot]

Comment on attachment 154803 [details]
Patch

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

New failing tests:
compositing/geometry/vertical-scroll-composited.html
compositing/overflow/overflow-scaled-descendant-overlapping.html
platform/chromium/compositing/huge-layer-rotated.html

[WebKit Review Bot]

Created attachment 154866 [details]
Archive of layout-test-results from gce-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Dana Jansens]

Created attachment 155081 [details]
Patch

[Dana Jansens]

Summary of the test failures:

- The gutter quads were using the root layer's sharedQuadState - thus its "clipRect" also. This clipRect is not correct for them! It worked before because nothing read that rect, since we did not give gutter quads to QuadCuller and therefore didn't scissor them.  Fixed this by making a sharedquadstate for the gutter quads that has the correct clipRect.

- There was a lone use of quad->quadVisibleRect() in drawTileQuad, where everything else used "tileRect" which was equal to it. I switched all the use of tileRcet to quadDrawRect, but quadDrawRect gets scissored (quad->quadVisibleRect no longer does). So there was disagreement on which rect was being drawn. Fixed by using quadDrawRect consistently.

[Adrienne Walker]

Comment on attachment 155081 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:290
> +
> +    m_swapBufferRect.intersect(IntRect(IntPoint(), viewportSize()));

This seems like it should be handled at the damage tracker level and not here.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:434
> +    WebTransformationMatrix inverseTransformToRoot = renderPass->transformToRootTarget().inverse();
> +    FloatRect framebufferDamageRect = CCMathUtil::projectClippedRect(inverseTransformToRoot, frame.rootDamageRect);

This probably outside the scope of this patch, but it seems like we should be really be considering the damage inside the surface here.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:444
> +    if (texture)
> +        texture->setIsComplete(!renderPass->hasOcclusionFromOutsideTargetSurface());

Should this be true if any quads get rejected by the damage scissor that would have otherwise drawn into the surface?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:283
> +    IntRect m_swapBufferRect;

I feel like this doesn't need to be a member variable, and you should just pass frame.rootDamageRect to swapBuffers.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:545
> +        // Once a RenderPass has been drawn, its damage should be cleared in
> +        // case the RenderPass will be reused next frame.
> +        frame.renderPasses[i]->setFramebufferDamageRect(FloatRect());

Shouldn't you only clear the damage inside the scissor that was used when drawing into that surface?

Or, am I thinking about this wrong? A RenderPass is immutable when passed from embedded compositor to host compositor, so from the pass's perspective the damage is entirely cleared, but if not all of the damaged regions were drawn, then maybe the *surface* owned by the host compositor is no longer complete and has some invalidations?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.h:78
> +    FloatRect framebufferDamageRect() const { return m_framebufferDamageRect; }
> +    void setFramebufferDamageRect(FloatRect rect) { m_framebufferDamageRect = rect; }

What space is this in? Maybe this should just be "damageRect"?

[Dana Jansens]

Comment on attachment 155081 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:290
>> +    m_swapBufferRect.intersect(IntRect(IntPoint(), viewportSize()));
> 
> This seems like it should be handled at the damage tracker level and not here.

The reason it's here is because there is no contractual guarantee from the LRC interface that you will call swap after you call drawFrame. If you don't and you change the size of the viewport, then the last frame's swapRect needs to be shrunk accordingly.

There is no reason to put this code here right now, though, since we always swap. I could instead add an assert here that the swapBufferRect is empty?

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:434
>> +    FloatRect framebufferDamageRect = CCMathUtil::projectClippedRect(inverseTransformToRoot, frame.rootDamageRect);
> 
> This probably outside the scope of this patch, but it seems like we should be really be considering the damage inside the surface here.

That is the problem space of surface texture cacheing I believe. When theres no visible damage inside the surface, then it reuses the texture, culls the render pass, and we never get here.

If you're saying we should do render surface cacheing decisions inside LRC, then I probably agree with you and we should think about that some...

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:444
>> +        texture->setIsComplete(!renderPass->hasOcclusionFromOutsideTargetSurface());
> 
> Should this be true if any quads get rejected by the damage scissor that would have otherwise drawn into the surface?

Nope. Thanks.

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:283
>> +    IntRect m_swapBufferRect;
> 
> I feel like this doesn't need to be a member variable, and you should just pass frame.rootDamageRect to swapBuffers.

I did this again to handle the case that you don't swapBuffers after calling drawFrame, then the accumulated swapRect would be saved. You prefer to just use the rootDamageRect?

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:545
>> +        frame.renderPasses[i]->setFramebufferDamageRect(FloatRect());
> 
> Shouldn't you only clear the damage inside the scissor that was used when drawing into that surface?
> 
> Or, am I thinking about this wrong? A RenderPass is immutable when passed from embedded compositor to host compositor, so from the pass's perspective the damage is entirely cleared, but if not all of the damaged regions were drawn, then maybe the *surface* owned by the host compositor is no longer complete and has some invalidations?

I believe its's an invariant that all damage will be inside the partial swap rect. The line above clears all the damage in the tracker similarly.

So, I don't think it should be possible for the host compositor to not draw all the known damaged parts of its render passes. The ExternalCompositorLayer will interact with the damage tracker in the host compositor to ensure this, right?

>> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.h:78
>> +    void setFramebufferDamageRect(FloatRect rect) { m_framebufferDamageRect = rect; }
> 
> What space is this in? Maybe this should just be "damageRect"?

Its in the space of the framebuffer.. which is in "target space" where this render pass is the target. I was using framebuffer space to denote this as it's device pixels but not litterally device space (no perspective/window matrices applied). Should we rename this and the framebufferOutputRect above to remove the framebuffer prefix?

[Dana Jansens]

Comment on attachment 155081 [details]
Patch

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

>>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:444
>>> +        texture->setIsComplete(!renderPass->hasOcclusionFromOutsideTargetSurface());
>> 
>> Should this be true if any quads get rejected by the damage scissor that would have otherwise drawn into the surface?
> 
> Nope. Thanks.

Retracting this.. If a quad was rejected by the damage scissor that means the texture is already valid there. So I think this code is correct and I will include a test that demonstrates it.

[Dana Jansens]

Comment on attachment 155081 [details]
Patch

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

>>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:283
>>> +    IntRect m_swapBufferRect;
>> 
>> I feel like this doesn't need to be a member variable, and you should just pass frame.rootDamageRect to swapBuffers.
> 
> I did this again to handle the case that you don't swapBuffers after calling drawFrame, then the accumulated swapRect would be saved. You prefer to just use the rootDamageRect?

The problem here is that frame.rootDamageRect doesn't exist when swapBuffers is called.

1) The client calls drawFrame, which creates a "frame" and computes the rootDamageRect.
2) The client calls swapBuffers.

The client doesn't know what rootDamageRect was used by drawFrame. And I didn't want to have to return it and then pass it in again.. or is that preferable to a member variable here?

[Dana Jansens]

Created attachment 157313 [details]
Patch

[Dana Jansens]

I took out all the renaming of scissor things in occlusion tracker (and related tests). So the names are wrong in this CL but it's a lot smaller as a result.

I renamed the "framebufferFoo" stuff on RenderPass to just be "Foo". Hope you like.

Also added a test that verifies damaging a part of a surface and drawing with partial swap doesn't cause the texture to be marked as incomplete.

[Adrienne Walker]

Comment on attachment 157313 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:285
> +    m_swapBufferRect.intersect(IntRect(IntPoint(), viewportSize()));

I still find this a little weird, since it handles the case that the viewport gets smaller but expects other code to handle the viewport getting bigger.  Maybe just intersect m_swapBufferRect right before the partial swap instead to guarantee that this intersect happens at the right time?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:439
> +    if (texture)
> +        texture->setIsComplete(!renderPass->hasOcclusionFromOutsideTargetSurface());

Re: previous thread about this line.  I think I was wondering if there could be some interaction with clipping or occlusion here that could cause this to be untrue, but I think I've convinced myself that you're right and this is correct.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:455
> +    IntRect scissorRectInQuad = enclosingIntRect(CCMathUtil::projectClippedRect(quad->quadTransform().inverse(), scissorRect));
> +    IntRect quadDrawRect = intersection(quad->quadVisibleRect(), scissorRectInQuad);
> +    if (quadDrawRect.isEmpty())
> +        return;

I think this is unnecessary.  Setting the actual scissor is going to be at least as accurate than projecting into the quad's space, and there's really not really much advantage to doing extra work to try to not draw outside of the scissor.  That's what the GPU is for.

(I do totally buy that more drawQuad functions should use quad->quadVisibleRect(), but I don't think you need this CPU-side scissor step.)

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:457
> +    setScissorToRect(frame, enclosingIntRect(scissorRect));

What are you gaining by setting the scissor for every quad? Can you set it once for the framebufferDamageRect?

[Dana Jansens]

Comment on attachment 157313 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:285
>> +    m_swapBufferRect.intersect(IntRect(IntPoint(), viewportSize()));
> 
> I still find this a little weird, since it handles the case that the viewport gets smaller but expects other code to handle the viewport getting bigger.  Maybe just intersect m_swapBufferRect right before the partial swap instead to guarantee that this intersect happens at the right time?

Sure, I'll do that!

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:439
>> +        texture->setIsComplete(!renderPass->hasOcclusionFromOutsideTargetSurface());
> 
> Re: previous thread about this line.  I think I was wondering if there could be some interaction with clipping or occlusion here that could cause this to be untrue, but I think I've convinced myself that you're right and this is correct.

Ok thanks.

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:455
>> +        return;
> 
> I think this is unnecessary.  Setting the actual scissor is going to be at least as accurate than projecting into the quad's space, and there's really not really much advantage to doing extra work to try to not draw outside of the scissor.  That's what the GPU is for.
> 
> (I do totally buy that more drawQuad functions should use quad->quadVisibleRect(), but I don't think you need this CPU-side scissor step.)

See http://code.google.com/p/chromium/issues/detail?id=115966#c20 for history about this. At least on ChromeOS devices, doing the CPU-side scissor has a large impact.

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:457
>> +    setScissorToRect(frame, enclosingIntRect(scissorRect));
> 
> What are you gaining by setting the scissor for every quad? Can you set it once for the framebufferDamageRect?

Hm yeh. I'll try that out :)

[Adrienne Walker]

Comment on attachment 157313 [details]
Patch

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

>>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:455
>>> +        return;
>> 
>> I think this is unnecessary.  Setting the actual scissor is going to be at least as accurate than projecting into the quad's space, and there's really not really much advantage to doing extra work to try to not draw outside of the scissor.  That's what the GPU is for.
>> 
>> (I do totally buy that more drawQuad functions should use quad->quadVisibleRect(), but I don't think you need this CPU-side scissor step.)
> 
> See http://code.google.com/p/chromium/issues/detail?id=115966#c20 for history about this. At least on ChromeOS devices, doing the CPU-side scissor has a large impact.

There's two cases: fully clipped and partially clipped.  You are already handling the fully clipped case above by intersecting the quad->clippedRectInTarget() with framebufferDamageRect.  I think that's also what the bug you link to is about.  Partially clipped quads on the other hand already have to submit the same amount of data to the GPU, and that's what I think doesn't need to be optimized for here.

[Dana Jansens]

Comment on attachment 157313 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:455
>>>> +        return;
>>> 
>>> I think this is unnecessary.  Setting the actual scissor is going to be at least as accurate than projecting into the quad's space, and there's really not really much advantage to doing extra work to try to not draw outside of the scissor.  That's what the GPU is for.
>>> 
>>> (I do totally buy that more drawQuad functions should use quad->quadVisibleRect(), but I don't think you need this CPU-side scissor step.)
>> 
>> See http://code.google.com/p/chromium/issues/detail?id=115966#c20 for history about this. At least on ChromeOS devices, doing the CPU-side scissor has a large impact.
> 
> There's two cases: fully clipped and partially clipped.  You are already handling the fully clipped case above by intersecting the quad->clippedRectInTarget() with framebufferDamageRect.  I think that's also what the bug you link to is about.  Partially clipped quads on the other hand already have to submit the same amount of data to the GPU, and that's what I think doesn't need to be optimized for here.

Oh I see! Okay!

[Dana Jansens]

Created attachment 157487 [details]
Patch

[Adrienne Walker]

Comment on attachment 157487 [details]
Patch

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

R=me.

> Source/WebCore/ChangeLog:15
> +        This also make partial-swap functionality completely contained within
> +        LayerRendererChromium!

!!!

[Dana Jansens]

Committed r125196: <http://trac.webkit.org/changeset/125196>