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

[bugzilla-daemon at webkit.org]

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





--- Comment #26 from zlieber at chromium.org  2012-06-13 16:44:21 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.

>> 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.

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