[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


Adrienne Walker <enne at google.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |enne at google.com




--- Comment #32 from Adrienne Walker <enne at google.com>  2012-06-14 12:17:04 PST ---
(In reply to comment #26)
> (From update of attachment 147153 [details])
> 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.

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.

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

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.

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

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