Bug 94050 – [chromium] set scissorRect per quad so that quads are correctly clipped

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Shawn Singh]

Currently, some quad types draw their entire contentBounds instead of only their visibleRect, and they rely on scissoring to correctly clip the layers.  This is proposed to be refactored in https://bugs.webkit.org/show_bug.cgi?id=94049, but in the meantime, for correctness we need to do per-quad scissoring so that all layer types are correctly clipped.   Before fixing this, after a recent refactor, scissoring is done only per-surface and results in incorrect unclipped layers for layers such as canvas.

[Shawn Singh]

The fix is quite straightforward, but i'm not sure if we have a good way to unit test this.  So I'll see if a layout test can cover the issue instead.

[Shawn Singh]

Created attachment 158586 [details]
Patch

passes all unit tests and layout tests on osx.  In this patch I opted for the always-scissor approach.  I felt like adding more logic to turn off scissoring was over-engineering the solution without enough evidence that it's worth the savings. If you want I can make a different solution that adds a bit more logic passed into drawQuad, so that the quad can decide for itself whether to turn off scissoring or not.  But, especially if we do intend to refactor quad types to draw only their clipped/visible bounds, then maybe this solution is better to keep things simple.

[Dana Jansens]

Comment on attachment 158586 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:-425
> -    else
> -        GLC(m_context, m_context->disable(GraphicsContext3D::SCISSOR_TEST));

This shouldn't go away. Else the last layer's scissor could apply to the clear here when it shouldn't.

[Shawn Singh]

(In reply to comment #3)
> (From update of attachment 158586 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158586&action=review
> 
> > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:-425
> > -    else
> > -        GLC(m_context, m_context->disable(GraphicsContext3D::SCISSOR_TEST));
> 
> This shouldn't go away. Else the last layer's scissor could apply to the clear here when it shouldn't.

Thanks - good catch =)

But, if you're ok with it, I think it's better to remove the if statement instead, to keep the always-scissor approach.

[Shawn Singh]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 158586 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=158586&action=review
> > 
> > > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:-425
> > > -    else
> > > -        GLC(m_context, m_context->disable(GraphicsContext3D::SCISSOR_TEST));
> > 
> > This shouldn't go away. Else the last layer's scissor could apply to the clear here when it shouldn't.
> 
> Thanks - good catch =)
> 
> But, if you're ok with it, I think it's better to remove the if statement instead, to keep the always-scissor approach.

to be clear, I meant, literally remove the if (...), and always set the scissor, even if it's the same as the surface's rect.

[Dana Jansens]

Comment on attachment 158586 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:-425
>>>> -        GLC(m_context, m_context->disable(GraphicsContext3D::SCISSOR_TEST));
>>> 
>>> This shouldn't go away. Else the last layer's scissor could apply to the clear here when it shouldn't.
>> 
>> Thanks - good catch =)
>> 
>> But, if you're ok with it, I think it's better to remove the if statement instead, to keep the always-scissor approach.
> 
> to be clear, I meant, literally remove the if (...), and always set the scissor, even if it's the same as the surface's rect.

Is that better? It differs from how the code was before I touched any of this, so I defer to enne on that.

[Adrienne Walker]

Comment on attachment 158586 [details]
Patch

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

>>>>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:-425
>>>>> -        GLC(m_context, m_context->disable(GraphicsContext3D::SCISSOR_TEST));
>>>> 
>>>> This shouldn't go away. Else the last layer's scissor could apply to the clear here when it shouldn't.
>>> 
>>> Thanks - good catch =)
>>> 
>>> But, if you're ok with it, I think it's better to remove the if statement instead, to keep the always-scissor approach.
>> 
>> to be clear, I meant, literally remove the if (...), and always set the scissor, even if it's the same as the surface's rect.
> 
> Is that better? It differs from how the code was before I touched any of this, so I defer to enne on that.

Why are you setting the scissor in two places?

[Dana Jansens]

Comment on attachment 158586 [details]
Patch

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

>>>>>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:-425
>>>>>> -        GLC(m_context, m_context->disable(GraphicsContext3D::SCISSOR_TEST));
>>>>> 
>>>>> This shouldn't go away. Else the last layer's scissor could apply to the clear here when it shouldn't.
>>>> 
>>>> Thanks - good catch =)
>>>> 
>>>> But, if you're ok with it, I think it's better to remove the if statement instead, to keep the always-scissor approach.
>>> 
>>> to be clear, I meant, literally remove the if (...), and always set the scissor, even if it's the same as the surface's rect.
>> 
>> Is that better? It differs from how the code was before I touched any of this, so I defer to enne on that.
> 
> Why are you setting the scissor in two places?

One's for the whole render surface for clear.

[Shawn Singh]

