Bug 94752 – [chromium] Create sharedQuadState at same time as creating quads and give them to the quadSink

• Component: New Bugs
• Status: RESOLVED FIXED

[Dana Jansens]

[chromium] Create sharedQuadState at same time as creating quads and give them to the quadSink

[Dana Jansens]

Created attachment 160027 [details]
Patch

[Adrienne Walker]

Comment on attachment 160027 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:101
> +int CCQuadCuller::generateSharedQuadStateId() const
> +{
> +    return m_sharedQuadStateList.size();
> +}

A potential foot gun is that you might think you could call generateSharedQuadStateId() n times and then add n states, which is wrong.  

As a potential suggestion, what about always having to appendSharedQuadState first prior to appending quads.  That function sets the id internally on the shared quad state and returns a raw pointer to the modified shared quad state (for convenience, since you released the other pointer).  You would use this pointer to pass to quads that you append (since you set the shared quad state id on quads and need it there).  Maybe CCDrawQuad could assert if you gave it a shared quad state that hadn't been appended yet.  At least that'd be a runtime check that you'd done the right thing.

[Dana Jansens]

Comment on attachment 160027 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:101
>> +}
> 
> A potential foot gun is that you might think you could call generateSharedQuadStateId() n times and then add n states, which is wrong.  
> 
> As a potential suggestion, what about always having to appendSharedQuadState first prior to appending quads.  That function sets the id internally on the shared quad state and returns a raw pointer to the modified shared quad state (for convenience, since you released the other pointer).  You would use this pointer to pass to quads that you append (since you set the shared quad state id on quads and need it there).  Maybe CCDrawQuad could assert if you gave it a shared quad state that hadn't been appended yet.  At least that'd be a runtime check that you'd done the right thing.

Brilliant. I was not satisfied with this either but was not sure what to do with it, this is perfect. Thanks.

[Dana Jansens]

Created attachment 160206 [details]
Patch

[Dana Jansens]

Done!

[Adrienne Walker]

Comment on attachment 160206 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:106
> +    m_currentSharedQuadState = sharedQuadState.get();

Why useSharedQuadState + this stateful 'current' variable only used for an assert vs. just appendQuadState? If you're worried about writing an ASSERT, you could always just make sure that the passDrawQuad's quad state id in the quad state list has the same pointer as the passDrawQuad's quad state pointer?

[Dana Jansens]

Comment on attachment 160206 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:106
>> +    m_currentSharedQuadState = sharedQuadState.get();
> 
> Why useSharedQuadState + this stateful 'current' variable only used for an assert vs. just appendQuadState? If you're worried about writing an ASSERT, you could always just make sure that the passDrawQuad's quad state id in the quad state list has the same pointer as the passDrawQuad's quad state pointer?

Actually I was thinking ahead to dropping the sharedQuadState if all quads for it were culled. So only allowing you to append quads when the sharedQuadState is active. Since this patch doesn't do that though, it doesn't need the m_current thing either, true.

[Adrienne Walker]

(In reply to comment #7)
> (From update of attachment 160206 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=160206&action=review
> 
> >> Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:106
> >> +    m_currentSharedQuadState = sharedQuadState.get();
> > 
> > Why useSharedQuadState + this stateful 'current' variable only used for an assert vs. just appendQuadState? If you're worried about writing an ASSERT, you could always just make sure that the passDrawQuad's quad state id in the quad state list has the same pointer as the passDrawQuad's quad state pointer?
> 
> Actually I was thinking ahead to dropping the sharedQuadState if all quads for it were culled. So only allowing you to append quads when the sharedQuadState is active. Since this patch doesn't do that though, it doesn't need the m_current thing either, true.

Ohhh, that makes a lot more sense.

[Adrienne Walker]

Comment on attachment 160206 [details]
Patch

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

R=me, with a few nits.

> Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:87
> +    ASSERT(passDrawQuad->sharedQuadState() == m_currentSharedQuadState);

Could you also assert here that m_sharedQuadStateList[passDrawQuad->sharedQuadStateId()] == m_currentSharedQuadState and that m_currentSharedQuadState->id() == passDrawQuad->sharedQuadStateId()? There's a lot of duplicate state running around (which I think is necessary), but I just want extra sanity checks that everything is as expected.

> Source/WebCore/platform/graphics/chromium/cc/CCQuadSink.h:44
> +    virtual CCSharedQuadState* useSharedQuadState(PassOwnPtr<CCSharedQuadState>) = 0;

Add a comment here describing that this needs to be set prior to calling append with a quad using it?

[Dana Jansens]

Created attachment 160218 [details]
Patch

Ok I added some more asserts, thanks. I don't want to assert that id == array index, because it doesn't have to be. So instead I assert that it's the last thing in the array (it's current so it should be most recently added).

I stuck the comment in CCQuadSink (and added the missing "CCQuadSink implementation" comment to CCQuadCuller).

[WebKit Review Bot]

Comment on attachment 160218 [details]
Patch

Clearing flags on attachment: 160218

Committed r126482: <http://trac.webkit.org/changeset/126482>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.