[chromium] Move WebFilterOperations from RenderPassDrawQuad to RenderPass
Created attachment 153549 [details] Patch
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 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 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..
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 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 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.
Created attachment 154100 [details] Patch
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 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)
Created attachment 154427 [details] remove-ephemeral-data-from-lrc
Created attachment 154431 [details] Patch
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.
Created attachment 154435 [details] Patch for landing
Comment on attachment 154435 [details] Patch for landing Clearing flags on attachment: 154435 Committed r123666: <http://trac.webkit.org/changeset/123666>
All reviewed patches have been landed. Closing bug.