[webkit-reviews] review granted: [Bug 49206] Add multisampling support to DrawingBuffer. : [Attachment 73552] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has granted Chris Marrin
<cmarrin at apple.com>'s request for review:
Bug 49206: Add multisampling support to DrawingBuffer.
https://bugs.webkit.org/show_bug.cgi?id=49206

Attachment 73552: Patch
https://bugs.webkit.org/attachment.cgi?id=73552&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
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.