Bug 64879

Summary: [Qt] Implement WebGL antialiasing (part 3)
Product: WebKit Reporter: Andrew Wason <rectalogic>
Component: WebKit QtAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: noam, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 64878    
Bug Blocks: 57261    
Attachments:
Description Flags
implement antialiasing
none
adopt Extensions3DOpenGL
noam: review-
implement antialiasing
noam: review-
adopt Extensions3DOpenGL
none
implement antialiasing
noam: review-, noam: commit-queue-
implement antialiasing
noam: review-, noam: commit-queue-
implement antialiasing none

Description Andrew Wason 2011-07-20 10:13:06 PDT
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
Comment 1 Andrew Wason 2011-08-05 08:18:14 PDT
Created attachment 103072 [details]
implement antialiasing
Comment 2 Noam Rosenthal 2011-08-05 10:42:49 PDT
Can this be done in two patches? (one for extensions, one for antialiasing)
Comment 3 Andrew Wason 2011-08-05 10:49:53 PDT
(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?
Comment 4 Noam Rosenthal 2011-08-05 13:07:43 PDT
(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.
Comment 5 Andrew Wason 2011-08-05 14:01:10 PDT
Created attachment 103106 [details]
adopt Extensions3DOpenGL
Comment 6 Andrew Wason 2011-08-05 14:01:34 PDT
Created attachment 103107 [details]
implement antialiasing
Comment 7 Noam Rosenthal 2011-08-05 14:05:28 PDT
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 8 Noam Rosenthal 2011-08-05 14:16:46 PDT
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)
Comment 9 Andrew Wason 2011-08-08 07:23:54 PDT
Created attachment 103240 [details]
adopt Extensions3DOpenGL
Comment 10 Andrew Wason 2011-08-08 07:24:21 PDT
Created attachment 103241 [details]
implement antialiasing
Comment 11 Noam Rosenthal 2011-08-08 07:42:31 PDT
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.
Comment 12 Andrew Wason 2011-08-08 08:24:37 PDT
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 13 WebKit Review Bot 2011-08-08 08:29:59 PDT
Comment on attachment 103240 [details]
adopt Extensions3DOpenGL

Clearing flags on attachment: 103240

Committed r92596: <http://trac.webkit.org/changeset/92596>
Comment 14 Noam Rosenthal 2011-08-08 08:30:04 PDT
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 :)
Comment 15 Andrew Wason 2011-08-08 08:36:28 PDT
Created attachment 103253 [details]
implement antialiasing
Comment 16 WebKit Review Bot 2011-08-08 11:42:33 PDT
Comment on attachment 103253 [details]
implement antialiasing

Clearing flags on attachment: 103253

Committed r92615: <http://trac.webkit.org/changeset/92615>
Comment 17 WebKit Review Bot 2011-08-08 11:42:38 PDT
All reviewed patches have been landed.  Closing bug.