Bug 85414 – [chromium] Show borders for partial-draw-culled quads to visualize culling behaviour

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dana Jansens]

[chromium] Show brown borders for partial-draw-culled quads to visualize culling behaviour

[Dana Jansens]

Created attachment 139872 [details]
Patch

[Adrienne Walker]

Comment on attachment 139872 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        The borders are brown, and are only shown when the quad's visible rect

(This is somewhat of an aside, but do you have any thoughts on how we can make the meanings of our rainbow salad of border colors more clear? Maybe we could put a link to a legend somewhere? Or even display a legend on the screen somehow?)

> Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:68
> +    if (keepQuad) {

...so this only applies for partially culled quads and not fully culled ones? Is that just to prevent too many debug borders on screen?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.cpp:68
> +    if (layer->hasDebugBorders())

(I keep thinking this should be a setting somewhere and not a per-layer flag.  Or, better yet, a bag of settings so you could turn on the things you cared about, since we seem to keep adding more and more border types.  Not in this patch, I guess.)

> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.cpp:91
> +        quadCuller.createCulledDebugBordersWithSharedQuadState(sharedQuadState.get());
>      quadCuller.appendSurface(CCRenderSurfaceDrawQuad::create(sharedQuadState.get(), surface->contentRect(), layer, surfaceDamageRect(), isReplica));
> +    quadCuller.createCulledDebugBordersWithSharedQuadState(0);

Whoa there, spooky action at a distance.  Also, this has bad ownership issues--quads have naked pointers to shared quad state and the owner of the shared quad state is CCRenderPass::m_sharedQuadStateList.  You can't just stick any old pointer to an shared object state into a quad that you then delete.

Why can't the partially culled border quad re-use the same shared quad state and just get that from the quad itself? It looks like you're using the border shared quad state as a boolean.  Maybe CCQuadCuller::appendFoo should just take an enum about whether or not to append culling quads, rather than depending on the nullness of a member variable set elsewhere?

[Dana Jansens]

Comment on attachment 139872 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        The borders are brown, and are only shown when the quad's visible rect
> 
> (This is somewhat of an aside, but do you have any thoughts on how we can make the meanings of our rainbow salad of border colors more clear? Maybe we could put a link to a legend somewhere? Or even display a legend on the screen somehow?)

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

But maybe we could do more on-screen..

>> Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:68
>> +    if (keepQuad) {
> 
> ...so this only applies for partially culled quads and not fully culled ones? Is that just to prevent too many debug borders on screen?

A fully culled quad would be an empty rect, not very interesting to display. This is putting a rect around what is left after culling.

>> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.cpp:91
>> +    quadCuller.createCulledDebugBordersWithSharedQuadState(0);
> 
> Whoa there, spooky action at a distance.  Also, this has bad ownership issues--quads have naked pointers to shared quad state and the owner of the shared quad state is CCRenderPass::m_sharedQuadStateList.  You can't just stick any old pointer to an shared object state into a quad that you then delete.
> 
> Why can't the partially culled border quad re-use the same shared quad state and just get that from the quad itself? It looks like you're using the border shared quad state as a boolean.  Maybe CCQuadCuller::appendFoo should just take an enum about whether or not to append culling quads, rather than depending on the nullness of a member variable set elsewhere?

So I had at first added sharedQuadState() on CCDrawQuad, but I thought you would not like that, as there are all the helper methods on CCDrawQuad to avoid accessing it directly. I'm happy to do that instead.

I don't see the ownership issue here though.. The shared quad state is owned by the RenderPass, and it gives it to the quad culler to stick on quads it creates. Those quads also end up in this RenderPass's quadList.

I had a bool instead of a pointer here, when I got the sharedQuadState from the quad directly, I can do that again.

[Dana Jansens]

Comment on attachment 139872 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.cpp:68
>> +    if (layer->hasDebugBorders())
> 
> (I keep thinking this should be a setting somewhere and not a per-layer flag.  Or, better yet, a bag of settings so you could turn on the things you cared about, since we seem to keep adding more and more border types.  Not in this patch, I guess.)

I think so too.

[Dana Jansens]

Created attachment 140291 [details]
Patch

[Dana Jansens]

I'd rather not pass in something with appendFoo() because then we have to push that down to all the layer classes as well. This way the RenderPass can make the decision and the layers don't need to know about it. Wdyt?

[Adrienne Walker]

(In reply to comment #3)
> I don't see the ownership issue here though.. The shared quad state is owned by the RenderPass, and it gives it to the quad culler to stick on quads it creates. Those quads also end up in this RenderPass's quadList.

Oh, I see.  The "create" function was really just a "hang on to this pointer" function and not a "make a copy" function.  My mistake.

(In reply to comment #6)
> I'd rather not pass in something with appendFoo() because then we have to push that down to all the layer classes as well. This way the RenderPass can make the decision and the layers don't need to know about it. Wdyt?

There's some code smell for me in setting and unsetting a flag in calling code to cause behavior elsewhere.

I guess the real fix is to instantiate the quad culler with a flag from CCSettings about layer borders.  So, in the short term, I guess this is ok.

[Adrienne Walker]

Comment on attachment 140291 [details]
Patch

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

R=me.

> Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.h:46
> +    void createCulledDebugBorderQuads(bool debug) { m_createCulledDebugBorderQuads = debug; }

"create" here sounds like this function is doing something, rather than setting a flag.  setCreateCulledDebugBorderQuads, maybe?

[Dana Jansens]

(In reply to comment #7)
> 
> There's some code smell for me in setting and unsetting a flag in calling code to cause behavior elsewhere.
> 
> I guess the real fix is to instantiate the quad culler with a flag from CCSettings about layer borders.  So, in the short term, I guess this is ok.

We could instantiate the quadCuller with a flag from the layer, and not do this set/unset dance, if quadCuller can tell if the quad being added is already a debug quad.

CCDrawQuad::isDebugQuad() const { return material() == DebugWhatever; } ?

[Dana Jansens]

Created attachment 140818 [details]
Patch

[Dana Jansens]

Added CCDrawQuad::isDebugQuad() and put the "showCullingWithDebugBorderQuads" flag into CCQuadCuller constructor.

[Adrienne Walker]

Comment on attachment 140818 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCDrawQuad.h:78
> +    bool isDebugQuad() const { return m_material != DebugBorder; }

ಠ_ಠ

[Dana Jansens]

(In reply to comment #12)
> (From update of attachment 140818 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140818&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCDrawQuad.h:78
> > +    bool isDebugQuad() const { return m_material != DebugBorder; }
> 
> ಠ_ಠ

Hah.. oops.

I guess you can't tell this apart in use, because the culling debug quads are only there if the other debug quads are, and as long as you show it for one and not the other, you get the same result. :P Fixing...

[Dana Jansens]

Created attachment 140836 [details]
Patch

[Adrienne Walker]

Comment on attachment 140836 [details]
Patch

Thanks.  R=me.

[Dana Jansens]

Comment on attachment 140836 [details]
Patch

Thanks too :)

[WebKit Review Bot]

Comment on attachment 140836 [details]
Patch

Clearing flags on attachment: 140836

Committed r116481: <http://trac.webkit.org/changeset/116481>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.