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

[bugzilla-daemon at webkit.org]

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





--- Comment #23 from Dana Jansens <danakj at chromium.org>  2012-05-30 14:41:07 PST ---
(From update of attachment 144908)
View in context: https://bugs.webkit.org/attachment.cgi?id=144908&action=review

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:79
>> +        clipAndDamage.intersect(renderSurface->contentRect());
> 
> I'm guessing you just copied the code the way I had written it in the past, but now I see a more readable way we could write this (ignoring my abbreviated variables):
> 
> FloatRect rootSurfaceDamageInLocalSurfaceSpace = projectClippedRect(inverse, rootDamageRect)
> FloatRect clipAndDamageRect;
> if (layer->usesLayerClipping())
>     clipAndDamageRect = intersection(layer->clipRect(), rootSurfaceDamageInLocalSurfaceSpace);
> else
>     clipAndDamageRect = intersection(renderSurface->contentRect(), rootSurfaceDamageInLocalSurfaceSpace);
> 
> The advantage of this is that we don't accidentally mislead anyone by temporarily assigning the transformed damage to the clipAndDamage.

rootSurfaceDamageInLocalSurfaceSpace == surfaceSwapRect, right? Can we call it that instead? Then we're saying scissor = intersection(clip, swapRect) which is more precise yes?

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:749
>> +        }
> 
> should we put an else { assert not reached } here?   I'm genuinely asking, not suggesting - maybe reviewers can say more definitively if its appropriate.

It would be reached, with it.representsTargetRenderSurface(). We just don't do anything in that case, we use the contributing surface case instead. This avoids trying to set a scissor on the root surface (it's not contributing) based on its non-existent parent.

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

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

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