Bug 76248 – Initial implementation of GraphicsContext3DOpenGLES.cpp

• Component: WebGL
• Status: RESOLVED FIXED

[ChangSeok Oh]

This bug is for implementing GraphicsContext3DOpenGLES.cpp.

[ChangSeok Oh]

Created attachment 122537 [details]
Patch

[Martin Robinson]

Comment on attachment 122537 [details]
Patch

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

I think this needs a bit of clean up, but it's looking pretty good so far!

> Source/WebCore/ChangeLog:13
> +        No new tests required.

Might want to explain why there are no new tests required.

> Source/WebCore/ChangeLog:27
> +        * platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:
> +        (WebCore::GraphicsContext3D::readRenderingResults):
> +        (WebCore::GraphicsContext3D::reshape):
> +        (WebCore::GraphicsContext3D::prepareTexture):
> +        (WebCore::GraphicsContext3D::bindFramebuffer):
> +        (WebCore::GraphicsContext3D::copyTexImage2D):
> +        (WebCore::GraphicsContext3D::copyTexSubImage2D):
> +        (WebCore::GraphicsContext3D::getActiveUniform):
> +        (WebCore::GraphicsContext3D::readPixels):
> +        (WebCore::GraphicsContext3D::renderbufferStorage):
> +        (WebCore::GraphicsContext3D::getIntegerv):
> +        (WebCore::GraphicsContext3D::texImage2D):

Here it might be good to briefly explain how each method differs from it's desktop GL counterpart.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:103
> +    // resize multisample FBO

Nit. Comments should begin with a capital letter and end with a period.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:107
> +    // resize regular FBO

Ditto.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:225
> +    // FIXME : We don't care about multisampleFBO currently.

Extra space after FIXME. Might want to open a bug about this now.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:240
> +    if (m_attrs.antialias && m_boundFBO == m_multisampleFBO)
> +        notImplemented();
> +    ::glCopyTexImage2D(target, level, internalformat, x, y, width, height, border);
> +    if (m_attrs.antialias && m_boundFBO == m_multisampleFBO)
> +        notImplemented();

Why do you do this check twice? Perhaps you should return early if multisampling is needed?

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:250
> +    if (m_attrs.antialias && m_boundFBO == m_multisampleFBO)
> +        notImplemented();
> +    ::glCopyTexSubImage2D(target, level, xoffset, yoffset, x, y, width, height);
> +    if (m_attrs.antialias && m_boundFBO == m_multisampleFBO)
> +        notImplemented();

Ditto.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:281
> +    // FIXME : "[0]" is added at end of name in WebGLRenderingContext::getActiveUniform()
> +    // when isGLES2Compliant is only false. I haven't found the reason.
> +    // So I add "[0]" here to avoid faling webgl conformance test(get-active-test.html),
> +    // but if it makes something wrong later, remove following two lines.
> +    if (isGLES2Compliant() && info.size > 1 && !info.name.endsWith("[0]"))
> +        info.name.append("[0]");

Extra space after the FIXME. Might want to investigate this a bit now to avoid code churn.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:289
> +    // FIXME: remove the two glFlush calls when the driver bug is fixed, i.e.,
> +    // all previous rendering calls should be done before reading pixels.

Certainly the driver bug wouldn't affect both OpenGLES drivers and for whatever driver this work-around was added for. Can you just remove it?

[ChangSeok Oh]

Created attachment 122730 [details]
Patch

[ChangSeok Oh]

Comment on attachment 122537 [details]
Patch

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

Thank you for review. :)

>> Source/WebCore/ChangeLog:13
>> +        No new tests required.
> 
> Might want to explain why there are no new tests required.

Done.

>> Source/WebCore/ChangeLog:27
>> +        (WebCore::GraphicsContext3D::texImage2D):
> 
> Here it might be good to briefly explain how each method differs from it's desktop GL counterpart.

Done.

>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:103
>> +    // resize multisample FBO
> 
> Nit. Comments should begin with a capital letter and end with a period.

Done.

>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:107
>> +    // resize regular FBO
> 
> Ditto.

Done.

>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:225
>> +    // FIXME : We don't care about multisampleFBO currently.
> 
> Extra space after FIXME. Might want to open a bug about this now.

Done. I filed a new bug to support anti-aliasing on GLES backend.
https://bugs.webkit.org/show_bug.cgi?id=76420

>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:240
>> +        notImplemented();
> 
> Why do you do this check twice? Perhaps you should return early if multisampling is needed?

Removed two checking. I think it's better to deal with supporting multisampleing in an another bug.

