Bug 79927 – [chromium] Add clipping to scissor rect to CCOcclusionTracker

• Component: New Bugs
• Status: RESOLVED FIXED

[Dana Jansens]

[chromium] Add clipping to scissor rect to CCOcclusionTracker

[Dana Jansens]

Created attachment 129481 [details]
Patch

Adds new unit tests for CCOcclusionTracker, for the scenarios tested wrt using scissor rect in CCQuadCullerTest.cpp.

[Adrienne Walker]

Comment on attachment 129481 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:298
> +    const FloatRect& clipRect = currentDamageRect(m_stack.last().surface);

I think the clip rect here should include an explicit scissor.  The mapping of surface scissor into layer space ends up being conservative, so it seems possible to me that the union of scissor + occlusion could hide a quad.  We might as well do that in this patch if you're going to add logic for damage rects as well.

> Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:348
> +FloatRect CCOcclusionTrackerBase<LayerType, RenderSurfaceType>::currentDamageRect(const RenderSurfaceChromium* /* surface */) const
> +{
> +    return FloatRect();
> +}
> +
> +template<typename LayerType, typename RenderSurfaceType>
> +FloatRect CCOcclusionTrackerBase<LayerType, RenderSurfaceType>::currentDamageRect(const CCRenderSurface* surface) const
> +{
> +    return surface->damageTracker()->currentDamageRect();
> +}
> +

Design quibble: I think these functions would be better suited to live on RenderSurfaceType.  If you did that, then rather than having to push a boolean for "am I caring about damage rects?" all the way through CCOcclusionTracker*, you could simply ask the surface "what's your current scissor?" (including clip rect and damage, if enabled) and use that to intersect occlusion queries.

[Adrienne Walker]

Comment on attachment 129481 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:298
>> +    const FloatRect& clipRect = currentDamageRect(m_stack.last().surface);
> 
> I think the clip rect here should include an explicit scissor.  The mapping of surface scissor into layer space ends up being conservative, so it seems possible to me that the union of scissor + occlusion could hide a quad.  We might as well do that in this patch if you're going to add logic for damage rects as well.

Sorry to use confusing language.  By explicit scissor, I mean "clip rect on the surface".

[Shawn Singh]

(1)  One minor nit on my part.  As far as I understand, it should be consistent with Enne's feedback too.

The naming conventions I thought we had (in fact, we renamed the old scissorRect to clipRect to clear up this exact confusion):
  clip rect - reserved for actual overflow/maskToBounds clipping
  damage rect - the rect that encloses what really has changed
  scissor rect - the actual rect that limits the draw/paint region, an intersection of clip and damage.

If this sounds OK, then there may be a few places in code to change:
- the bug title on the changelog, perhaps should say damage instead of scissor?
- various locations where the word "clip" is used


(2)  I wouldn't be surprised if, after accounting for Enne's idea, that it may be appropriate to create a helper function that does it in general, since that pattern of (clipRect or damage or clip intersect damage) has cropped up in multiple places in code.  OK with me if we leave that for another patch, as long as a bug is created to track it =)

[Shawn Singh]

> 
> (2)  I wouldn't be surprised if, after accounting for Enne's idea, that it may be appropriate to create a helper function that does it in general, since that pattern of (clipRect or damage or clip intersect damage) has cropped up in multiple places in code.  OK with me if we leave that for another patch, as long as a bug is created to track it =)

Just realized this is redundant with what Enne said.  I like Enne's proposed way of doing this.

[Dana Jansens]

Created attachment 129618 [details]
Patch

- Moved the computation of the surface scissor rect over to RenderSurface*.
- Fixed naming of scissor/damage/clip.
- Removed the "bool clip" parameter passed around to the static inline functions.

Making a CCFloatScissorRect that can be infinite cleans up this CL a whole lot. What do you think?

[James Robinson]

"scissor" makes me think of glScissor, which has to be integer so at first blush CCFloatScissorRect is pretty confusing.  Is being infinite the only thing we want to get out of being floating point?

[Dana Jansens]

Yeh I just want a rect class that can be infinite.

Ultimately it's for dealing with damage and scissoring, but I have a float version because currentDamageRect() gives a FloatRect. You're right about where the name came from though.

The point of it is to be able to do stuff like

enclosingIntRect(intersection(someFloatRect, aPossiblyInfiniteFloatRect))

without needing 4 or 5 lines of code, and to avoid having to pass around pairs of rects and bools all over the place.

It could be something more general like CCFloatInfiniteRect? I'm not thinking of anything overly clever at the moment.

[Adrienne Walker]

(In reply to comment #8)
> Yeh I just want a rect class that can be infinite.

Maybe I'm missing something, but why do we need a rect class that can be infinite? In all cases, a surface has a content rect.  Can RenderSurfaceType::scissorRect() use the surface's content rect as the initial scissor rect before intersecting with clip or damage?

[Dana Jansens]

Yep it can. The hassle becomes visible when you conside the CCOcclusionTracker::scissorRectInScreenSpace(). 

This rect can exist or not (ie infinite for intersections). All the helper functions to compute occlusion would need to take bool+rect to handle the screen space scissor, or be duplicated with scissor/non-scissor versions. I tried variations of each of these today and this was the cleanest version I thot by far, since it allows intersect() to simply be a noop at the right times. Rather than
m_hasScissorRectInScreenSpace
m_scissorRectInScreenSpace

This same hassle was duplicated for me when making the existing unit tests - which expect no scissoring - continue to pass. Enabling scissoring made the existing tests much less useful.

Those are the reasons I thot to propose this class.

It's also possible to make the class inside CCOcclusionTracker. But I thot it might do a good job of cleaning up other scissor rect code, and let me make unit tests.

[Dana Jansens]

In terms of naming, I realized what it is. It's more or less a MaybeRect (via Haskell's Maybe). It would probably be best to stick it inside the CCOcclusionTracker class, and pull it out only if needed in the future. I'll throw up a CL like that tomorrow morning.

[Dana Jansens]

Created attachment 129711 [details]
Patch

Erg. If the MaybeRect class is contained inside CCOcclusionTracker, then the helper methods can't be static inline functions, they have to join the class to see the data type. How about CCMaybeRect?

[WebKit Review Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[WebKit Review Bot]

Attachment 129711 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'LayoutTests/ChangeLog', u'La..." exit_code: 1
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitDefines.h"
Source/WebKit2/WebProcess/gtk/WebProcessMainGtk.cpp:71:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 197 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dana Jansens]

Bots are going crazy..

[WebKit Review Bot]

Comment on attachment 129711 [details]
Patch

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

New failing tests:
fast/workers/storage/use-same-database-in-page-and-workers.html

[Adrienne Walker]

