[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 #4 from heejin.r.chung at samsung.com  2012-09-16 23:46:17 PST ---
Hi Simon, thank you for your comments!
I uploaded a new patch with most of your suggestions applied, except the one about making FBO and ANGLE initialization code sharable.
Maybe that one should be in another patch?

(In reply to comment #2)
> (From update of attachment 164049 [details])
> 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.

Took your advice and changed it to "initializedShims".

> > 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.

You're right. Removed #if and only left initializeOpenGLShims().

> > 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?

I agree that it would be nice if this code could be shared. Maybe something like GC3D::initializeFBOs() and GC3D::initializeANGLE().



> > 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?

Changed code so that false is returned when !m_private, and true is returned when m_renderStyle == RenderToCurrentGLContext.

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

Fixed typo.

> > 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?

Added notImplemented().

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