Bug 91800 - [chromium] Do partial-swap scissoring on quads during draw instead of on layers
Summary: [chromium] Do partial-swap scissoring on quads during draw instead of on layers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dana Jansens
URL:
Keywords:
Depends on:
Blocks: 93630
  Show dependency treegraph
 
Reported: 2012-07-19 16:57 PDT by Dana Jansens
Modified: 2012-08-09 13:21 PDT (History)
13 users (show)

See Also:


Attachments
Patch (115.02 KB, patch)
2012-07-19 16:57 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (196.97 KB, patch)
2012-07-26 15:13 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (212.43 KB, patch)
2012-07-26 18:19 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-01 (998.64 KB, application/zip)
2012-07-27 00:37 PDT, WebKit Review Bot
no flags Details
Patch (213.30 KB, patch)
2012-07-27 16:03 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (142.83 KB, patch)
2012-08-08 15:35 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (133.98 KB, patch)
2012-08-09 10:44 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dana Jansens 2012-07-19 16:57:34 PDT
[chromium] Do partial-swap scissoring on quads during draw instead of on layers
Comment 1 Dana Jansens 2012-07-19 16:57:45 PDT
Created attachment 153379 [details]
Patch
Comment 2 Dana Jansens 2012-07-19 16:58:48 PDT
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.
Comment 3 Dana Jansens 2012-07-26 15:13:21 PDT
Created attachment 154763 [details]
Patch
Comment 4 WebKit Review Bot 2012-07-26 15:16:01 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 5 Dana Jansens 2012-07-26 18:07:40 PDT
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.
Comment 6 Dana Jansens 2012-07-26 18:19:53 PDT
Created attachment 154803 [details]
Patch
Comment 7 WebKit Review Bot 2012-07-27 00:37:47 PDT
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
Comment 8 WebKit Review Bot 2012-07-27 00:37:52 PDT
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
Comment 9 Dana Jansens 2012-07-27 16:03:43 PDT
Created attachment 155081 [details]
Patch
Comment 10 Dana Jansens 2012-07-27 16:08:34 PDT
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.
Comment 11 Adrienne Walker 2012-07-30 10:44:33 PDT
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"?
Comment 12 Dana Jansens 2012-08-08 12:31:00 PDT
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?
Comment 13 Dana Jansens 2012-08-08 14:58:31 PDT
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.
Comment 14 Dana Jansens 2012-08-08 15:35:36 PDT
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?
Comment 15 Dana Jansens 2012-08-08 15:35:49 PDT
Created attachment 157313 [details]
Patch
Comment 16 Dana Jansens 2012-08-08 15:37:35 PDT
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.
Comment 17 Adrienne Walker 2012-08-08 17:02:10 PDT
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?
Comment 18 Dana Jansens 2012-08-09 09:02:08 PDT
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 :)
Comment 19 Adrienne Walker 2012-08-09 09:19:56 PDT
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.
Comment 20 Dana Jansens 2012-08-09 09:36:02 PDT
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!
Comment 21 Dana Jansens 2012-08-09 10:44:23 PDT
Created attachment 157487 [details]
Patch
Comment 22 Adrienne Walker 2012-08-09 11:08:33 PDT
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!

!!!
Comment 23 Dana Jansens 2012-08-09 13:18:44 PDT
Committed r125196: <http://trac.webkit.org/changeset/125196>