(In reply to comment #10)
> Yep it can. The hassle becomes visible when you conside the CCOcclusionTracker::scissorRectInScreenSpace(). 
> 
> This rect can exist or not (ie infinite for intersections).

Forgive me for being dense, but when do you not have a finite scissor rect in screen space, in practice? It's not clear from the patch where that would happen in practice, since this value is only set directly (or not) in tests.

Also, does the occlusion tracker have to know about the damage tracking setting? In other words, how hard is it to change scissorRect(bool) => scissorRect()?

[Dana Jansens]

(In reply to comment #17)
> (In reply to comment #10)
> > Yep it can. The hassle becomes visible when you conside the CCOcclusionTracker::scissorRectInScreenSpace(). 
> > 
> > This rect can exist or not (ie infinite for intersections).
> 
> Forgive me for being dense, but when do you not have a finite scissor rect in screen space, in practice? It's not clear from the patch where that would happen in practice, since this value is only set directly (or not) in tests.

Hm I guess we could use IntRect(IntPoint(), CCLTHI::m_viewportSize) here? If so, lemme see about that.

> Also, does the occlusion tracker have to know about the damage tracking setting? In other words, how hard is it to change scissorRect(bool) => scissorRect()?

I'd like if it didn't. The damage tracker setting is only accessible via LRC, which is not available to the RenderSurface unless we pass it in. The tracker could pass an LRC pointer instead, but that's more "impl specific" which is why I left it as a bool. Any ideas?

[Adrienne Walker]

(In reply to comment #18)
> (In reply to comment #17) 
> > Also, does the occlusion tracker have to know about the damage tracking setting? In other words, how hard is it to change scissorRect(bool) => scissorRect()?
> 
> I'd like if it didn't. The damage tracker setting is only accessible via LRC, which is not available to the RenderSurface unless we pass it in. The tracker could pass an LRC pointer instead, but that's more "impl specific" which is why I left it as a bool. Any ideas?

One option might be to only create the damage tracker if we're going to use it, and the surface could then know whether or not to ask the damage tracker. But, then there are a number of places where you have to start checking if the tracker exists.  On the other hand, it's kind of weird to read (e.g. CCLTHI::m_rootDamageRect) and write (e.g. CCLTHI::setFullRootLayerDamage()) to the damage tracker if those values are never going to be correct or used, respectively.

At any rate, that sounds like a bit of a yak shave.  I'd be ok with punting that to another patch and not worrying about it here, unless you have any better ideas.

[Dana Jansens]

*** Bug 76786 has been marked as a duplicate of this bug. ***

[Dana Jansens]

Created attachment 129810 [details]
Patch

Yak punted! There's lots of things I want to address wrt to RenderSurface, and this is another.

No MaybeRects needed, success. Thanks for that.

Realized the RenderSurface::clipRect() is in the target's coord space, not the surface's own. Reworked how scissor
is computed for layers and surfaces accordingly. This made me use RenderSurface::damageContentRect() instead of
RenderSurface::scissorRect() because the surface can't compute its own scissor without knowing the damage in its target.
The damageContentRect() is combined with the current layer/surface's clip in CCOcclusionTracker to determine the
appropriate scissor rect.

[Dana Jansens]

Comment on attachment 129810 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:330
> +    IntRect damage = m_stack.last().surface->damageContentRect(m_useSurfaceDamage);

It occurred to me on my way home that getting the damage from the surface's damageTracker() is different than how the damage is computed for the actual glScissor, and may have adverse effects. Or is it okay to do this? Should I instead pass a over the computed surfaceDamageRect from CCLTHI for each surface? Something like:

occlusionTracker.enterTargetRenderSurface(surface, surfaceDamageRect);

[Dana Jansens]

Created attachment 129912 [details]
Patch

Ok this is converging I hope.

1) Uncomfortable with doing a different scissor calculation, I changed the interface so that CCLTHI can give the
same damage rects to the occlusionTracker that LRC will be using for drawing. I did this with a damageClient interface,
so CCOcclusionTracker can just query the client which will give back the damage. This lets us avoid knowing about things
 like CCRenderPass etc. And it can be null to not use damage.

1b) Also the client pattern here has a perk that we can still move the for-each-layer logic from CCLTH(I) into the
occlusionTracker in the future if we should choose to.

2) I realized that your uncertainty about computing occlusion of surfaces was well founded. While the
m_owningLayer->screenSpaceTransform seemed to do well enough for the surface contents, it would not include
the surface replica, and if that's not occluded, we need to consider the surface unoccluded. So I just took
out the surface methods, and we'll just worry about layers like we do currently in CCQuadCuller.

3) Added some more unit tests to test the interaction with the damage rect, instead of the overall scissor rects.

[Dana Jansens]

(In reply to comment #23)
> Created an attachment (id=129912) [details]
> Patch
> 
> Ok this is converging I hope.
> 
> 1) Uncomfortable with doing a different scissor calculation, I changed the interface so that CCLTHI can give the
> same damage rects to the occlusionTracker that LRC will be using for drawing. 

Shawn confirmed this is the right scissor rect to be using.

[Dana Jansens]

Created attachment 129986 [details]
Patch

Little naming cleanup (be more explicit about coordinate space)

[Adrienne Walker]

Comment on attachment 129986 [details]
Patch

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

I think this could be simplified a little bit more.

> Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.h:43
> +template<typename RenderSurfaceType>
> +class CCOcclusionTrackerDamageClientBase {
> +public:
> +    virtual FloatRect damageRect(const RenderSurfaceType*) const = 0;
> +};

I feel like this is introducing yet another abstraction mechanism.  We're already using templates to deal with CCRenderSurface and RenderSurfaceChromium having the same interface but being different classes.  Is it possible to stick with that approach and just add RenderSurfaceType::damageRect() that returns the real damage rect for CCRenderSurface and the full content rect for RenderSurfaceChromium?

And, going further with this simplification, maybe there could just be a RenderSurfaceType::scissorRect() that includes both damage and content.  That'd eliminate the need to test damage and content rects separately, yeah?

This approach would still allow a future change to move the for-each-layer logic here, right?

> Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.h:101
> +    // Allow tests to override this.
> +    virtual IntRect layerScissorRectInTargetSurface(const LayerType*) const;

Is it possible to just set the content rect directly on surfaces (or let it be calculated)?

[Adrienne Walker]

Comment on attachment 129986 [details]
Patch

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

I have to admit that it's really hard to understand design when the only consumer of an interface is a test.  I've finally wrapped my head around all of this, though.  Thanks for bearing with me and for the offline discussion to explain the constraints here.  R=me, with one bikeshed nit below.

>> Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.h:43
>> +};
> 
> I feel like this is introducing yet another abstraction mechanism.  We're already using templates to deal with CCRenderSurface and RenderSurfaceChromium having the same interface but being different classes.  Is it possible to stick with that approach and just add RenderSurfaceType::damageRect() that returns the real damage rect for CCRenderSurface and the full content rect for RenderSurfaceChromium?
> 
> And, going further with this simplification, maybe there could just be a RenderSurfaceType::scissorRect() that includes both damage and content.  That'd eliminate the need to test damage and content rects separately, yeah?
> 
> This approach would still allow a future change to move the for-each-layer logic here, right?

I see.  You need to get the damage rect in surface space, for which you need the damage rect in screen space, the flag for whether to use partial swaps, and the transform.  So you're passing in DamageClientBase almost like a closure to calculate it.  I like your suggestion of filing a bug to better abstract the damage rects somewhere so that the occlusion tracker doesn't have to know so much about them.  In the short term, I think this is reasonable.