>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:250
>> +        notImplemented();
> 
> Ditto.

Removed two checking, too.

>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:281
>> +        info.name.append("[0]");
> 
> Extra space after the FIXME. Might want to investigate this a bit now to avoid code churn.

Removed lines.
Though I opened a new bug76427 to fix it, the issue is not reproduced after updating webgl conformance test.
Let me keep track of it. If there is no issue, then I'll close the bug without any change.

>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:289
>> +    // all previous rendering calls should be done before reading pixels.
> 
> Certainly the driver bug wouldn't affect both OpenGLES drivers and for whatever driver this work-around was added for. Can you just remove it?

Done.

[Martin Robinson]

Comment on attachment 122730 [details]
Patch

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

This is a good first pass, but I think we can get a lot closer to sharing most of this code. Please consider the suggestions below. Ping me online if you need further information about my comments.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:51
> +    if (m_attrs.antialias) {
> +        // FIXME: We don't support antialiasing yet.

See below: Here you could call resolveMultisamplingIfNecessary. I think with that change you could share this method entirely.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:70
> +    for (int i = 0; i < totalBytes; i += 4)
> +        std::swap(pixels[i], pixels[i + 2]); // Convert to BGRA

If you put this function back in the shared file you could just add a member like m_convertRenderingResultsToBGRA and make it true for GLES and false for desktop GL.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:133
> +        if (m_attributes.stencil) {
> +            ::glBindRenderbufferEXT(GL_RENDERBUFFER_EXT, m_stencilBuffer);
> +            ::glRenderbufferStorageEXT(GL_RENDERBUFFER_EXT, GL_STENCIL_INDEX8, width, height);
> +            ::glFramebufferRenderbufferEXT(GL_FRAMEBUFFER_EXT, GL_STENCIL_ATTACHMENT_EXT, GL_RENDERBUFFER_EXT, m_stencilBuffer);
> +            ::glBindRenderbufferEXT(GL_RENDERBUFFER_EXT, 0);
> +        }
> +        if (m_attributes.depth) {
> +            ::glBindRenderbufferEXT(GL_RENDERBUFFER_EXT, m_depthBuffer);
> +            ::glRenderbufferStorageEXT(GL_RENDERBUFFER_EXT, GL_DEPTH_COMPONENT16, width, height);
> +            ::glFramebufferRenderbufferEXT(GL_FRAMEBUFFER_EXT, GL_DEPTH_ATTACHMENT_EXT, GL_RENDERBUFFER_EXT, m_depthBuffer);
> +            ::glBindRenderbufferEXT(GL_RENDERBUFFER_EXT, 0);
> +        }
> +
> +        ::glBindRenderbufferEXT(GL_RENDERBUFFER_EXT, 0);

OpenGL ES also has the concept of a packed depth/stencil buffer as well -- see the GL_OES_packed_depth_stencil extension. If you used that you could share setting up the single-sample framebuffer with the desktop implementation. If, for some reason, that's not an option (your target device does not support packed depth/stencil), then at the very least you should consider avoiding calling ::glBindRenderbufferEXT(GL_RENDERBUFFER_EXT, 0) twice in a row here.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:194
> +    // Initialize renderbuffers to 0.
> +    GLfloat clearColor[] = {0, 0, 0, 0}, clearDepth = 0;
> +    GLint clearStencil = 0;
> +    GLboolean colorMask[] = {GL_TRUE, GL_TRUE, GL_TRUE, GL_TRUE}, depthMask = GL_TRUE;
> +    GLuint stencilMask = 0xffffffff;
> +    GLboolean isScissorEnabled = GL_FALSE;
> +    GLboolean isDitherEnabled = GL_FALSE;
> +    GLbitfield clearMask = GL_COLOR_BUFFER_BIT;
> +    ::glGetFloatv(GL_COLOR_CLEAR_VALUE, clearColor);
> +    ::glClearColor(0, 0, 0, 0);
> +    ::glGetBooleanv(GL_COLOR_WRITEMASK, colorMask);
> +    ::glColorMask(GL_TRUE, GL_TRUE, GL_TRUE, GL_TRUE);
> +    if (m_attrs.depth) {
> +        ::glGetFloatv(GL_DEPTH_CLEAR_VALUE, &clearDepth);
> +        ::glClearDepth(1);
> +        ::glGetBooleanv(GL_DEPTH_WRITEMASK, &depthMask);
> +        ::glDepthMask(GL_TRUE);
> +        clearMask |= GL_DEPTH_BUFFER_BIT;
> +    }
> +    if (m_attrs.stencil) {
> +        ::glGetIntegerv(GL_STENCIL_CLEAR_VALUE, &clearStencil);
> +        ::glClearStencil(0);
> +        ::glGetIntegerv(GL_STENCIL_WRITEMASK, reinterpret_cast<GLint*>(&stencilMask));
> +        ::glStencilMaskSeparate(GL_FRONT, 0xffffffff);
> +        clearMask |= GL_STENCIL_BUFFER_BIT;
> +    }
> +    isScissorEnabled = ::glIsEnabled(GL_SCISSOR_TEST);
> +    ::glDisable(GL_SCISSOR_TEST);
> +    isDitherEnabled = ::glIsEnabled(GL_DITHER);
> +    ::glDisable(GL_DITHER);
> +
> +    ::glClear(clearMask);
> +
> +    ::glClearColor(clearColor[0], clearColor[1], clearColor[2], clearColor[3]);
> +    ::glColorMask(colorMask[0], colorMask[1], colorMask[2], colorMask[3]);
> +    if (m_attrs.depth) {
> +        ::glClearDepth(clearDepth);
> +        ::glDepthMask(depthMask);
> +    }
> +    if (m_attrs.stencil) {
> +        ::glClearStencil(clearStencil);
> +        ::glStencilMaskSeparate(GL_FRONT, stencilMask);
> +    }
> +    if (isScissorEnabled)
> +        ::glEnable(GL_SCISSOR_TEST);
> +    else
> +        ::glDisable(GL_SCISSOR_TEST);
> +
> +    if (isDitherEnabled)
> +        ::glEnable(GL_DITHER);
> +    else
> +        ::glDisable(GL_DITHER);
> +
> +    if (mustRestoreFBO)
> +        ::glBindFramebufferEXT(GL_FRAMEBUFFER_EXT, m_boundFBO);

Is this bit of code exactly the same as the desktop counterpart? If so, you should split it off into a helper method and call it, rather than copy and pasting.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:213
> +    if (m_layerComposited)
> +        return;
> +
> +    makeContextCurrent();
> +    ::glBindFramebufferEXT(GL_FRAMEBUFFER_EXT, m_fbo);
> +    ::glActiveTexture(GL_TEXTURE0);
> +    ::glBindTexture(GL_TEXTURE_2D, m_compositorTexture);
> +    ::glCopyTexImage2D(GL_TEXTURE_2D, 0, m_internalColorFormat, 0, 0, m_currentWidth, m_currentHeight, 0);
> +    ::glBindTexture(GL_TEXTURE_2D, m_boundTexture0);
> +    ::glActiveTexture(m_activeTexture);
> +    ::glBindFramebufferEXT(GL_FRAMEBUFFER_EXT, m_boundFBO);
> +    ::glFinish();
> +    m_layerComposited = true;

Hrm. This is one case where it would have made more sense to use and #ifdef. Another approach is to move this to the common file and make a helper called resolveMultisamplingIfNecessary which has an empty implementation for OpenGLES.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:223
> +    makeContextCurrent();
> +    GLuint fbo = buffer ? buffer : m_fbo;
> +    if (fbo != m_boundFBO) {
> +        ::glBindFramebufferEXT(target, fbo);
> +        m_boundFBO = fbo;
> +    }

This could be shared easily if you simply ensured that m_attrs.antialias was always false for OpenGL ES.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:229
> +    makeContextCurrent();
> +    ::glCopyTexImage2D(target, level, internalformat, x, y, width, height, border);

You can also share this if you make resolveMultisamplingIfNecessary take a rectangle.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:242
> +    makeContextCurrent();
> +    ::glFlush();
> +    ::glReadPixels(x, y, width, height, format, type, data);

Hey, same thing here! :)

