[Webkit-unassigned] [Bug 88482] [Chromium] RenderSurface for opacity with descendentDrawsContent is slow when large

[bugzilla-daemon at webkit.org]

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





--- Comment #14 from zlieber at chromium.org  2012-06-12 12:35:46 PST ---
(In reply to comment #8)
> Can you describe the state of the current patch you have up? Does it already handle the surface owned by non-drawsContent layer case, and work correctly for the iconify animation etc? ie. is this done from your point of view and ready for a more thorough review yet, or where is it at right now? Thanks!
> 
> (In reply to comment #7)
> > Thanks for comments; some responses below.
> > 
> > > 
> > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:278
> > > > +bool CCLayerImpl::layerTargetSurfaceDirty() const
> > > 
> > > Since this is only called from damage tracker, can it be done there? Maybe file-static method if you like? Can the name be better so we don't need the verbose comment below?
> > 
> > It sure could, but this answers a pretty generic question using only class's own data, so seemed natural to be on the layer. The question is how generic that question is - maybe a different name will make this better? layerTargetSurfaceRequiresRedrawing/ContentsChanged/PixelsChanged?
> 
> I think the pattern we've been using for layers is that they are dumb as possible, and we have other classes that act on them instead. So I think if we can just have simple getter/setter on the class, and do more clever things elsewhere (closer to where the computed result it used) that would be desirable. Others should correct me if I'm wrong here. :)

If this is the policy I will of course move this. Though I do have to wonder why we have such a constraint. To me, object = state + behaviour, and to artificially limit it to state only needs some reason behind it. Also to properly implement this policy, layer should be a struct without any methods.

I also have to point out that right now we have some "sacred knowledge" around the layer, things like - "you shouldn't call setVisibleLayerRect manually, it will be set by other code". This is also an artifact of having layer-related logic outside the layer. If this method shouldn't be called, then the layer is the one to compute it and return it via a getter method, with setter being either private or none at all. However, since the relevant algorithm is outside we have to expose the setter and warn people verbally not to call it.

Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:231
> > > > +        renderSurface->setContentsChanged(!renderSurface->damageTracker()->currentDamageRect().isEmpty());
> > > 
> > > I'm not sure why we need this bool on the RenderSurface. It can already see the rect is empty as it looks up here.
> > 
> > Yeah I really did want to put this in, but couldn't see how the surface can look at this rect without talking to damage tracker directly. Is there a way? 
> > 
> > If not then this flag can go after we fix the quad creation and relationship between these classes and occlusion tracker, so the flag can be set on the quad externally.
> 
> I'm not sure I follow, can't we replace places that say "renderSurface->contentsChanged()" with "!renderSurface->damageTracker()->currentDamageRect().isEmpty()"? Or at worst, put a contentsChanged() method on CCRenderSurface that queries the damageTracker rect. I don't get why we need a new bool in the class.

Because both these options will require renderSurface to talk with damage tracker (talk with X == call methods on X). This is the circular dependency we are trying to get rid of - right now render surface only creates it, and we can fix it pretty easily.

I agree the bool should go away, but I see it done differently and in a new CL. We need to first remove the RS talking to quad culler directly, and then insert setting the bool on the draw quad outside the RS.

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