Bug 91885 - [chromium] Move WebFilterOperations from RenderPassDrawQuad to RenderPass
Summary: [chromium] Move WebFilterOperations from RenderPassDrawQuad to RenderPass
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dana Jansens
URL:
Keywords:
Depends on:
Blocks: 92294
  Show dependency treegraph
 
Reported: 2012-07-20 11:19 PDT by Dana Jansens
Modified: 2012-07-25 15:08 PDT (History)
8 users (show)

See Also:


Attachments
Patch (21.53 KB, patch)
2012-07-20 11:24 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (28.48 KB, patch)
2012-07-24 10:39 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
remove-ephemeral-data-from-lrc (36.30 KB, patch)
2012-07-25 13:34 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (60.45 KB, patch)
2012-07-25 13:42 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch for landing (28.49 KB, patch)
2012-07-25 13:48 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dana Jansens 2012-07-20 11:19:55 PDT
[chromium] Move WebFilterOperations from RenderPassDrawQuad to RenderPass
Comment 1 Dana Jansens 2012-07-20 11:24:27 PDT
Created attachment 153549 [details]
Patch
Comment 2 Alexandre Elias 2012-07-20 11:38:33 PDT
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.)
Comment 3 Adrienne Walker 2012-07-20 12:19:22 PDT
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.
Comment 4 Dana Jansens 2012-07-20 12:23:52 PDT
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..
Comment 5 Alexandre Elias 2012-07-20 12:47:48 PDT
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.
Comment 6 Adrienne Walker 2012-07-20 13:32:09 PDT
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.
Comment 7 Dana Jansens 2012-07-20 13:33:29 PDT
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.
Comment 8 Dana Jansens 2012-07-24 10:39:30 PDT
Created attachment 154100 [details]
Patch
Comment 9 Adrienne Walker 2012-07-24 10:50:18 PDT
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.
Comment 10 Dana Jansens 2012-07-24 10:55:12 PDT
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)
Comment 11 Dana Jansens 2012-07-25 13:34:28 PDT
Created attachment 154427 [details]
remove-ephemeral-data-from-lrc
Comment 12 Dana Jansens 2012-07-25 13:42:34 PDT
Created attachment 154431 [details]
Patch
Comment 13 Dana Jansens 2012-07-25 13:43:53 PDT
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.
Comment 14 Dana Jansens 2012-07-25 13:48:49 PDT
Created attachment 154435 [details]
Patch for landing
Comment 15 WebKit Review Bot 2012-07-25 15:08:12 PDT
Comment on attachment 154435 [details]
Patch for landing

Clearing flags on attachment: 154435

Committed r123666: <http://trac.webkit.org/changeset/123666>
Comment 16 WebKit Review Bot 2012-07-25 15:08:17 PDT
All reviewed patches have been landed.  Closing bug.