[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 #22 from Shawn Singh <shawnsingh at chromium.org>  2012-05-30 14:28:41 PST ---
(From update of attachment 144908)
Looks really awesome Zeev!  I did have a lot of comments...  Hopefully they don't contradict Dana =)


View in context: https://bugs.webkit.org/attachment.cgi?id=144908&action=review

> Source/WebCore/ChangeLog:13
> +        Covered by existing layout tests. Introduced more unit tests to

Its worth pointing out that damage tracking and scissoring aren't actually covered by layout tests.  DumpRenderTree reads back from the backbuffer, and therefore partial swap (and hence the scissoring) doesn't apply in those cases.  This can possibly be fixed in the long run for "repaint" layout tests, but in general it is just the nature of layout tests that prevents it.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:74
> +    FloatRect clipAndDamage = CCMathUtil::projectClippedRect(inverseScreenSpaceTransform, rootDamageRect);

Since we removed that helper function, I think we should add a comment here now that makes it clear what we're doing with this transform (i.e. that we're computing the damage in local surface space, where the rootDamageRect is in root surface space.)

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:79
> +    if (layer->usesLayerClipping())
> +        clipAndDamage.intersect(layer->clipRect());
> +    else
> +        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.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:89
> +    RenderSurfaceType* targetSurface = parentLayer->targetRenderSurface();
> +    RenderSurfaceType* surface = layer->renderSurface();

can we add an ASSERT(surface) and ASSERT(targetSurface) just after this?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:91
> +    if (clipRect.isEmpty())

We should definitely add a comment here saying that checking isEmpty() is how (for now) we encoded whether the clipRect is supposed to be used or not, for surfaces, unlike layers which have usesLayerClipping().

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:97
> +    FloatRect clipAndDamage = CCMathUtil::projectClippedRect(inverseScreenSpaceTransform, rootDamageRect);
> +
> +    clipAndDamage.intersect(clipRect);

If you were OK with creating the more readable version in the previous function, then let's do it here, too.

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:277
> +    FloatRect rootDamageRect = calculateRenderSurfaceLayerList(renderSurfaceLayerList);
>  
>      TRACE_EVENT1("webkit", "CCLayerTreeHostImpl::calculateRenderPasses", "renderSurfaceLayerList.size()", static_cast<long long unsigned>(renderSurfaceLayerList.size()));
>  
> -    if (layerRendererCapabilities().usingPartialSwap || settings().showSurfaceDamageRects)
> -        trackDamageForAllSurfaces(m_rootLayerImpl.get(), renderSurfaceLayerList);
> -    m_rootDamageRect = m_rootLayerImpl->renderSurface()->damageTracker()->currentDamageRect();
> +    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.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:425
> +        TransformationMatrix inverseScreenSpaceTransform = renderPass->targetSurface()->screenSpaceTransform().inverse();
> +        FloatRect surfaceSwapRect = CCMathUtil::projectClippedRect(inverseScreenSpaceTransform, m_rootDamageRect);
> +        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?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:178
> +    // Exposed for testing. Returns root damage rect or full viewport if not using partial swap.
> +    FloatRect calculateRenderSurfaceLayerList(CCLayerList&);

If you agree with my earlier comment about removing this return value, then don't forget to fix this comment, too.

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