[ChangSeok Oh]

Created attachment 123938 [details]
Patch

[ChangSeok Oh]

Comment on attachment 122730 [details]
Patch

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

Thank you very much for review!

>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:51
>> +        // FIXME: We don't support antialiasing yet.
> 
> See below: Here you could call resolveMultisamplingIfNecessary. I think with that change you could share this method entirely.

Added resolveMultisamplingIfNecessary as you commented.  And also I moved readRenderingResults into GraphicsContext3DCommon.

>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:70
>> +        std::swap(pixels[i], pixels[i + 2]); // Convert to BGRA
> 
> If you put this function back in the shared file you could just add a member like m_convertRenderingResultsToBGRA and make it true for GLES and false for desktop GL.

Done. In my humble opinion, a new member is not needed. Because just using isGLES2Compliant() is enough to do that.

>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:194
>> +        ::glBindFramebufferEXT(GL_FRAMEBUFFER_EXT, m_boundFBO);
> 
> Is this bit of code exactly the same as the desktop counterpart? If so, you should split it off into a helper method and call it, rather than copy and pasting.

Done. I moved GC3D::reshape() into GC3DOpenGLCommon and newly added helper function resizeFBOs at GC3DOpenGL.cpp & GC3DOpenGLES.cpp for each files.

>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:213
>> +    m_layerComposited = true;
> 
> Hrm. This is one case where it would have made more sense to use and #ifdef. Another approach is to move this to the common file and make a helper called resolveMultisamplingIfNecessary which has an empty implementation for OpenGLES.

