Bug 91885 – [chromium] Move WebFilterOperations from RenderPassDrawQuad to RenderPass

• Component: New Bugs
• Status: RESOLVED FIXED

[Dana Jansens]

[chromium] Move WebFilterOperations from RenderPassDrawQuad to RenderPass

[Dana Jansens]

Created attachment 153549 [details]
Patch

[Alexandre Elias]

LGTM.  This also means it's ready to be moved into WebKit::WebCompositorRenderPassQuad. 

This doesn't block this patch, but note the WebFilterOperations serialization at the RenderPass level still needs more work -- I was thinking the WebKit-side equivalent structure to CCRenderPass could contain a WebVector<WebCompositorFilterOperation>.  But it would be much easier to deal with if WebCompositorFilterOperation was a pure POD struct.  I already tried to write a custom pickler for it but ran into difficulties -- the C++ encapsulation niceties seem like more trouble than they're worth in this case.  (WebCompositorFilterOperations with an 's' on the other hand, won't be serialized and can remain as is.)

[Adrienne Walker]

Comment on attachment 153549 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:339
>      for (size_t i = 0; i < renderPassesInDrawOrder.size(); ++i)
> -        passesInFrame.set(renderPassesInDrawOrder[i]->id(), renderPassesInDrawOrder[i]);
> +        m_renderPassesInFrame.set(renderPassesInDrawOrder[i]->id(), renderPassesInDrawOrder[i]);

Hmm.  The problem with member variables is that they persist and so it becomes more complicated about who sets them, when they're valid, and when they need to be cleared.

I would feel better if m_defaultRenderPass and m_renderPassesInFrame were only valid during drawFrame or alternatively if you asked the CCRendererClient for these values.

[Dana Jansens]

Comment on attachment 153549 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:339
>> +        m_renderPassesInFrame.set(renderPassesInDrawOrder[i]->id(), renderPassesInDrawOrder[i]);
> 
> Hmm.  The problem with member variables is that they persist and so it becomes more complicated about who sets them, when they're valid, and when they need to be cleared.
> 
> I would feel better if m_defaultRenderPass and m_renderPassesInFrame were only valid during drawFrame or alternatively if you asked the CCRendererClient for these values.

Me too! I was trying various things. They need to exist in decideRenderPassAllocations and drawFrame. One is called from prepareToDraw and one from drawLayers. And drawLayers isn't always called after prepare to draw so I was a little meh about how to do it.

Using the client might be very elegant..

[Alexandre Elias]

I'd prefer if decideRenderPassAllocationsForFrame() was made private to LayerRendererChromium and called from drawFrame().  The only problem is the "culling passes with cached textures" in between the decide and the final draw.  I lack context on what this is and how we could refactor that away though.

[Adrienne Walker]

Comment on attachment 153549 [details]
Patch

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

>>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:339
>>> +        m_renderPassesInFrame.set(renderPassesInDrawOrder[i]->id(), renderPassesInDrawOrder[i]);
>> 
>> Hmm.  The problem with member variables is that they persist and so it becomes more complicated about who sets them, when they're valid, and when they need to be cleared.
>> 
>> I would feel better if m_defaultRenderPass and m_renderPassesInFrame were only valid during drawFrame or alternatively if you asked the CCRendererClient for these values.
> 
> Me too! I was trying various things. They need to exist in decideRenderPassAllocations and drawFrame. One is called from prepareToDraw and one from drawLayers. And drawLayers isn't always called after prepare to draw so I was a little meh about how to do it.
> 
> Using the client might be very elegant..

Why does it need to exist in prepareToDraw? It didn't exist there before.

[Dana Jansens]

Comment on attachment 153549 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:339
>>>> +        m_renderPassesInFrame.set(renderPassesInDrawOrder[i]->id(), renderPassesInDrawOrder[i]);
>>> 
>>> Hmm.  The problem with member variables is that they persist and so it becomes more complicated about who sets them, when they're valid, and when they need to be cleared.
>>> 
>>> I would feel better if m_defaultRenderPass and m_renderPassesInFrame were only valid during drawFrame or alternatively if you asked the CCRendererClient for these values.
>> 
>> Me too! I was trying various things. They need to exist in decideRenderPassAllocations and drawFrame. One is called from prepareToDraw and one from drawLayers. And drawLayers isn't always called after prepare to draw so I was a little meh about how to do it.
>> 
>> Using the client might be very elegant..
> 
> Why does it need to exist in prepareToDraw? It didn't exist there before.

Ya decideAllocations has been in prepareToDraw because its used to decide which renderpasses should be in the frame (based on what is in cache), and this is what prepareToDraw does atm, make a FrameData that can be drawn as is.

[Dana Jansens]

Created attachment 154100 [details]
Patch

[Adrienne Walker]

Comment on attachment 154100 [details]
Patch

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

R=me.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:383
> +    m_renderPassesById = &renderPassesById;

Thanks, that's way better.  Theoretically, if you wanted I suppose you could get rid of both m_defaultRenderPass and m_renderPassesById by passing them through drawRenderPass/drawQuad like rootScissorRectInCurrentSurface is.  It's not necessary, but just a thought.

[Dana Jansens]

Comment on attachment 154100 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:383
>> +    m_renderPassesById = &renderPassesById;
> 
> Thanks, that's way better.  Theoretically, if you wanted I suppose you could get rid of both m_defaultRenderPass and m_renderPassesById by passing them through drawRenderPass/drawQuad like rootScissorRectInCurrentSurface is.  It's not necessary, but just a thought.

Thanks. The renderPassesById would need to be passed additionally to the drawRenderPassQuad method. It was a lot of plumbing that I thot cluttered things up.. so I'm not sure if it's a good plan.

Also, this is going to crash the HUD as it uses m_defaultRenderPass, and it draws after drawFrame is done. Lemme take another crack at this..

(We have no unit tests that exercise that apparently)

[Dana Jansens]

Created attachment 154427 [details]
remove-ephemeral-data-from-lrc

[Dana Jansens]

Created attachment 154431 [details]
Patch

[Dana Jansens]

The 2nd last patch is just the changes since the R+.

The whole HUD crash problem went away by rebase, but this additional change cleans up the "which variables are valid when" issues tremendously.

[Dana Jansens]

Created attachment 154435 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 154435 [details]
Patch for landing

Clearing flags on attachment: 154435

Committed r123666: <http://trac.webkit.org/changeset/123666>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.