Bug 77575 – [Texmap] Use glScissors for clipping in TextureMapperGL when possible

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Noam Rosenthal]

Right now we use stencil for all clip operations in TextureMapperGL. Stencils are far more costly, especially on mobile devices, so the use of scissors can be a nice performance boost.

[Noam Rosenthal]

Created attachment 125017 [details]
Patch

[Martin Robinson]

Comment on attachment 125017 [details]
Patch

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

This looks good, but does the patch need to enable the scissor test for non-Qt ports as well?

> Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:452
> +    glEnable(GL_SCISSOR_TEST);

Shouldn't this be enabled for non-Qt ports as well?

> Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:736
> +    GLint viewport[4];
> +    glGetIntegerv(GL_VIEWPORT, viewport);
> +    glScissor(rect.x(), viewport[3] - rect.maxY(), rect.width(), rect.height());

I assume this is for handling flipped-Y situations here? Nice!

[Noam Rosenthal]

(In reply to comment #2)
> (From update of attachment 125017 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=125017&action=review
> 
> This looks good, but does the patch need to enable the scissor test for non-Qt ports as well?
> 
> > Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:452
> > +    glEnable(GL_SCISSOR_TEST);
> 
> Shouldn't this be enabled for non-Qt ports as well?
Yes, I was not sure about whether other ports wanted to control it themselves, especially the part that disables the scissor test on exit. I don't mind putting this disable/enable code outside the #ifdef.

> 
> > Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:736
> > +    GLint viewport[4];
> > +    glGetIntegerv(GL_VIEWPORT, viewport);
> > +    glScissor(rect.x(), viewport[3] - rect.maxY(), rect.width(), rect.height());
> 
> I assume this is for handling flipped-Y situations here? Nice!

Well, we have to do this somewhere or nothing will appear on the screen :)

[Martin Robinson]

(In reply to comment #3)

> Yes, I was not sure about whether other ports wanted to control it themselves, especially the part that disables the scissor test on exit. I don't mind putting this disable/enable code outside the #ifdef.

I think it could be more general by using glIsEnabled beforehand and then restoring the previous value. I don't think this would properly handle cases where the context already had a scissored region before calling into the texture mapper, but I'm not sure if that's an interesting usecase.

[Noam Rosenthal]

Created attachment 125037 [details]
Patch

Added glIsEnabled in beginPainting, and made some of it cross-platform.

[Noam Rosenthal]

Created attachment 125049 [details]
Patch

[WebKit Review Bot]

Comment on attachment 125049 [details]
Patch

Clearing flags on attachment: 125049

Committed r106524: <http://trac.webkit.org/changeset/106524>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.