[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 #124068|review?                     |review-
               Flag|                            |




--- Comment #11 from Martin Robinson <mrobinson at webkit.org>  2012-02-02 16:00:58 PST ---
(From update of attachment 124068)
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.

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