Bug 64879 – [Qt] Implement WebGL antialiasing (part 3)

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Andrew Wason]

This is part 3 of a 3 part migration for bug 57261 to migrate to GraphicsContext3DOpenGL.cpp for Qt.

Part 3 implements antialiasing leveraging the existing partial antialiasing implementation
in GraphicsContext3DOpenGL.cpp and also adopting Extensions3DOpenGL.cpp

[Andrew Wason]

Created attachment 103072 [details]
implement antialiasing

[Noam Rosenthal]

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

[Andrew Wason]

(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?

[Noam Rosenthal]

(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.

[Andrew Wason]

Created attachment 103106 [details]
adopt Extensions3DOpenGL

[Andrew Wason]

Created attachment 103107 [details]
implement antialiasing

[Noam Rosenthal]

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.

[Noam Rosenthal]

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)

[Andrew Wason]

Created attachment 103240 [details]
adopt Extensions3DOpenGL

[Andrew Wason]

Created attachment 103241 [details]
implement antialiasing

[Noam Rosenthal]

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.

[Andrew Wason]

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.

[WebKit Review Bot]

Comment on attachment 103240 [details]
adopt Extensions3DOpenGL

Clearing flags on attachment: 103240

Committed r92596: <http://trac.webkit.org/changeset/92596>

[Noam Rosenthal]

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 :)

[Andrew Wason]

Created attachment 103253 [details]
implement antialiasing

[WebKit Review Bot]

Comment on attachment 103253 [details]
implement antialiasing

Clearing flags on attachment: 103253

Committed r92615: <http://trac.webkit.org/changeset/92615>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.