> Source/WebKit/chromium/tests/CCOcclusionTrackerTest.cpp:90
> +    void resetLayerScissorRect() { m_overrideLayerScissorRect = false; }

bikeshed nit: This is kind of an opaque function name to my ears.  I usually expect a "reset" function to clear something, but instead it often makes the scissor rect even more constraining.  This is more like "useDefaultLayerScissorRect" or "clearScissorRectOverride".  Another option that might be clearer would be to just set the scissor rect explicitly to whatever you want it to be at all times, always using an override.

[Dana Jansens]

Created attachment 130492 [details]
Patch

[Dana Jansens]

(In reply to comment #27)
> (From update of attachment 129986 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129986&action=review
> 
> I have to admit that it's really hard to understand design when the only consumer of an interface is a test.  I've finally wrapped my head around all of this, though.  Thanks for bearing with me and for the offline discussion to explain the constraints here.  R=me, with one bikeshed nit below.

Indeed, thanks very much!

> >> Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.h:43
> >> +};
> > 
> > I feel like this is introducing yet another abstraction mechanism.  We're already using templates to deal with CCRenderSurface and RenderSurfaceChromium having the same interface but being different classes.  Is it possible to stick with that approach and just add RenderSurfaceType::damageRect() that returns the real damage rect for CCRenderSurface and the full content rect for RenderSurfaceChromium?
> > 
> > And, going further with this simplification, maybe there could just be a RenderSurfaceType::scissorRect() that includes both damage and content.  That'd eliminate the need to test damage and content rects separately, yeah?
> > 
> > This approach would still allow a future change to move the for-each-layer logic here, right?
> 
> I see.  You need to get the damage rect in surface space, for which you need the damage rect in screen space, the flag for whether to use partial swaps, and the transform.  So you're passing in DamageClientBase almost like a closure to calculate it.  I like your suggestion of filing a bug to better abstract the damage rects somewhere so that the occlusion tracker doesn't have to know so much about them.  In the short term, I think this is reasonable.

Done. http://code.google.com/p/chromium/issues/detail?id=117091

> > Source/WebKit/chromium/tests/CCOcclusionTrackerTest.cpp:90
> > +    void resetLayerScissorRect() { m_overrideLayerScissorRect = false; }
> 
> bikeshed nit: This is kind of an opaque function name to my ears.  I usually expect a "reset" function to clear something, but instead it often makes the scissor rect even more constraining.  This is more like "useDefaultLayerScissorRect" or "clearScissorRectOverride".  Another option that might be clearer would be to just set the scissor rect explicitly to whatever you want it to be at all times, always using an override.

Thanks, done.

[WebKit Review Bot]

Comment on attachment 130492 [details]
Patch

Clearing flags on attachment: 130492

Committed r110020: <http://trac.webkit.org/changeset/110020>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Shawn Singh]

Comment on attachment 130492 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:298
> +    if (!layer->clipRect().isEmpty())

I know this patch already landed, but this line of code may be fishy.  Enne and I are wracking our brains right now to identify where clipRects might accidentally be used without usesLayerClipping() == true.  This is certainly not the source of our bugs, but I found it along the way.  should this condition be "if (usesLayerClipping())" instead of checking whether the clipRect is empty?

[Dana Jansens]

Comment on attachment 130492 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:298
>> +    if (!layer->clipRect().isEmpty())
> 
> I know this patch already landed, but this line of code may be fishy.  Enne and I are wracking our brains right now to identify where clipRects might accidentally be used without usesLayerClipping() == true.  This is certainly not the source of our bugs, but I found it along the way.  should this condition be "if (usesLayerClipping())" instead of checking whether the clipRect is empty?

I'm not sure.. I thought that the clipRect was empty when it was not meant to be used. Since "real" empty clip rects get filtered out by calcDrawEtc. I was also confused about this issue and didn't find places that checked usesLayerClipping() except in RenderSurface as I recall. But I don't really know.. It's landed but I'm happy to fix it if it's wrong. It's not really clear to me though atm.

[Dana Jansens]

Comment on attachment 130492 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:298
>> +    if (!layer->clipRect().isEmpty())
> 
> I know this patch already landed, but this line of code may be fishy.  Enne and I are wracking our brains right now to identify where clipRects might accidentally be used without usesLayerClipping() == true.  This is certainly not the source of our bugs, but I found it along the way.  should this condition be "if (usesLayerClipping())" instead of checking whether the clipRect is empty?

I'm not sure.. I thought that the clipRect was empty when it was not meant to be used. Since "real" empty clip rects get filtered out by calcDrawEtc. I was also confused about this issue and didn't find places that checked usesLayerClipping() except in RenderSurface as I recall. But I don't really know.. It's landed but I'm happy to fix it if it's wrong. It's not really clear to me though atm.