Bug 62339 – [chromium] Scissor rect not set for clipping layers set offscreen

• Component: New Bugs
• Status: RESOLVED FIXED

[James Robinson]

[chromium] Scissor rect not set for clipping layers set offscreen

[James Robinson]

Created attachment 96516 [details]
Patch

[Kenneth Russell]

Comment on attachment 96516 [details]
Patch

If Vangelis can unofficially review this I'll gladly r+ it.

[Vangelis Kokkevis]

Comment on attachment 96516 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:980
> +    setScissorToRect(layer->usesLayerScissor() ? layer->scissorRect() : targetSurfaceRect, true);

I think scissorRect handling needs to be cleaned up as it's getting more and more complicated.
In theory, setScissorToRect(targetSurfaceRect) should have the same effect a disabling scissoring  (you cannot draw outside the bounds of your render surface anyway).
So, if layer->usesLayerScissor() then enable scissoring and apply the scissor rect, otherwise disable scissoring. 

A usesLayerScissor boolean can also be added to the render surface and we can do the same query and enable/disable.

[James Robinson]

(In reply to comment #3)
> (From update of attachment 96516 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96516&action=review
> 
> > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:980
> > +    setScissorToRect(layer->usesLayerScissor() ? layer->scissorRect() : targetSurfaceRect, true);
> 
> I think scissorRect handling needs to be cleaned up as it's getting more and more complicated.
> In theory, setScissorToRect(targetSurfaceRect) should have the same effect a disabling scissoring  (you cannot draw outside the bounds of your render surface anyway).
> So, if layer->usesLayerScissor() then enable scissoring and apply the scissor rect, otherwise disable scissoring. 
> 

You mean disable the scissor test completely?  I could do that.

> A usesLayerScissor boolean can also be added to the render surface and we can do the same query and enable/disable.

Any idea when I should set this?  It's non-obvious.

[James Robinson]

Created attachment 96529 [details]
Patch

[James Robinson]

How's this?  I'm using "has an empty m_scissorRect" on RenderSurfaceChromium as a proxy for "should scissor".  It seems to work, at least on the maps case and on the compositing tests we currently have.  I hope that enabling/disabling the scissor test isn't significantly different perf wise than constantly setting a new scissor rect.

[Vangelis Kokkevis]

Comment on attachment 96529 [details]
Patch

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

In addition, you need to:
1. Always call layer->setUsersLayerScissor(false) at the top of updatePropertiesAndRenderSurfaces to reset the flag
2. Remove line 697 "layer->setScissorRect(IntRect());"  as that was essentially telling the layer that it didn't have a scissor rect.

> Source/WebCore/platform/graphics/chromium/RenderSurfaceChromium.cpp:158
> +        GLC(layerRenderer()->context(), layerRenderer()->context()->disable(GraphicsContext3D::SCISSOR_TEST));

The RenderSurface's have the same issue as regular layers.  Without a flag, there's no way to tell if an empty scissor rect means don't render or don't scissor.  The render surface inherits the scissor rect of its parent layer so the logic would be:
 
if (!m_owningLayer->parent() || !m_owningLayer->parent()->usesScissor())
  disable scissor test
else
  scissorRect = m_owningLayer->parent()->scissorRect()

[James Robinson]

Created attachment 96616 [details]
Patch

[James Robinson]

Patch updated w/ feedback from comment #7.  This sets layerUsesScissor on every layer in updateProperties...() which is definitely a bit of a hack, but should work as it's explicitly set to true for layers that need it.  Passes all chromium-gpu layout tests (but so did the previous patch).

[Vangelis Kokkevis]

Comment on attachment 96616 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:661
> +    // FIXME: This seems like the wrong place to set this

Why do you think this is the wrong place?  Seems right to me.

[Vangelis Kokkevis]

(In reply to comment #9)
> Patch updated w/ feedback from comment #7.  This sets layerUsesScissor on every layer in updateProperties...() which is definitely a bit of a hack, but should work as it's explicitly set to true for layers that need it.  Passes all chromium-gpu layout tests (but so did the previous patch).

The last patch looks good to me.  Since layers can move around in the tree we need some way to reset the usesScissor bit every time we go through so resetting at the top looks like the right solution.

[Kenneth Russell]

Comment on attachment 96616 [details]
Patch

r+ based on Vangelis's review.

[WebKit Review Bot]

Comment on attachment 96616 [details]
Patch

Clearing flags on attachment: 96616

Committed r88496: <http://trac.webkit.org/changeset/88496>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.