Bug 64879

Created attachment 103072 [details] implement antialiasing

Can this be done in two patches? (one for extensions, one for antialiasing)

(In reply to comment #2) > Can this be done in two patches? (one for extensions, one for antialiasing) Not really because Extensions3DOpenGL::supports() is queried to see if multisample etc. is supported and antialiasing is disabled if not. Well, I guess it could be done as 2 patches but antialiasing won't actually work unless both are applied. Should I split it?

(In reply to comment #3) > (In reply to comment #2) > > Can this be done in two patches? (one for extensions, one for antialiasing) > > Not really because Extensions3DOpenGL::supports() is queried to see if multisample etc. is supported and antialiasing is disabled if not. > > Well, I guess it could be done as 2 patches but antialiasing won't actually work unless both are applied. Should I split it? Yes, please send in two patches. You can keep one bug report, and commit together - but for reviewing, the smaller the better.

Created attachment 103106 [details] adopt Extensions3DOpenGL

Created attachment 103107 [details] implement antialiasing

Comment on attachment 103106 [details] adopt Extensions3DOpenGL View in context: https://bugs.webkit.org/attachment.cgi?id=103106&action=review Code is good, Changelog needs to be a lot more descriptive. In particular the differences between how we handle ES and desktop. Don't be afraid to repeat yourself. > Source/WebCore/ChangeLog:10 > + Adopt Extensions3DOpenGL for Qt antialiasing support. Insufficient. We're doing things differently for ES and desktop - please describe it in the changelog.

Comment on attachment 103107 [details] implement antialiasing View in context: https://bugs.webkit.org/attachment.cgi?id=103107&action=review > Source/WebCore/ChangeLog:11 > + Implement WebGL antialiasing for Qt. > + Requires adoption of Extensions3DOpenGL to be functional. Please explain concisely what this would do, how we're using existing code for antialiasing, and what your new patch does. > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:79 > + void multisampleResolve() const; Maybe a more descriptive name? something like blitMultisampleFboToMainFbo? (you can choose a better name). "Resolve" sounds a bit too close to resolving GL function names, even though it's a technically accurate term. > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:154 > + if (currentContext) { > + if (currentContext != widgetContext) > + const_cast<QGLContext*>(currentContext)->makeCurrent(); > + } else > + m_glWidget->doneCurrent(); > + } > + Please add a comment (e.g. // switch to the FBO's context for resolving multisamples)

Created attachment 103240 [details] adopt Extensions3DOpenGL

Created attachment 103241 [details] implement antialiasing

Comment on attachment 103241 [details] implement antialiasing View in context: https://bugs.webkit.org/attachment.cgi?id=103241&action=review > Source/WebCore/ChangeLog:8 > + Existing WebGL layout tests. Do you mean "Existing WebGL layout tests cover this"? > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:155 > + if (m_context->m_attrs.antialias) { > + // Switch to FBOs context for resolving multisamples, then restore. > + const QGLContext* currentContext = QGLContext::currentContext(); > + const QGLContext* widgetContext = m_glWidget->context(); > + if (currentContext != widgetContext) > + m_glWidget->makeCurrent(); > + blitMultisampleFramebuffer(); > + if (currentContext) { > + if (currentContext != widgetContext) > + const_cast<QGLContext*>(currentContext)->makeCurrent(); > + } else > + m_glWidget->doneCurrent(); > + } > + I don't like this inside paintToTextureMapper. Maybe put this in a new function like blitMultisampleFramebufferAndRestoreContext? If we do that you don't actually need the comment and the code describes itself. > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:389 > if (m_internal->m_glWidget->isValid()) { > glDeleteTextures(1, &m_texture); > - glDeleteRenderbuffers(1, &m_depthStencilBuffer); > + if (m_attrs.antialias) { > + glDeleteRenderbuffers(1, &m_multisampleColorBuffer); > + if (m_attrs.stencil || m_attrs.depth) > + glDeleteRenderbuffers(1, &m_multisampleDepthStencilBuffer); > + glDeleteFramebuffers(1, &m_multisampleFBO); > + } else if (m_attrs.stencil || m_attrs.depth) > + glDeleteRenderbuffers(1, &m_depthStencilBuffer); > glDeleteFramebuffers(1, &m_fbo); > } > } This is becoming rather unreadable... can we refactor to use early returns? Looks to me that with early returns this can shrink to a single condition level from 3.

Created attachment 103248 [details] implement antialiasing > This is becoming rather unreadable... can we refactor to use early returns? > Looks to me that with early returns this can shrink to a single condition level from 3. I refactored, but I kept the 2nd condition level for deleting m_multisampleDepthStencilBuffer, it looked more confusing using an early return for that.

Comment on attachment 103240 [details] adopt Extensions3DOpenGL Clearing flags on attachment: 103240 Committed r92596: <http://trac.webkit.org/changeset/92596>

Comment on attachment 103248 [details] implement antialiasing View in context: https://bugs.webkit.org/attachment.cgi?id=103248&action=review Almost there. > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:143 > + if (m_context->m_attrs.antialias) This should be an early return inside blitMultisampleFramebufferAndRestoreContext > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:209 > + if (m_context->m_attrs.antialias) This should be an early return inside blitMultisampleFramebuffer > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:348 > + // Bind canvas FBO Period at end of sentence :)

Created attachment 103253 [details] implement antialiasing

Comment on attachment 103253 [details] implement antialiasing Clearing flags on attachment: 103253 Committed r92615: <http://trac.webkit.org/changeset/92615>

All reviewed patches have been landed. Closing bug.