Done.

>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:223
>> +    }
> 
> This could be shared easily if you simply ensured that m_attrs.antialias was always false for OpenGL ES.

Done. Added condition in GC3D::validateAttributes to disable antialiasing for GLES.

>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:229
>> +    ::glCopyTexImage2D(target, level, internalformat, x, y, width, height, border);
> 
> You can also share this if you make resolveMultisamplingIfNecessary take a rectangle.

Done.

>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:242
>> +    ::glReadPixels(x, y, width, height, format, type, data);
> 
> Hey, same thing here! :)

ditto. :)

[ChangSeok Oh]

Comment on attachment 122730 [details]
Patch

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

>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:133
>> +        ::glBindRenderbufferEXT(GL_RENDERBUFFER_EXT, 0);
> 
> OpenGL ES also has the concept of a packed depth/stencil buffer as well -- see the GL_OES_packed_depth_stencil extension. If you used that you could share setting up the single-sample framebuffer with the desktop implementation. If, for some reason, that's not an option (your target device does not support packed depth/stencil), then at the very least you should consider avoiding calling ::glBindRenderbufferEXT(GL_RENDERBUFFER_EXT, 0) twice in a row here.

Hm. To tell you the truth. I haven't know about GL_OES_packed_depth_stencil extension. I need more time to research and test it. :p. Here I opened a new bug to support it. http://webkit.org/b/77011
Removed  calling ::glBindRenderbufferEXT twice.

[ChangSeok Oh]

Created attachment 124068 [details]
Patch

[ChangSeok Oh]

