Bug 95485 – [chromium] Add a copy() method to CCRenderPass

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

[Dana Jansens]

[chromium] Add a copy() method to CCRenderPass

[Dana Jansens]

Created attachment 161525 [details]
Patch

[Alexandre Elias]

I'm wondering if these copy methods are really needed?  In the child, the pickler can take references to the objects and then serialize them into a byte blob.  In the parent, it can populate objects from the byte blob.  Where does copying a CCRenderPass into another CCRenderPass come in?

[Dana Jansens]

The DelegatedRendererLayer has a set of RenderPasses. It needs to copy them into the FrameData's set of RenderPasses in order to have them be part of the frame. (It's unrelated to IPC)

[Alexandre Elias]

What if the DelegatedRendererLayer held a FrameData in the first place?  Seems we should be able to come up with some ownership model that avoids copying.

[Dana Jansens]

It still has to copy from its own FrameData into the CCLTHI's FrameData.

You can do it without copying certainly but it complicates the code some. Currently CCLTHI::FrameData::renderPassedById owns all the renderPasses in the frame. Then suddenly it could only own some, with some DelegatedRendererLayer (DRL) owning others.

Also it complicates updating the DRL. If it gets a new set of RenderPasses now it has to keep its old RenderPasses alive until the frame is done with them.

As enne put it to me, other layers create quads/renderPasses every frame, so I don't think a copy is going to be bad here.

[Dana Jansens]

Created attachment 161538 [details]
Patch

Make the copy() test fail when a new field is added to CCRenderPass without some user intervention in a cross-platform way. And verify that quads are not copied.

[Dana Jansens]

Created attachment 161539 [details]
Patch

fix changelog

[Dana Jansens]

To further discuss copying, it's more complicated for quads as well. A renderPass owns quads. But the quads in the root renderPass within the DRL need to be merged into the targetRenderPass for the layer. So they would need to be unowned by the DRL, owned by the targetRenderPass, then given back to the DRL after the frame was done... copying is so much simpler in this case. And I think using the same ownership model for quads in non-root render passes within DRL will make our lives much easier.

[Alexandre Elias]

OK, sounds good, thanks for explaining.

[Dana Jansens]

Created attachment 162312 [details]
Patch

rebase

[Adrienne Walker]

Comment on attachment 162312 [details]
Patch

R=me.

aelias: I had the same mental balking about the extra copying, but I'm willing to take the software complexity vs. execution time tradeoff in the short term until we figure out that it's too slow.  That being said, there's probably simpler optimizations to decrease copying and number of allocations than changing the ownership model, in my opinion.

[Dana Jansens]

Created attachment 162326 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 162326 [details]
Patch for landing

Clearing flags on attachment: 162326

Committed r127715: <http://trac.webkit.org/changeset/127715>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.