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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Jun 12 08:39:58 PDT 2012


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





--- Comment #8 from Dana Jansens <danakj at chromium.org>  2012-06-12 08:39:58 PST ---
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. :)

> > > Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:317
> > > +    // affect the pixels on its target surface, but would require redrawing
> > 
> > "on its target surface" -> "on its own surface" ?
> 
> Sounds good, also need to add something about layers that don't have surfaces.

Add something to the comment? That sounds good.

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

> > 
> > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:422
> > > +void CCLayerTreeHostImpl::removeRedundantPasses(CCRenderPassList& passes)
> > 
> > I think this could have a better name. "Redundant" doesn't explain what this function is doing really - why are they redundant?
> 
> removePassesWithCachedTextures?

Yes :)

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