(In reply to comment #8)
> (From update of attachment 122730 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122730&action=review
> 
> >> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:133
> >> +        ::glBindRenderbufferEXT(GL_RENDERBUFFER_EXT, 0);
> > 
> > OpenGL ES also has the concept of a packed depth/stencil buffer as well -- see the GL_OES_packed_depth_stencil extension. If you used that you could share setting up the single-sample framebuffer with the desktop implementation. If, for some reason, that's not an option (your target device does not support packed depth/stencil), then at the very least you should consider avoiding calling ::glBindRenderbufferEXT(GL_RENDERBUFFER_EXT, 0) twice in a row here.
> 
> Hm. To tell you the truth. I haven't know about GL_OES_packed_depth_stencil extension. I need more time to research and test it. :p. Here I opened a new bug to support it. http://webkit.org/b/77011
> Removed  calling ::glBindRenderbufferEXT twice.

My target looks like supporting GL_OES_packed_depth_stencil. So I've updated the patch.

[Martin Robinson]

Comment on attachment 124068 [details]
Patch

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

Looking good. I've listed a few small things below.

> Source/WebCore/platform/graphics/GraphicsContext3D.h:902
> +    void resolveMultisamplingIfNecessary(GC3Dint x, GC3Dint y, GC3Dsizei width, GC3Dsizei height);

Better to pass an IntRect here.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:46
> +bool GraphicsContext3D::resizeFBOs(int width, int height)

Better to pass an const IntSize& here. reshapeFBOs might be a better name to match the caller.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:74
> +        bool supportPackedDepthStencilBuffer = false;
> +        if (isGLES2Compliant())
> +            supportPackedDepthStencilBuffer = extensions->supports("GL_OES_packed_depth_stencil");
> +        else
> +            supportPackedDepthStencilBuffer = extensions->supports("GL_EXT_packed_depth_stencil");
> +
> +        if (supportPackedDepthStencilBuffer) {
> +            if (isGLES2Compliant())
> +                extensions->ensureEnabled("GL_OES_packed_depth_stencil");
> +            else
> +                extensions->ensureEnabled("GL_EXT_packed_depth_stencil");

Here it might be better to do something like this:

const const char* packedDepthStencilExtension = isGLES2Compliant() ? "GL_OES_packed_depth_stencil" : "GL_EXT_packed_depth_stencil";
if (extensions->supports(packedDepthStencilExtension)) {
    extensions->ensureEnabled(packedDepthStencilExtension);

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:160
> +    if (m_attrs.antialias)
> +        resolveMultisamplingIfNecessary(0, 0, m_currentWidth, m_currentHeight);

It seems like this should be checking whether m_boundFBO == m_multisampleFBO, but we need to ask the original author to answer that question, I think. Later it might be good to suck more of the logic into resolveMultisamplingIfNecessary. Perhaps it would be better as bool resolveMultisamplingIfNecessaryAndBindSingleSampleFramebuffer() or something like that. :)

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:204
> +            std::swap(pixels[i], pixels[i + 2]); // Convert to BGRA

Nit: Missing a period at the end of this comment.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:59
> +    bool supportPackedDepthStencilBuffer = false;
> +    if (m_attrs.stencil || m_attrs.depth) {
> +        // We don't allow the logic where stencil is required and depth is not.
> +        // See GraphicsContext3D::validateAttributes.
> +        Extensions3D* extensions = getExtensions();
> +        supportPackedDepthStencilBuffer = extensions->supports("GL_OES_packed_depth_stencil");

This could simply be:

// We don't allow the logic where stencil is required and depth is not.
// See GraphicsContext3D::validateAttributes.
supportsPackedDepthStencilBuffer = (m_attrs.stencil || m_attrs.depth) && getExtensions()->->supports("GL_OES_packed_depth_stencil");

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:77
> +    if (!m_attrs.antialias && (m_attrs.stencil || m_attrs.depth)) {

Better to ASSERT(!m_attrs.antialias) instead of checking it when you know it's always false.

[ChangSeok Oh]

Comment on attachment 124068 [details]
Patch

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

Thank you for review. The codes look more simple and better! :)
BTW, I have a small question, please see below.

>> Source/WebCore/platform/graphics/GraphicsContext3D.h:902
>> +    void resolveMultisamplingIfNecessary(GC3Dint x, GC3Dint y, GC3Dsizei width, GC3Dsizei height);
> 
> Better to pass an IntRect here.

Done.

>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:46
>> +bool GraphicsContext3D::resizeFBOs(int width, int height)
> 
> Better to pass an const IntSize& here. reshapeFBOs might be a better name to match the caller.

Done.

>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:74
>> +                extensions->ensureEnabled("GL_EXT_packed_depth_stencil");
> 
> Here it might be better to do something like this:
> 
> const const char* packedDepthStencilExtension = isGLES2Compliant() ? "GL_OES_packed_depth_stencil" : "GL_EXT_packed_depth_stencil";
> if (extensions->supports(packedDepthStencilExtension)) {
>     extensions->ensureEnabled(packedDepthStencilExtension);

Done.

>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:160
>> +        resolveMultisamplingIfNecessary(0, 0, m_currentWidth, m_currentHeight);
> 
> It seems like this should be checking whether m_boundFBO == m_multisampleFBO, but we need to ask the original author to answer that question, I think. Later it might be good to suck more of the logic into resolveMultisamplingIfNecessary. Perhaps it would be better as bool resolveMultisamplingIfNecessaryAndBindSingleSampleFramebuffer() or something like that. :)

Reasonable. When I test with "if (m_attrs.antialias && m_boundFBO == m_multisampleFBO)" in here, no issue found. So, do you want to deal with the change you mentioned in this bug? Please let me know. :)

>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:204
>> +            std::swap(pixels[i], pixels[i + 2]); // Convert to BGRA
> 
> Nit: Missing a period at the end of this comment.

Done. Also I did it at some lines else. :)

>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:59
>> +        supportPackedDepthStencilBuffer = extensions->supports("GL_OES_packed_depth_stencil");
> 
> This could simply be:
> 
> // We don't allow the logic where stencil is required and depth is not.
> // See GraphicsContext3D::validateAttributes.
> supportsPackedDepthStencilBuffer = (m_attrs.stencil || m_attrs.depth) && getExtensions()->->supports("GL_OES_packed_depth_stencil");

Done.

>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:77
>> +    if (!m_attrs.antialias && (m_attrs.stencil || m_attrs.depth)) {
> 
> Better to ASSERT(!m_attrs.antialias) instead of checking it when you know it's always false.

Done. Inserted it at the upper row.

[ChangSeok Oh]

Created attachment 125490 [details]
Patch

[WebKit Review Bot]

Comment on attachment 125490 [details]
Patch

Clearing flags on attachment: 125490

Committed r106815: <http://trac.webkit.org/changeset/106815>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.