[webkit-reviews] review denied: [Bug 91800] [chromium] Do partial-swap scissoring on quads during draw instead of on layers : [Attachment 155081] Patch

[bugzilla-daemon at webkit.org]

Adrienne Walker <enne at google.com> has denied Dana Jansens
<danakj at chromium.org>'s request for review:
Bug 91800: [chromium] Do partial-swap scissoring on quads during draw instead
of on layers
https://bugs.webkit.org/show_bug.cgi?id=91800

Attachment 155081: Patch
https://bugs.webkit.org/attachment.cgi?id=155081&action=review

------- Additional Comments from Adrienne Walker <enne at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=155081&action=review


> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:290
> +
> +    m_swapBufferRect.intersect(IntRect(IntPoint(), viewportSize()));

This seems like it should be handled at the damage tracker level and not here.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:434
> +    WebTransformationMatrix inverseTransformToRoot =
renderPass->transformToRootTarget().inverse();
> +    FloatRect framebufferDamageRect =
CCMathUtil::projectClippedRect(inverseTransformToRoot, frame.rootDamageRect);

This probably outside the scope of this patch, but it seems like we should be
really be considering the damage inside the surface here.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:444
> +    if (texture)
> +	  
texture->setIsComplete(!renderPass->hasOcclusionFromOutsideTargetSurface());

Should this be true if any quads get rejected by the damage scissor that would
have otherwise drawn into the surface?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:283
> +    IntRect m_swapBufferRect;

I feel like this doesn't need to be a member variable, and you should just pass
frame.rootDamageRect to swapBuffers.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:545
> +	   // Once a RenderPass has been drawn, its damage should be cleared in

> +	   // case the RenderPass will be reused next frame.
> +	   frame.renderPasses[i]->setFramebufferDamageRect(FloatRect());

Shouldn't you only clear the damage inside the scissor that was used when
drawing into that surface?

Or, am I thinking about this wrong? A RenderPass is immutable when passed from
embedded compositor to host compositor, so from the pass's perspective the
damage is entirely cleared, but if not all of the damaged regions were drawn,
then maybe the *surface* owned by the host compositor is no longer complete and
has some invalidations?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.h:78
> +    FloatRect framebufferDamageRect() const { return
m_framebufferDamageRect; }
> +    void setFramebufferDamageRect(FloatRect rect) { m_framebufferDamageRect
= rect; }

What space is this in? Maybe this should just be "damageRect"?