Bug 91023 – [chromium] Remove the RenderPass pointer from RenderPassDrawQuad

• Component: New Bugs
• Status: RESOLVED FIXED

[Dana Jansens]

[chromium] Remove the RenderPass pointer from RenderPassDrawQuad

[Dana Jansens]

Created attachment 151795 [details]
Patch

[Dana Jansens]

Created attachment 151800 [details]
Patch

rebased

[Alexandre Elias]

Thanks for tackling this.

Hmm, this is a good step, but ultimately we'll want to serialize FrameData or a structure derived from it, and the HashMap will be unserializable and we'd have to reconstruct it on the other end.  Given that render-passes all live inside CCRenderPassList objects, could we use an array index instead?

[Dana Jansens]

(In reply to comment #3)
> Thanks for tackling this.
> 
> Hmm, this is a good step, but ultimately we'll want to serialize FrameData or a structure derived from it, and the HashMap will be unserializable and we'd have to reconstruct it on the other end.  Given that render-passes all live inside CCRenderPassList objects, could we use an array index instead?

My thinking was that the renderPassId is going to be globally unique across all compositors. This way when we give the RenderPass and quads to another compositor, it doesn't need to rewrite the ids to new array indexes (potential bugs).

Also, I don't think we are going to serialize the FrameData as it is (it also includes the RenderSurfaceLayerList). I think we'll just be passing out the CCRenderPassList from FrameData alone right?

[Alexandre Elias]

(In reply to comment #4)
> (In reply to comment #3)
> > Thanks for tackling this.
> > 
> > Hmm, this is a good step, but ultimately we'll want to serialize FrameData or a structure derived from it, and the HashMap will be unserializable and we'd have to reconstruct it on the other end.  Given that render-passes all live inside CCRenderPassList objects, could we use an array index instead?
> 
> My thinking was that the renderPassId is going to be globally unique across all compositors. This way when we give the RenderPass and quads to another compositor, it doesn't need to rewrite the ids to new array indexes (potential bugs).

Hmm, but how do you plan to enforce globally unique IDs across processes?  I see a TODO for that in CCLayerTreeHostImpl::calculateRenderPasses.

I don't think we'll want the parent compositor to just mix in the new render passes with its own top-level list anyway.  There are also dependency (order-of-execution) problems, and the question of how the CCQuadListLayer will apply its transformations, etc.  I think we can expect a kind of tree structure in the receiver, which will allow index-based IDs to work within individual nodes.

> 
> Also, I don't think we are going to serialize the FrameData as it is (it also includes the RenderSurfaceLayerList). I think we'll just be passing out the CCRenderPassList from FrameData alone right?

I haven't figured yet what is going to be directly serialized and what's going to be reconstructed at the top level.  Generally speaking though, it'll be less overhead to work with anything that's closer to its serialized state.

[Dana Jansens]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > Thanks for tackling this.
> > > 
> > > Hmm, this is a good step, but ultimately we'll want to serialize FrameData or a structure derived from it, and the HashMap will be unserializable and we'd have to reconstruct it on the other end.  Given that render-passes all live inside CCRenderPassList objects, could we use an array index instead?
> > 
> > My thinking was that the renderPassId is going to be globally unique across all compositors. This way when we give the RenderPass and quads to another compositor, it doesn't need to rewrite the ids to new array indexes (potential bugs).
> 
> Hmm, but how do you plan to enforce globally unique IDs across processes?  I see a TODO for that in CCLayerTreeHostImpl::calculateRenderPasses.

Each compositor has an ID. RenderPass id high bits are compositor id.

> I don't think we'll want the parent compositor to just mix in the new render passes with its own top-level list anyway.  There are also dependency (order-of-execution) problems, and the question of how the CCQuadListLayer will apply its transformations, etc.  I think we can expect a kind of tree structure in the receiver, which will allow index-based IDs to work within individual nodes.

The ordering should be okay. If you have a layer reprensenting a compositor, when it would insert its quad, you instead insert all the quads from the root render pass of the nested compositor. These quads need their opacity/transforms adjusted as they are inserted.

If the layer is rotated or non-opaque or something and needs a surface, you insert a renderpass drawquad instead pointing to the root of the nested compositor. Either way, then you insert all the passes from the nested compositor right before the current render pass.

I think we benefit from just inserting them into the hosting compositors list of render passes directly. We don't need anything more complex.

> > Also, I don't think we are going to serialize the FrameData as it is (it also includes the RenderSurfaceLayerList). I think we'll just be passing out the CCRenderPassList from FrameData alone right?
> 
> I haven't figured yet what is going to be directly serialized and what's going to be reconstructed at the top level.  Generally speaking though, it'll be less overhead to work with anything that's closer to its serialized state.

Seems like just a list of RenderPasses (each holding a list of quads) and the list of SharedQuadState is all we need. FrameData has a lot of other things in it that we just don't need for drawing. The only things given to LRC are the RenderPasses in the FrameData::renderPasses list.

[Alexandre Elias]

OK, sounds reasonable.  We can go with the HashSet.

(In reply to comment #6)
> > Hmm, but how do you plan to enforce globally unique IDs across processes?  I see a TODO for that in CCLayerTreeHostImpl::calculateRenderPasses.
> 
> Each compositor has an ID. RenderPass id high bits are compositor id.

Let's remove the FIXME in CCLayerTreeHostImpl::calculateRenderPasses if that's already the case then.

[Antoine Labour]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (In reply to comment #3)
> > > > Thanks for tackling this.
> > > > 
> > > > Hmm, this is a good step, but ultimately we'll want to serialize FrameData or a structure derived from it, and the HashMap will be unserializable and we'd have to reconstruct it on the other end.  Given that render-passes all live inside CCRenderPassList objects, could we use an array index instead?
> > > 
> > > My thinking was that the renderPassId is going to be globally unique across all compositors. This way when we give the RenderPass and quads to another compositor, it doesn't need to rewrite the ids to new array indexes (potential bugs).
> > 
> > Hmm, but how do you plan to enforce globally unique IDs across processes?  I see a TODO for that in CCLayerTreeHostImpl::calculateRenderPasses.
> 
> Each compositor has an ID. RenderPass id high bits are compositor id.

But that means compositors themselves need to enforce a globally-unique id, which means a round-trip at compositor creation time... meh. In particular, if a child renderer becomes malicious it could start mucking around with other render passes and do bad things. So we'd need to add a verification step in the parent compositor anyway.
I think it's easier to do a remapping in the parent.


> 
> > I don't think we'll want the parent compositor to just mix in the new render passes with its own top-level list anyway.  There are also dependency (order-of-execution) problems, and the question of how the CCQuadListLayer will apply its transformations, etc.  I think we can expect a kind of tree structure in the receiver, which will allow index-based IDs to work within individual nodes.
> 
> The ordering should be okay. If you have a layer reprensenting a compositor, when it would insert its quad, you instead insert all the quads from the root render pass of the nested compositor. These quads need their opacity/transforms adjusted as they are inserted.
> 
> If the layer is rotated or non-opaque or something and needs a surface, you insert a renderpass drawquad instead pointing to the root of the nested compositor. Either way, then you insert all the passes from the nested compositor right before the current render pass.
> 
> I think we benefit from just inserting them into the hosting compositors list of render passes directly. We don't need anything more complex.
> 
> > > Also, I don't think we are going to serialize the FrameData as it is (it also includes the RenderSurfaceLayerList). I think we'll just be passing out the CCRenderPassList from FrameData alone right?
> > 
> > I haven't figured yet what is going to be directly serialized and what's going to be reconstructed at the top level.  Generally speaking though, it'll be less overhead to work with anything that's closer to its serialized state.
> 
> Seems like just a list of RenderPasses (each holding a list of quads) and the list of SharedQuadState is all we need. FrameData has a lot of other things in it that we just don't need for drawing. The only things given to LRC are the RenderPasses in the FrameData::renderPasses list.

[Dana Jansens]

(In reply to comment #8)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > (In reply to comment #4)
> > > > (In reply to comment #3)
> > > > > Thanks for tackling this.
> > > > > 
> > > > > Hmm, this is a good step, but ultimately we'll want to serialize FrameData or a structure derived from it, and the HashMap will be unserializable and we'd have to reconstruct it on the other end.  Given that render-passes all live inside CCRenderPassList objects, could we use an array index instead?
> > > > 
> > > > My thinking was that the renderPassId is going to be globally unique across all compositors. This way when we give the RenderPass and quads to another compositor, it doesn't need to rewrite the ids to new array indexes (potential bugs).
> > > 
> > > Hmm, but how do you plan to enforce globally unique IDs across processes?  I see a TODO for that in CCLayerTreeHostImpl::calculateRenderPasses.
> > 
> > Each compositor has an ID. RenderPass id high bits are compositor id.
> 
> But that means compositors themselves need to enforce a globally-unique id, which means a round-trip at compositor creation time... meh. In particular, if a child renderer becomes malicious it could start mucking around with other render passes and do bad things. So we'd need to add a verification step in the parent compositor anyway.
> I think it's easier to do a remapping in the parent.

True, fair enough. I can remove that FIXME in a followup or here if this needs to go another review round.

[Adrienne Walker]

Comment on attachment 151800 [details]
Patch

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

I think I agree with Dana that the serialized bit of FrameData is renderPasses and maybe occludingScreenSpaceRects.  Everything else is there as a cached calculated value (skippedPasses, renderPassesById), or as a hook to get debug rect information that needs to be refactored (renderSurfaceLayerList), or as a callback mechanism that only the embedder needs (willDrawLayers).  All of these seem like things that need to either stay in the embedded compositor or be recalculated as needed in the host compositor.

FrameData seems a bit of a tangle and likely should be split up better around its various concerns.  I don't think this patch makes it any worse.

> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:2968
> +    std::map<char, RenderPassCacheEntry> renderPassCache;

Unused?

> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:2969
> +    OwnPtr<CCSharedQuadState> sharedQuadState;

No.  This will then go out of scope after this function returns and the pointer will be stale on the test quads.

[Dana Jansens]

Comment on attachment 151800 [details]
Patch

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

>> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:2969
>> +    OwnPtr<CCSharedQuadState> sharedQuadState;
> 
> No.  This will then go out of scope after this function returns and the pointer will be stale on the test quads.

Ohh. Ok. Let me just leave these 2 in the testData struct. I thot they were only used here so just keep them local but there's other concerns to consider clearly.

[Dana Jansens]

Created attachment 152010 [details]
Patch

- removed fixme for global renderpass id since we'll map it in the host compositor
- changed ownership around for RenderPasses so I can remove skippedPasses from FrameData. now the HashMap owns all passes. The renderPasses list is just pointers that will actually be drawn. This will be more friendly in ubercomp land, where we want to hold onto passes we didn't generate across frames and possibly use them/not use them depending on cached textures.

The ownership change exposed a bug that a unit test causes - a root layer with opacity < 1 tries to make another RenderSurface, causing it to appear in the RSLL twice. Fixing this separately.

[Dana Jansens]

Created attachment 152054 [details]
Patch

rebase

[Adrienne Walker]

Comment on attachment 152054 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:321
>      HashMap<int, const CCRenderPass*> passesInFrame;
>      for (size_t i = 0; i < renderPassesInDrawOrder.size(); ++i)
> -        passesInFrame.set(renderPassesInDrawOrder[i]->id(), renderPassesInDrawOrder[i].get());
> +        passesInFrame.set(renderPassesInDrawOrder[i]->id(), renderPassesInDrawOrder[i]);

Is this passesInFrame hash redundant? Should decideRenderPassAllocationsForFrame just take FrameData?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:412
> +    CCRenderPassIdHashMap::const_iterator itToRemove = frame.renderPassesById.find(renderPassId);
> +    ASSERT(itToRemove != frame.renderPassesById.end());
> +    return itToRemove->second.get();

bikeshed: itToRemove is not really the right name, since you're finding not removing?

[Dana Jansens]

Comment on attachment 152054 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:321
>> +        passesInFrame.set(renderPassesInDrawOrder[i]->id(), renderPassesInDrawOrder[i]);
> 
> Is this passesInFrame hash redundant? Should decideRenderPassAllocationsForFrame just take FrameData?

Hm! LRC should not take a FrameData. But we could pass in the HashMap that we have now.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:412
>> +    return itToRemove->second.get();
> 
> bikeshed: itToRemove is not really the right name, since you're finding not removing?

Ohyarite, thanks. #refactorfail

[Dana Jansens]

Created attachment 152081 [details]
Patch

[Dana Jansens]

(In reply to comment #14)
> (From update of attachment 152054 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152054&action=review
> 
> > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:321
> >      HashMap<int, const CCRenderPass*> passesInFrame;
> >      for (size_t i = 0; i < renderPassesInDrawOrder.size(); ++i)
> > -        passesInFrame.set(renderPassesInDrawOrder[i]->id(), renderPassesInDrawOrder[i].get());
> > +        passesInFrame.set(renderPassesInDrawOrder[i]->id(), renderPassesInDrawOrder[i]);
> 
> Is this passesInFrame hash redundant? Should decideRenderPassAllocationsForFrame just take FrameData?

It is actually not redundant. This map includes only the passes that are in the draw order. It does not include empty render passes (passes that dont fit in memory). The hashmap in FrameData has all RenderPasses in it as its the owner of them all.

[Adrienne Walker]

Comment on attachment 152081 [details]
Patch

R=me.

[WebKit Review Bot]

Comment on attachment 152081 [details]
Patch

Clearing flags on attachment: 152081

Committed r122525: <http://trac.webkit.org/changeset/122525>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.