Bug 64879 - [Qt] Implement WebGL antialiasing (part 3)
: [Qt] Implement WebGL antialiasing (part 3)
Status: RESOLVED FIXED
: WebKit
WebKit Qt
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 64878
: 57261
  Show dependency treegraph
 
Reported: 2011-07-20 10:13 PST by
Modified: 2011-08-08 11:42 PST (History)


Attachments
implement antialiasing (13.64 KB, patch)
2011-08-05 08:18 PST, Andrew Wason
no flags Review Patch | Details | Formatted Diff | Diff
adopt Extensions3DOpenGL (6.96 KB, patch)
2011-08-05 14:01 PST, Andrew Wason
noam: review-
Review Patch | Details | Formatted Diff | Diff
implement antialiasing (7.65 KB, patch)
2011-08-05 14:01 PST, Andrew Wason
noam: review-
Review Patch | Details | Formatted Diff | Diff
adopt Extensions3DOpenGL (7.05 KB, patch)
2011-08-08 07:23 PST, Andrew Wason
no flags Review Patch | Details | Formatted Diff | Diff
implement antialiasing (7.95 KB, patch)
2011-08-08 07:24 PST, Andrew Wason
noam: review-
noam: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
implement antialiasing (8.11 KB, patch)
2011-08-08 08:24 PST, Andrew Wason
noam: review-
noam: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
implement antialiasing (8.14 KB, patch)
2011-08-08 08:36 PST, Andrew Wason
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-07-20 10:13:06 PST
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 From 2011-08-05 08:18:14 PST -------
Created an attachment (id=103072) [details]
implement antialiasing
------- Comment #2 From 2011-08-05 10:42:49 PST -------
Can this be done in two patches? (one for extensions, one for antialiasing)
------- Comment #3 From 2011-08-05 10:49:53 PST -------
(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 From 2011-08-05 13:07:43 PST -------
(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 From 2011-08-05 14:01:10 PST -------
Created an attachment (id=103106) [details]
adopt Extensions3DOpenGL
------- Comment #6 From 2011-08-05 14:01:34 PST -------
Created an attachment (id=103107) [details]
implement antialiasing
------- Comment #7 From 2011-08-05 14:05:28 PST -------
(From update of attachment 103106 [details])
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 From 2011-08-05 14:16:46 PST -------
(From update of attachment 103107 [details])
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 From 2011-08-08 07:23:54 PST -------
Created an attachment (id=103240) [details]
adopt Extensions3DOpenGL
------- Comment #10 From 2011-08-08 07:24:21 PST -------
Created an attachment (id=103241) [details]
implement antialiasing
------- Comment #11 From 2011-08-08 07:42:31 PST -------
(From update of attachment 103241 [details])
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 From 2011-08-08 08:24:37 PST -------
Created an attachment (id=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 From 2011-08-08 08:29:59 PST -------
(From update of attachment 103240 [details])
Clearing flags on attachment: 103240

Committed r92596: <http://trac.webkit.org/changeset/92596>
------- Comment #14 From 2011-08-08 08:30:04 PST -------
(From update of attachment 103248 [details])
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 From 2011-08-08 08:36:28 PST -------
Created an attachment (id=103253) [details]
implement antialiasing
------- Comment #16 From 2011-08-08 11:42:33 PST -------
(From update of attachment 103253 [details])
Clearing flags on attachment: 103253

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