(In reply to comment #7)
> (From update of attachment 158586 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158586&action=review
> 
> >>>>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:-425
> >>>>> -        GLC(m_context, m_context->disable(GraphicsContext3D::SCISSOR_TEST));
> >>>> 
> >>>> This shouldn't go away. Else the last layer's scissor could apply to the clear here when it shouldn't.
> >>> 
> >>> Thanks - good catch =)
> >>> 
> >>> But, if you're ok with it, I think it's better to remove the if statement instead, to keep the always-scissor approach.
> >> 
> >> to be clear, I meant, literally remove the if (...), and always set the scissor, even if it's the same as the surface's rect.
> > 
> > Is that better? It differs from how the code was before I touched any of this, so I defer to enne on that.
> 
> Why are you setting the scissor in two places?

So that clearing the renderSurface gets correctly scissored, too.   I think we need that for when partial swap is enabled, we can't avoid scissoring there.

[Adrienne Walker]

Comment on attachment 158586 [details]
Patch

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

>>>>>>>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:-425
>>>>>>>> -        GLC(m_context, m_context->disable(GraphicsContext3D::SCISSOR_TEST));
>>>>>>> 
>>>>>>> This shouldn't go away. Else the last layer's scissor could apply to the clear here when it shouldn't.
>>>>>> 
>>>>>> Thanks - good catch =)
>>>>>> 
>>>>>> But, if you're ok with it, I think it's better to remove the if statement instead, to keep the always-scissor approach.
>>>>> 
>>>>> to be clear, I meant, literally remove the if (...), and always set the scissor, even if it's the same as the surface's rect.
>>>> 
>>>> Is that better? It differs from how the code was before I touched any of this, so I defer to enne on that.
>>> 
>>> Why are you setting the scissor in two places?
>> 
>> One's for the whole render surface for clear.
> 
> So that clearing the renderSurface gets correctly scissored, too.   I think we need that for when partial swap is enabled, we can't avoid scissoring there.

Ok, sure.

Re: original if statement.  I'd prefer to leave it as is.  Maybe unconditionally setting the scissor is also correct, but why change it?

[Shawn Singh]

(In reply to comment #10)
> (From update of attachment 158586 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158586&action=review
> 
> >>>>>>>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:-425
> >>>>>>>> -        GLC(m_context, m_context->disable(GraphicsContext3D::SCISSOR_TEST));
> >>>>>>> 
> >>>>>>> This shouldn't go away. Else the last layer's scissor could apply to the clear here when it shouldn't.
> >>>>>> 
> >>>>>> Thanks - good catch =)
> >>>>>> 
> >>>>>> But, if you're ok with it, I think it's better to remove the if statement instead, to keep the always-scissor approach.
> >>>>> 
> >>>>> to be clear, I meant, literally remove the if (...), and always set the scissor, even if it's the same as the surface's rect.
> >>>> 
> >>>> Is that better? It differs from how the code was before I touched any of this, so I defer to enne on that.
> >>> 
> >>> Why are you setting the scissor in two places?
> >> 
> >> One's for the whole render surface for clear.
> > 
> > So that clearing the renderSurface gets correctly scissored, too.   I think we need that for when partial swap is enabled, we can't avoid scissoring there.
> 
> Ok, sure.
> 
> Re: original if statement.  I'd prefer to leave it as is.  Maybe unconditionally setting the scissor is also correct, but why change it?

The logic is as follows:

1. if scissor is different than contentRect, set it accordingly
2. if scissor is not different, then disable it
3. clear the framebuffer, only in the scissored region
4. (new in this patch) enable scissor and set it to the correct rect so that quads are correctly clipped.

It feels a bit unclean to keep doing step 2 if we are always doing step 4.  I felt it's better to keep the code simple rather than keeping the patch simple just because the patch works...

[Shawn Singh]

Created attachment 158623 [details]
Addressed Dana's comment, still uses always-scissor approach

I had this patch just before your questions, so feel free to r- it if you still want that if-statement back =)

[Adrienne Walker]

Comment on attachment 158623 [details]
Addressed Dana's comment, still uses always-scissor approach

R=me.

[WebKit Review Bot]

Comment on attachment 158623 [details]
Addressed Dana's comment, still uses always-scissor approach

Attachment 158623 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13512262

New failing tests:
compositing/overflow/overflow-hidden-canvas-layer.html

[WebKit Review Bot]

Created attachment 158665 [details]
Archive of layout-test-results from gce-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Shawn Singh]

Created attachment 158748 [details]
Patch for landing

[Shawn Singh]

Committed r125758: <http://trac.webkit.org/changeset/125758>

[Adrienne Walker]

Committed r125824: <http://trac.webkit.org/changeset/125824>