[webkit-reviews] review denied: [Bug 76248] Initial implementation of GraphicsContext3DOpenGLES.cpp : [Attachment 122730] Patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied ChangSeok Oh
<kevin.cs.oh at gmail.com>'s request for review:
Bug 76248: Initial implementation of GraphicsContext3DOpenGLES.cpp
https://bugs.webkit.org/show_bug.cgi?id=76248

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

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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! :)