[Webkit-unassigned] [Bug 88482] [Chromium] Compositor should avoid drawing quads when cached textures are available and contents unchanged

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Jun 14 13:52:53 PDT 2012


https://bugs.webkit.org/show_bug.cgi?id=88482





--- Comment #34 from zlieber at chromium.org  2012-06-14 13:52:53 PST ---
(From update of attachment 147153)
View in context: https://bugs.webkit.org/attachment.cgi?id=147153&action=review

>>>> Source/WebCore/platform/graphics/chromium/cc/CCRenderPassDrawQuad.h:47
>>>> +    bool contentsChanged() const { return m_contentsChanged; }
>>> 
>>> I think this is the wrong direction to be going in for CCRenderPassDrawQuad; we should have fewer pointers in quads and not more.  In the future, I'd like to change the CCRenderPass pointer to just be an integer index into the render pass list for the frame.
>>> 
>>> I don't think either of these members should be added.  If you need the target render surface, get it from the pass.  If you need to know if contents have changed, you can get that from either the pass or the surface.  The quad doesn't need to know if its contents have changed or not in order to draw, that's really a concern of a higher level (CCLayerTreeHostImpl as the owner of the list of the passes).
>> 
>> I put in a pointer to renderSurface precisely because of this reason - so I don't have to assume that m_renderPass points to a valid render pass object. Once the ubercomp change is done, the LRC will have another way of getting the render surface and the m_targetRenderSurface can go. If we try to do some hack now, we will have to undo much more stuff with the ubercomp change, IMO.
>> 
>> For the contentsChanged, I would claim that this field is part of the information required in order to efficiently draw a render pass list, therefore it belongs in the quad. This is part of the information based on which the render pass list will be reduced. However, before transmitting the render pass to the other side, we do not have enough knowledge to use this information (i.e. whether or not cached textures exist), so we use this flag to preserve what we have and let the compositor make use of it.
> 
> When does m_renderPass not point to a valid render pass?
> 
> I don't understand what the hack is here; I'm just asking that the iteration to remove passes in CCLayerTreeHostImpl use the surface pointer and damage tracker that it already has access to, rather than pushing more pointers into quads.

> When does m_renderPass not point to a valid render pass?

In two cases. First, when the render pass it points to is removed from the vector and deleted. The pointer becomes dangling, as the vector has OwnPtr's. And second, when we are going to convert renderPass pointer to render pass ID. My understanding was that this is the direction we're going.

> I don't understand what the hack is here

A couple of possibilities, I'd consider them as hacks. First is to cache the removed render passes in a separate vector so that they don't go out of scope. That caching will have to be removed when render pass pointer becomes and ID, that's why I said it's more changes. The second possibility is to introduce a flag in render pass that will cause the draw to be skipped. I'd consider this problematic because we shouldn't have issues with removing render passes - they never survive a single frame anyways. Given that the current direction is for the render pass pointer inside the quad to become and ID, I thought this solution was the closest to it.

It is a matter of opinion however so if you want any of these possibilities or have a different one in mind this can be done. Just wanted to point out that it's not as simple as replacing quad->surface() with quad->pass()->surface().

> Sorry, but I disagree.  That field is part of the information used to remove some other render pass entirely, but not to actually draw the quad representing that pass.
>
> The damage is also a piece of information that we could use to set a partial scissor when beginning to draw a render pass, so it seems like it's most suited to live somewhere more like the pass or the surface, which it essentially does right now in the form of querying the damage tracker.  In an ubercomp world, I think we would pass the current damage rect with the pass and not with the quad.

OK, I'll query the surface.

>>>> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.h:183
>>>> +    // to CCRenderPassDrawQuad without creating new class dependencies.
>>> 
>>> Can you explain what the eventual goal is here, possibly with a diagram of who talks to who?
>>> 
>>> If you remove m_contentsChanged from the quad, then it seems to me that there's no extra dependency, and maybe no need for this extra member?
>> 
>> I don't think we can remove it from the quad (see separate comment), as we won't transmit the entire layer structure in the ubercomp. We need to transmit contentsChanged flag *somewhere* so the render passes can be deleted properly.
>> 
>> For the relationship change, I was just referring to the ubercomp and the change Dana is doing, making CCRenderPassDrawQuad more independent. I'm not really sure at this point whether the flag will actually go away, but I suspect it may.
>> 
>> The reason it is currently there, is that damage tracker is the one who knows whether or not this flag should be set, by having an empty currentDamageRect. At that time, CCRenderPassDrawQuad does not exist yet, so cannot set from damage tracker. At the same time, we cannot query CCDamageTracker from CCRenderSurface because it will create a dependency (it actually exists today, but it's circular and we should get rid of it). And we obviously cannot and shouldn't talk to CCDamageTracker from CCRenderPassDrawQuad.
> 
> I don't understand why you can't query CCDamageTracker from CCRenderSurface.  CCRenderSurface *owns* the damage tracker.  Offhand, I think the circular reference would be better broken by pushing the damage tracker up to CCRenderPass rather than pushing the results down.  That's problematic because CCRenderSurface is the persistent object from frame to frame, so it'd take more doing to make that work.
> 
> It seems like you're trying to do two things in this patch: preserve render passes and eliminate circular dependencies.  The former seems like it's more important and the latter seems more contentious.  Can we punt on the second for now?

Right now the situation is actually not so bad. Render surface instantiates damage tracker, but that can be fixed pretty easily. I checked in the source, this is their only interaction right now.

So I was looking at adding communication between surface and tracker as degradation in terms of dependencies; what you suggesting is pretty easily done and if you're not convinced by the above I can do this.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.



More information about the webkit-unassigned mailing list