WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
62339
[chromium] Scissor rect not set for clipping layers set offscreen
https://bugs.webkit.org/show_bug.cgi?id=62339
Summary
[chromium] Scissor rect not set for clipping layers set offscreen
James Robinson
Reported
2011-06-08 16:35:02 PDT
[chromium] Scissor rect not set for clipping layers set offscreen
Attachments
Patch
(10.88 KB, patch)
2011-06-08 17:45 PDT
,
James Robinson
no flags
Details
Formatted Diff
Diff
Patch
(10.50 KB, patch)
2011-06-08 19:27 PDT
,
James Robinson
no flags
Details
Formatted Diff
Diff
Patch
(11.64 KB, patch)
2011-06-09 12:25 PDT
,
James Robinson
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
James Robinson
Comment 1
2011-06-08 17:45:01 PDT
Created
attachment 96516
[details]
Patch
Kenneth Russell
Comment 2
2011-06-08 18:05:06 PDT
Comment on
attachment 96516
[details]
Patch If Vangelis can unofficially review this I'll gladly r+ it.
Vangelis Kokkevis
Comment 3
2011-06-08 18:48:01 PDT
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
Comment 4
2011-06-08 19:13:07 PDT
(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
Comment 5
2011-06-08 19:27:10 PDT
Created
attachment 96529
[details]
Patch
James Robinson
Comment 6
2011-06-08 19:29:04 PDT
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 7
2011-06-08 22:12:43 PDT
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
Comment 8
2011-06-09 12:25:14 PDT
Created
attachment 96616
[details]
Patch
James Robinson
Comment 9
2011-06-09 12:26:47 PDT
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 10
2011-06-09 14:26:32 PDT
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
Comment 11
2011-06-09 14:28:11 PDT
(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 12
2011-06-09 14:31:05 PDT
Comment on
attachment 96616
[details]
Patch r+ based on Vangelis's review.
WebKit Review Bot
Comment 13
2011-06-09 15:54:06 PDT
Comment on
attachment 96616
[details]
Patch Clearing flags on attachment: 96616 Committed
r88496
: <
http://trac.webkit.org/changeset/88496
>
WebKit Review Bot
Comment 14
2011-06-09 15:54:10 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug