[Webkit-unassigned] [Bug 76248] Initial implementation of GraphicsContext3DOpenGLES.cpp

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=76248


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #122537|review?, commit-queue?      |review-
               Flag|                            |




--- Comment #2 from Martin Robinson <mrobinson at webkit.org>  2012-01-16 09:22:21 PST ---
(From update of attachment 122537)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.