[Webkit-unassigned] [Bug 96627] [EFL] Use the shareable GraphicsContext3DOpenGL* implementation.

[bugzilla-daemon at webkit.org]

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





--- Comment #2 from Simon Hausmann <hausmann at webkit.org>  2012-09-14 23:08:14 PST ---
(From update of attachment 164049)
View in context: https://bugs.webkit.org/attachment.cgi?id=164049&action=review

> Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:73
> +    static bool initialized = false;
> +    static bool success = true;

In the presence of "success" I suggest to rename the generic "initialized" to something more specific, because it refers only to the initialization of the GL shims.

> Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:79
> +#if USE(OPENGL_ES_2)
> +        success = initializeOpenGLESShims();
> +#else
> +        success = initializeOpenGLShims();
> +#endif

initializeOpenGLESShims() doesn't exists AFAICS. initializeOpenGLShims() should be used for both I believe.

> Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:144
> +    if (renderStyle == RenderOffscreen) {
> +        // Create buffers for the canvas FBO.
> +        glGenFramebuffers(/* count */ 1, &m_fbo);
> +
> +        glGenTextures(1, &m_texture);
> +        glBindTexture(GraphicsContext3D::TEXTURE_2D, m_texture);
> +        glTexParameterf(GraphicsContext3D::TEXTURE_2D, GraphicsContext3D::TEXTURE_MAG_FILTER, GraphicsContext3D::LINEAR);
> +        glTexParameterf(GraphicsContext3D::TEXTURE_2D, GraphicsContext3D::TEXTURE_MIN_FILTER, GraphicsContext3D::LINEAR);
> +        glTexParameteri(GraphicsContext3D::TEXTURE_2D, GraphicsContext3D::TEXTURE_WRAP_S, GraphicsContext3D::CLAMP_TO_EDGE);
> +        glTexParameteri(GraphicsContext3D::TEXTURE_2D, GraphicsContext3D::TEXTURE_WRAP_T, GraphicsContext3D::CLAMP_TO_EDGE);
> +        glBindTexture(GraphicsContext3D::TEXTURE_2D, 0);
> +
> +        // Create a multisample FBO.
> +        if (m_attrs.antialias) {
> +            glGenFramebuffers(1, &m_multisampleFBO);
> +            glBindFramebuffer(GraphicsContext3D::FRAMEBUFFER, m_multisampleFBO);
> +            m_boundFBO = m_multisampleFBO;
> +            glGenRenderbuffers(1, &m_multisampleColorBuffer);
> +            if (m_attrs.stencil || m_attrs.depth)
> +                glGenRenderbuffers(1, &m_multisampleDepthStencilBuffer);
> +        } else {
> +            // Bind canvas FBO.
> +            glBindFramebuffer(GraphicsContext3D::FRAMEBUFFER, m_fbo);
> +            m_boundFBO = m_fbo;
> +#if USE(OPENGL_ES_2)
> +            if (m_attrs.depth)
> +                glGenRenderbuffers(1, &m_depthBuffer);
> +            if (m_context->m_attrs.stencil)
> +                glGenRenderbuffers(1, &m_stencilBuffer);
> +#endif
> +            if (m_attrs.stencil || m_attrs.depth)
> +                glGenRenderbuffers(1, &m_depthStencilBuffer);
> +        }
> +    }
> +
> +    // ANGLE initialization.
> +    ShBuiltInResources ANGLEResources;
> +    ShInitBuiltInResources(&ANGLEResources);
> +
> +    getIntegerv(GraphicsContext3D::MAX_VERTEX_ATTRIBS, &ANGLEResources.MaxVertexAttribs);
> +    getIntegerv(GraphicsContext3D::MAX_VERTEX_UNIFORM_VECTORS, &ANGLEResources.MaxVertexUniformVectors);
> +    getIntegerv(GraphicsContext3D::MAX_VARYING_VECTORS, &ANGLEResources.MaxVaryingVectors);
> +    getIntegerv(GraphicsContext3D::MAX_VERTEX_TEXTURE_IMAGE_UNITS, &ANGLEResources.MaxVertexTextureImageUnits);
> +    getIntegerv(GraphicsContext3D::MAX_COMBINED_TEXTURE_IMAGE_UNITS, &ANGLEResources.MaxCombinedTextureImageUnits);
> +    getIntegerv(GraphicsContext3D::MAX_TEXTURE_IMAGE_UNITS, &ANGLEResources.MaxTextureImageUnits);
> +    getIntegerv(GraphicsContext3D::MAX_FRAGMENT_UNIFORM_VECTORS, &ANGLEResources.MaxFragmentUniformVectors);
> +
> +    // Always set to 1 for OpenGL ES.
> +    ANGLEResources.MaxDrawBuffers = 1;
> +    m_compiler.setResources(ANGLEResources);
> +
> +#if !USE(OPENGL_ES_2)
> +    glEnable(GL_POINT_SPRITE);
> +    glEnable(GL_VERTEX_PROGRAM_POINT_SIZE);
> +#endif
>  
> +    glClearColor(0.0, 0.0, 0.0, 0.0);
> +}

It feels like that the bulk of this code is a prime candidate for sharing between difference GC3D implementations. What do you think?

> Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:169
> +    if (!m_private || m_renderStyle == RenderToCurrentGLContext)
> +        return false;

Are you sure that false should be returned when m_renderStyle == RenderToCurrentGLContext?

> Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:49
> +        // FIXME: Retrive evas object from platformPageClient->view().

Retrive -> Retrieve

> Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:163
> +#if USE(TEXTURE_MAPPER_GL)
> +void GraphicsContext3DPrivate::paintToTextureMapper(TextureMapper*, const FloatRect& target, const TransformationMatrix&, float opacity, BitmapTexture* mask)
>  {
> -    makeContextCurrent();
> -    m_api->glGetAttachedShaders(program, maxCount, count, shaders);
>  }

Perhaps add a notImplemented() here?

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