[Webkit-unassigned] [Bug 87167] [chromium] Cleanup scissor rect computation/use with damage

Wed May 30 15:15:05 PDT 2012

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

--- Comment #25 from Shawn Singh <shawnsingh at chromium.org>  2012-05-30 15:15:05 PST ---
(In reply to comment #23)
> rootSurfaceDamageInLocalSurfaceSpace == surfaceSwapRect, right? Can we call it that instead? Then we're saying scissor = intersection(clip, swapRect) which is more precise yes?
> 

so based on my previous comment - I think they are not the same at all... surfaceSwap in my opinion doesn't have meaning, the swap concept doesn't exist on surfaces.  So I would say it is more precise to say intersection(clip, rootSurfaceDamageInLocalSurfaceSpace).

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:261
> > +            rootDamageRect = FloatRect(FloatPoint(0, 0), viewportSize());
> 
> This should use deviceViewportSize() like the similar computation below does.
> 
> >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:277
> >> +    m_rootLayerImpl->setScissorRect(enclosingIntRect(rootDamageRect));
> > 
> > Personally I don't like the idea that calculateRenderSurfaceLayerList() would have any return value.  It feels right as a purely imperative, procedural function that changes state of the class.  It looks like the reason we needed this return value is to call setScissorRect for the rootLayerImpl.  Do we need this, however?  wouldn't this be correctly set by calculateVisibleAndScissorRects() ?
> > 
> > At the very least, how about we put this inside of calculateRenderSurfaceLayerList() so that we don't need to return that local rootDamageRect?  It makes sense for that line to be as closely grouped to calculateVisibleAndScissorRects as possible, if its not actually be computed inside of it.
> 
> The return value is actually the rootSwapRect right now, not the rootDamageRect I guess. This return value could also be used by the CCOcclusionTracker instead of recomputing it again below.
> 
> I agree the rootLayer's scissor should be set in CCLayerTreeHostCommon::calculateVisibleAndScissorRects. Right now it's not being set on the main thread right? If it's inside there then it would be always set when the others are set.
> 

Is it possible the swap/scissor/damage definition clarification changes what you're saying here?  If you still think we need this return value, lets discuss offline and then update this bug with a summary of our discussion?

> >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:425
> >> +        m_layerRenderer->drawRenderPass(renderPass, surfaceSwapRect);
> > 
> > I'm unclear why we need to pass surfaceSwapRect here (and the name seems awkward, since surfaces don't do swapping?)... isnt the whole point of this patch that we already computed the scissor rects we want to use by this point?  So this would be redundant and unnecessary, because the scissors for all surfaces and layers (and hence, quads) are already known by this time, right?
> 
> (In debug) We clear everything in the surface that is going to be drawn. This area is its swap rect, not its scissor rect (which is in its target). Can also think of it as the union of all scissorRects of things that draw into it.
> 
> Swap rect is the partial swap rect transformed into the surface, or the part of the surface that will be included in the swap. I got that name idea from convo with you about distinguishing damage rect vs partial swap rect. Do you think a different name would be more clear?

Same for here - if needed lets discuss offline briefly?

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