Bug 95109 – [chromium] A general mechanism for passing data into and out of appendQuads and quadCuller via CCAppendQuadsData

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

[Dana Jansens]

[chromium] A general mechanism for passing data into and out of appendQuads and quadCuller via CCAppendQuadsData

[Dana Jansens]

Created attachment 160771 [details]
Patch

[WebKit Review Bot]

Comment on attachment 160771 [details]
Patch

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

[Dana Jansens]

Oops I missed the new header, I'll rebase and upload again when 94957 lands.

[Dana Jansens]

Created attachment 160796 [details]
Patch

[Adrienne Walker]

Comment on attachment 160796 [details]
Patch

The CCQuadSinkData hierarchy feels a little bit overengineered to me and I think it's more complex than its benefits.  I understand the need to reduce plumbing churn by putting common function signature parameters into a struct.  I'd prefer to see something more simple like:

struct CCAppendQuadsData
{
    bool hadMissingTiles;
    bool hadOcclusionFromOutsideTargetSurface;
    // FIXME: Hold the delegatedRenderPassId for the DelegatedRendererLayer.
};

I don't find it problematic that CCQuadCuller has a boolean about whether it has occlusion from outside the target surface.  The quad culler is stateful about the quads that have been appended to it, and this is extra information that it has calculated.  It feels no different from asking about occlusion/unocclusion in a rect or overdraw.

What do you think about having CCQuadSink::append just take a bool& hadExternalOcclusion? Alternatively, CCQuadSink::hasExternalOcclusion()?

[Dana Jansens]

(In reply to comment #5)
> (From update of attachment 160796 [details])
> The CCQuadSinkData hierarchy feels a little bit overengineered to me and I think it's more complex than its benefits.  I understand the need to reduce plumbing churn by putting common function signature parameters into a struct.  I'd prefer to see something more simple like:
> 
> struct CCAppendQuadsData
> {
>     bool hadMissingTiles;
>     bool hadOcclusionFromOutsideTargetSurface;
>     // FIXME: Hold the delegatedRenderPassId for the DelegatedRendererLayer.
> };
> 
> I don't find it problematic that CCQuadCuller has a boolean about whether it has occlusion from outside the target surface.  The quad culler is stateful about the quads that have been appended to it, and this is extra information that it has calculated.  It feels no different from asking about occlusion/unocclusion in a rect or overdraw.
> 
> What do you think about having CCQuadSink::append just take a bool& hadExternalOcclusion? Alternatively, CCQuadSink::hasExternalOcclusion()?

I feel like the hadExternalOcclusion information is very similar to hadMissingTiles, but they are channeled through completely different paths. I like append() taking a bool& more, but we're just getting more and more parameters to CCLayerImpl::appendQuads(quadSink, bool&, bool&, CCRenderPassList&) and so on into the sunset.

We could just use a CCAppendQuadsData struct and have QuadSink take that. I was just trying to keep it more abstract-y.. for better or worse. Would that feel better to you?

[Adrienne Walker]

(In reply to comment #6)

> I feel like the hadExternalOcclusion information is very similar to hadMissingTiles, but they are channeled through completely different paths. I like append() taking a bool& more, but we're just getting more and more parameters to CCLayerImpl::appendQuads(quadSink, bool&, bool&, CCRenderPassList&) and so on into the sunset.

...but CCQuadSink won't need an additional parameter in the future, though.  CCLayerImpl::appendQuads could continue taking CCAppendQuadsData& which could expand later, but the underlying CCQuadSink::append function could just take a bool&.

[Dana Jansens]

(In reply to comment #7)
> (In reply to comment #6)
> 
> > I feel like the hadExternalOcclusion information is very similar to hadMissingTiles, but they are channeled through completely different paths. I like append() taking a bool& more, but we're just getting more and more parameters to CCLayerImpl::appendQuads(quadSink, bool&, bool&, CCRenderPassList&) and so on into the sunset.
> 
> ...but CCQuadSink won't need an additional parameter in the future, though.  CCLayerImpl::appendQuads could continue taking CCAppendQuadsData& which could expand later, but the underlying CCQuadSink::append function could just take a bool&.

Oh I see. But then every layer has to unpack the bool& from the struct in order to call append, instead of just passing along the forward-declared struct.

[Adrienne Walker]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > 
> > > I feel like the hadExternalOcclusion information is very similar to hadMissingTiles, but they are channeled through completely different paths. I like append() taking a bool& more, but we're just getting more and more parameters to CCLayerImpl::appendQuads(quadSink, bool&, bool&, CCRenderPassList&) and so on into the sunset.
> > 
> > ...but CCQuadSink won't need an additional parameter in the future, though.  CCLayerImpl::appendQuads could continue taking CCAppendQuadsData& which could expand later, but the underlying CCQuadSink::append function could just take a bool&.
> 
> Oh I see. But then every layer has to unpack the bool& from the struct in order to call append, instead of just passing along the forward-declared struct.

It's true.  Although, that way only CCLayerImpl really has to know about CCAppendQuadsData?

If you don't like that, I think I'm also ok with your other suggestion of CCQuadSink just taking a CCAppendQuadsData&.  It seems about equivalent.

[Dana Jansens]

Created attachment 160831 [details]
Patch

simplified to a single struct

[Adrienne Walker]

Comment on attachment 160831 [details]
Patch

R=me.

[WebKit Review Bot]

Comment on attachment 160831 [details]
Patch

Clearing flags on attachment: 160831

Committed r126819: <http://trac.webkit.org/changeset/126819>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.