Bug 49206 – Add multisampling support to DrawingBuffer.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Chris Marrin]

DrawingBuffer doesn't handle multisampling right now. This is needed to be able to do https://bugs.webkit.org/show_bug.cgi?id=47379.

[Chris Marrin]

Created attachment 73320 [details]
Patch

[Kenneth Russell]

Comment on attachment 73320 [details]
Patch

Could you please make the Qt and Chromium ports compile by adding these virtuals to their implementations? Qt's is in platform/graphics/qt and Chromium's is in platform/graphics/chromium and WebKit/chromium/src. They can be no-ops with FIXMEs; the Chromium port should explicitly not claim support for them initially.

[Early Warning System Bot]

Attachment 73320 [details] did not build on qt:
Build output: http://queues.webkit.org/results/5496034

[Eric Seidel (no email)]

Attachment 73320 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/5482039

[Build Bot]

Attachment 73320 [details] did not build on win:
Build output: http://queues.webkit.org/results/5447048

[Chris Marrin]

Created attachment 73427 [details]
Patch

[Eric Seidel (no email)]

Attachment 73427 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/5451064

[Chris Marrin]

Created attachment 73507 [details]
Patch

[WebKit Review Bot]

Attachment 73507 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/5603005

[Chris Marrin]

Created attachment 73552 [details]
Patch

[James Robinson]

Comment on attachment 73552 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=73552&action=review

Looks pretty good to me.  Left some comments, but nothing major.

To reduce risk, could you set antialias to 'false' in SharedGraphicsContext3D::create() for now?  That way we can land this and test it out a bit locally on various platforms/configs before changing the behavior of any DrawingBuffer clients.

> WebCore/platform/graphics/gpu/DrawingBuffer.cpp:114
> +    // resize multisample FBO

This (and the other comments) should be written as sentences (Leading caps, trailing period).

> WebCore/platform/graphics/gpu/DrawingBuffer.cpp:119
> +        int sampleCount = std::min(8, maxSampleCount);

I'm not sure why 8 is special here.  Could this be a constant with a more descriptive name?

Also, webkit style is to put a 'using namespace std' declaration at the top of the .cpp and just say min() here.

> WebCore/platform/graphics/gpu/DrawingBuffer.cpp:207
> +    if (multisample())
> +        m_context->bindFramebuffer(GraphicsContext3D::FRAMEBUFFER, m_multisampleFBO);
> +
> +    // Initialize renderbuffers to 0.
> +    unsigned char colorMask[] = {true, true, true, true}, depthMask = true, stencilMask = true;
> +    unsigned char isScissorEnabled = false;
> +    unsigned char isDitherEnabled = false;
> +    unsigned long clearMask = GraphicsContext3D::COLOR_BUFFER_BIT;
> +    m_context->getBooleanv(GraphicsContext3D::COLOR_WRITEMASK, colorMask);
> +    m_context->colorMask(true, true, true, true);
> +    if (attributes.depth) {
> +        m_context->getBooleanv(GraphicsContext3D::DEPTH_WRITEMASK, &depthMask);
> +        m_context->depthMask(true);
> +        clearMask |= GraphicsContext3D::DEPTH_BUFFER_BIT;
> +    }
> +    if (attributes.stencil) {
> +        m_context->getBooleanv(GraphicsContext3D::STENCIL_WRITEMASK, &stencilMask);
> +        m_context->stencilMask(true);
> +        clearMask |= GraphicsContext3D::STENCIL_BUFFER_BIT;
> +    }
> +    isScissorEnabled = m_context->isEnabled(GraphicsContext3D::SCISSOR_TEST);
> +    m_context->disable(GraphicsContext3D::SCISSOR_TEST);
> +    isDitherEnabled = m_context->isEnabled(GraphicsContext3D::DITHER);
> +    m_context->disable(GraphicsContext3D::DITHER);
> +
> +    m_context->clear(clearMask);
> +
> +    m_context->colorMask(colorMask[0], colorMask[1], colorMask[2], colorMask[3]);
> +    if (attributes.depth)
> +        m_context->depthMask(depthMask);
> +    if (attributes.stencil)
> +        m_context->stencilMask(stencilMask);
> +    if (isScissorEnabled)
> +        m_context->enable(GraphicsContext3D::SCISSOR_TEST);
> +    else
> +        m_context->disable(GraphicsContext3D::SCISSOR_TEST);
> +    if (isDitherEnabled)
> +        m_context->enable(GraphicsContext3D::DITHER);
> +    else
> +        m_context->disable(GraphicsContext3D::DITHER);
> +
> +    m_context->flush();

I think this would be useful to have this clearing logic as a separate utility function, if just to shorten up the reset() function a bit.

[Eric Seidel (no email)]

Attachment 73507 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/5529076

[WebKit Review Bot]

http://trac.webkit.org/changeset/71827 might have broken Leopard Intel Release (Tests)
The following tests are not passing:
media/video-played-collapse.html
media/video-played-ranges-1.html
media/video-played-reset.html

[James Robinson]

Sadly looks like this did break a bunch of canvas tests in chromium.  I'll debug and update the bug when I figure out why.

[Mihai Parparita]

For the record, the rollout change was http://trac.webkit.org/changeset/71839

[James Robinson]

Ah, as it turns out the DrawingBufferChromium changes weren't quite complete.  In particular, the newly added fields on DrawingBuffer weren't initialized in the DrawingBufferChromium.cpp version of the constructor and the DrawingBufferChromium.cpp functions were not using m_colorBuffer when they should have been.  I should have caught both of these issues in review - my bad.

I'll update this bug with a patch that passes all tests on chromium.

[Chris Marrin]

Created attachment 73662 [details]
Patch

[James Robinson]

Comment on attachment 73662 [details]
Patch

R=me

Confirmed that this passes all gpu tests in fast/canvas and canvas/philip on chromium linux.

[Chris Marrin]

Committed r71853: <http://trac.webkit.org/changeset/71853>