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.
Created attachment 125017 [details] Patch
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!
(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 :)
(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.
Created attachment 125037 [details] Patch Added glIsEnabled in beginPainting, and made some of it cross-platform.
Created attachment 125049 [details] Patch
Comment on attachment 125049 [details] Patch Clearing flags on attachment: 125049 Committed r106524: <http://trac.webkit.org/changeset/106524>
All reviewed patches have been landed. Closing bug.