Bug 96627 – [EFL] Use the shareable GraphicsContext3DOpenGL* implementation.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Regina Chung]

Current implementation of GraphicsContext3DEfl has much duplicated code, due to characteristics of Evas.
It would be better to remove the duplicated code, use the common GC3D implementation under graphics\opengl,
and deal with Evas in another way.

[Regina Chung]

Created attachment 164049 [details]
Patch

[Simon Hausmann]

Comment on attachment 164049 [details]
Patch

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?

[Regina Chung]

Created attachment 164343 [details]
Patch

[Regina Chung]

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

[Simon Hausmann]

Kim, do the Evas GL and other EFL bits look okay to you?

[Kenneth Rohde Christiansen]

Comment on attachment 164343 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164343&action=review

> Source/WebCore/ChangeLog:11
> +        Changed EFL implementation of GraphicsContext3D(GC3D) to use GraphicsContext3DOpenGL*.
> +        It was initially implemented in a different way, due to characteristics of Evas,
> +        but it would be better to use the common implementation and find another way
> +        to deal with Evas, especially because all the duplicated code.

Plus the fact that maybe using Evas will make less sense in the near future, where we can do the same directly using OpenGL and eventually combine it with Wayland for compositing. I support this change!

[Kenneth Rohde Christiansen]

Comment on attachment 164343 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164343&action=review

Did you compare this new code with what Qt is doing?

> Source/WebCore/PlatformEfl.cmake:295
> -    platform/graphics/cairo/GLContext.cpp
> -    platform/graphics/cairo/GraphicsContext3DCairo.cpp
> -    platform/graphics/cairo/GraphicsContext3DPrivate.cpp
> +    platform/graphics/efl/GraphicsContext3DEfl.cpp
> +    platform/graphics/efl/GraphicsContext3DPrivate.cpp

So any reason why GTK doesnt want to use the same? I guess this is still based on cairo right

> Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:48
> +    , m_attrs(attrs)

attributes?

> Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:92
> +        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);

Maybe a small comment here makes sense, so that people dont have to look up the functions

> Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:105
> +            glBindFramebuffer(GraphicsContext3D::FRAMEBUFFER, m_fbo);

Maybe m_fbo should be called soemthing like m_nonMultisampleFBO to make the difference clear

> Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:120
> +    // ANGLE initialization.
> +    ShBuiltInResources ANGLEResources;
> +    ShInitBuiltInResources(&ANGLEResources);

Shouldnt this be per PLATFORM(WIN) only?

> Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:139
> +    glClearColor(0.0, 0.0, 0.0, 0.0);

why not white?

> Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:165
> +    if (!m_private)
> +        return false;

enwline after return

> Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:167
> +    if (m_renderStyle == RenderToCurrentGLContext)
> +        return true;

same here

> Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:60
> +    // For WebKit2, we need to create a dummy ecoreEvas object for the WebProcess in order to use EvasGL APIs.
> +    ecore_evas_init();
> +    Ecore_Evas* ecoreEvas = ecore_evas_gl_x11_new(0, 0, 0, 0, 1, 1);
> +    if (!ecoreEvas)
> +        return;
> +    evas = ecore_evas_get(ecoreEvas);
> +    if (!evas)
> +        return;

As this is X11 dependent we might want to abstract that out. Or at least add some ifdefs

> Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:91
> +    if (m_surface)
> +        evas_gl_surface_destroy(m_evasGL, m_surface);

OwnPtr works for this, look at RenderThemeEfl for examples

> Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:96
> +
> +    evas_gl_free(m_evasGL);

where is the ecoreEvas freed?

[Gyuyoung Kim]

Comment on attachment 164343 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164343&action=review

> Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:48
> +        // WebKit1 code path.

I don't understand this comment well.

> Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:57
> +        return;

new line?

> Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:58
> +    evas = ecore_evas_get(ecoreEvas);

Why not define this here ?

Evas* evas =  ecore_evas_get(ecoreEvas);

[Regina Chung]

(In reply to comment #7)
> (From update of attachment 164343 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164343&action=review
> Did you compare this new code with what Qt is doing?

Yes, I referenced Qt's code while changing our code.

> > Source/WebCore/PlatformEfl.cmake:295
> > -    platform/graphics/cairo/GLContext.cpp
> > -    platform/graphics/cairo/GraphicsContext3DCairo.cpp
> > -    platform/graphics/cairo/GraphicsContext3DPrivate.cpp
> > +    platform/graphics/efl/GraphicsContext3DEfl.cpp
> > +    platform/graphics/efl/GraphicsContext3DPrivate.cpp
> So any reason why GTK doesnt want to use the same? I guess this is still based on cairo right
> > Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:48
> > +    , m_attrs(attrs)
> attributes?

m_attrs and m_fbo are actually defined in the common code, so I think it should be changed in a seperate patch (if necessary).
Maybe it can be done when moving the fbo and ANGLE initialization code to the common area.

> > Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:92
> > +        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);
> Maybe a small comment here makes sense, so that people dont have to look up the functions

Added a short comment.

> > Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:105
> > +            glBindFramebuffer(GraphicsContext3D::FRAMEBUFFER, m_fbo);
> Maybe m_fbo should be called soemthing like m_nonMultisampleFBO to make the difference clear
> > Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:120
> > +    // ANGLE initialization.
> > +    ShBuiltInResources ANGLEResources;
> > +    ShInitBuiltInResources(&ANGLEResources);
> Shouldnt this be per PLATFORM(WIN) only?
> > Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:139
> > +    glClearColor(0.0, 0.0, 0.0, 0.0);
> why not white?

The default clear color for opengl(es) is black. 
However, I just removed this line because I think the clear color 
should be set in the actual WebGL content or whatever is using GC3D.

> > Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:165
> > +    if (!m_private)
> > +        return false;
> enwline after return
> > Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:167
> > +    if (m_renderStyle == RenderToCurrentGLContext)
> > +        return true;
> same here

Added newlines.

> > Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:60
> > +    // For WebKit2, we need to create a dummy ecoreEvas object for the WebProcess in order to use EvasGL APIs.
> > +    ecore_evas_init();
> > +    Ecore_Evas* ecoreEvas = ecore_evas_gl_x11_new(0, 0, 0, 0, 1, 1);
> > +    if (!ecoreEvas)
> > +        return;
> > +    evas = ecore_evas_get(ecoreEvas);
> > +    if (!evas)
> > +        return;
> As this is X11 dependent we might want to abstract that out. Or at least add some ifdefs

Added #ifdef HAVE_ECORE_X.

> > Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:91
> > +    if (m_surface)
> > +        evas_gl_surface_destroy(m_evasGL, m_surface);
> OwnPtr works for this, look at RenderThemeEfl for examples

I see that it works for Ecore_Evas* but not for Evas_GL related pointers.
Do you think I should add code for them to our OwnPtr implementation?

> > Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:96
> > +
> > +    evas_gl_free(m_evasGL);
> where is the ecoreEvas freed?

added OwnPtr<m_ecoreEvas> as a member variable.

[Regina Chung]

(In reply to comment #8)
> (From update of attachment 164343 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164343&action=review
> > Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:48
> > +        // WebKit1 code path.
> I don't understand this comment well.

I temporarily removed the WebKit1 related code because it brang up problems in WebKit2 but am planning on putting it back in with some fixes in another patch.
(https://bugs.webkit.org/show_bug.cgi?id=97651)
Anyways I changed the comment to make it a bit more understandable.

> > Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:57
> > +        return;
> new line?

Added new line.

> > Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:58
> > +    evas = ecore_evas_get(ecoreEvas);
> Why not define this here ?
> Evas* evas =  ecore_evas_get(ecoreEvas);

This also had to do with putting back the WebKit1 code in, but I changed the code as you suggested. I guess I shouldn't commit code that 'must' be fixed in the future :)

[Regina Chung]

Created attachment 166164 [details]
Patch

[Simon Hausmann]

Comment on attachment 166164 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166164&action=review

> Source/WebCore/ChangeLog:15
> +        * PlatformEfl.cmake: Removed cairo implementaion of GC3D and added efl files to WebCore_SOURCES.

implementaion -> implementation

> Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:205
> +    cairo_rectangle(cr, 0, 0, canvasWidth, canvasHeight);
> +    cairo_set_operator(cr, CAIRO_OPERATOR_CLEAR);
> +    cairo_paint(cr);
>  
> -void GraphicsContext3D::deleteProgram(Platform3DObject program)
> -{
> -    m_private->deleteProgram(program);
> -}
> +    RefPtr<cairo_surface_t> imageSurface = adoptRef(cairo_image_surface_create_for_data(
> +        const_cast<unsigned char*>(imagePixels), CAIRO_FORMAT_ARGB32, imageWidth, imageHeight, imageWidth * 4));
>  
> -void GraphicsContext3D::deleteRenderbuffer(Platform3DObject buffer)
> -{
> -    m_private->deleteRenderbuffer(buffer);
> -}
> +    // OpenGL keeps the pixels stored bottom up, so we need to flip the image here.
> +    cairo_translate(cr, 0, imageHeight);
> +    cairo_scale(cr, 1, -1);
>  
> -void GraphicsContext3D::deleteShader(Platform3DObject shader)
> -{
> -    m_private->deleteShader(shader);
> -}
> +    cairo_set_operator(cr, CAIRO_OPERATOR_OVER);

Just a thought, but isn't CLEAR + OVER the same as just OPERATOR_SOURCE? I mean, perhaps you can safe an operation here :)

[Yael]

Comment on attachment 166164 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166164&action=review

> Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:43
> +    if (renderStyle == GraphicsContext3D::RenderToCurrentGLContext)

There seems to be an inconsistency here. GraphicsContext3D::createForCurrentGLContext() sets renderStyle to GraphicsContext3D::RenderToCurrentGLContext.
So we always return here without creating a GL context.

[Yael]

Comment on attachment 166164 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166164&action=review

> Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:47
> +    , m_compiler(isGLES2Compliant() ? SH_ESSL_OUTPUT : SH_GLSL_OUTPUT)

This will crash.
GraphicsContext3D::isGLES2Compliant() is calling on m_private which was not created yet.
You need to change GraphicsContext3D::isGLES2Compliant() to simply return a value, like other ports do.

[Simon Hausmann]

Comment on attachment 166164 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166164&action=review

>> Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:47
>> +    , m_compiler(isGLES2Compliant() ? SH_ESSL_OUTPUT : SH_GLSL_OUTPUT)
> 
> This will crash.
> GraphicsContext3D::isGLES2Compliant() is calling on m_private which was not created yet.
> You need to change GraphicsContext3D::isGLES2Compliant() to simply return a value, like other ports do.

Yael is right, and there's no reason for isGLES2Compliant() to use m_private, since it's just returning true or false depending on #ifdef, so that's easy to fix :)

>> Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:43
>> +    if (renderStyle == GraphicsContext3D::RenderToCurrentGLContext)
> 
> There seems to be an inconsistency here. GraphicsContext3D::createForCurrentGLContext() sets renderStyle to GraphicsContext3D::RenderToCurrentGLContext.
> So we always return here without creating a GL context.

That seems generally correct, doesn't it? There's no surface, etc. to create in that case, no?

[Yael]

(In reply to comment #15)
> (From update of attachment 166164 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166164&action=review
> 
> >> Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:43
> >> +    if (renderStyle == GraphicsContext3D::RenderToCurrentGLContext)
> > 
> > There seems to be an inconsistency here. GraphicsContext3D::createForCurrentGLContext() sets renderStyle to GraphicsContext3D::RenderToCurrentGLContext.
> > So we always return here without creating a GL context.
> 
> That seems generally correct, doesn't it? There's no surface, etc. to create in that case, no?

The context that we are supposed to create here is used as key for the hash table. Not creating one, as proposed by this patch, triggers ASSERT in debug builds.

[Regina Chung]

(In reply to comment #12)
> (From update of attachment 166164 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166164&action=review
> > Source/WebCore/ChangeLog:15
> > +        * PlatformEfl.cmake: Removed cairo implementaion of GC3D and added efl files to WebCore_SOURCES.
> implementaion -> implementation

I'll fix that typo :)
> > Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:205
> > +    cairo_rectangle(cr, 0, 0, canvasWidth, canvasHeight);
> > +    cairo_set_operator(cr, CAIRO_OPERATOR_CLEAR);
> > +    cairo_paint(cr);
> >  
> > -void GraphicsContext3D::deleteProgram(Platform3DObject program)
> > -{
> > -    m_private->deleteProgram(program);
> > -}
> > +    RefPtr<cairo_surface_t> imageSurface = adoptRef(cairo_image_surface_create_for_data(
> > +        const_cast<unsigned char*>(imagePixels), CAIRO_FORMAT_ARGB32, imageWidth, imageHeight, imageWidth * 4));
> >  
> > -void GraphicsContext3D::deleteRenderbuffer(Platform3DObject buffer)
> > -{
> > -    m_private->deleteRenderbuffer(buffer);
> > -}
> > +    // OpenGL keeps the pixels stored bottom up, so we need to flip the image here.
> > +    cairo_translate(cr, 0, imageHeight);
> > +    cairo_scale(cr, 1, -1);
> >  
> > -void GraphicsContext3D::deleteShader(Platform3DObject shader)
> > -{
> > -    m_private->deleteShader(shader);
> > -}
> > +    cairo_set_operator(cr, CAIRO_OPERATOR_OVER);
> Just a thought, but isn't CLEAR + OVER the same as just OPERATOR_SOURCE? I mean, perhaps you can safe an operation here :)

Yes, you're right, removed the unnecessary operation as you suggested. Thanks!

[Regina Chung]

(In reply to comment #14)
> (From update of attachment 166164 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166164&action=review
> > Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:47
> > +    , m_compiler(isGLES2Compliant() ? SH_ESSL_OUTPUT : SH_GLSL_OUTPUT)
> This will crash.
> GraphicsContext3D::isGLES2Compliant() is calling on m_private which was not created yet.
> You need to change GraphicsContext3D::isGLES2Compliant() to simply return a value, like other ports do.

I'll move the isGLES2Compliant() implementation in GC3DPrivate to GC3D.

[Regina Chung]

(In reply to comment #16)
> (In reply to comment #15)
> > (From update of attachment 166164 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=166164&action=review
> > 
> > >> Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:43
> > >> +    if (renderStyle == GraphicsContext3D::RenderToCurrentGLContext)
> > > 
> > > There seems to be an inconsistency here. GraphicsContext3D::createForCurrentGLContext() sets renderStyle to GraphicsContext3D::RenderToCurrentGLContext.
> > > So we always return here without creating a GL context.
> > 
> > That seems generally correct, doesn't it? There's no surface, etc. to create in that case, no?
> The context that we are supposed to create here is used as key for the hash table. Not creating one, as proposed by this patch, triggers ASSERT in debug builds.

I should set m_evasGLContext and m_surface to the current values, but Evas doesn't provide any APIs to query them :(
I guess I'll just have to query the current context and surface using egl in GC3D and set the GC3DPrivate values with them.
(EvasGL and EGL APIs can't be used in the same source files so I'll need to call the EGL functions from GC3DEfl.cpp)
Do you think that will be okay? Or do you have another suggestion?

[Regina Chung]

(In reply to comment #19)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > (From update of attachment 166164 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=166164&action=review
> > > 
> > > >> Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:43
> > > >> +    if (renderStyle == GraphicsContext3D::RenderToCurrentGLContext)
> > > > 
> > > > There seems to be an inconsistency here. GraphicsContext3D::createForCurrentGLContext() sets renderStyle to GraphicsContext3D::RenderToCurrentGLContext.
> > > > So we always return here without creating a GL context.
> > > 
> > > That seems generally correct, doesn't it? There's no surface, etc. to create in that case, no?
> > The context that we are supposed to create here is used as key for the hash table. Not creating one, as proposed by this patch, triggers ASSERT in debug builds.
> I should set m_evasGLContext and m_surface to the current values, but Evas doesn't provide any APIs to query them :(
> I guess I'll just have to query the current context and surface using egl in GC3D and set the GC3DPrivate values with them.
Actually, it will be glX functions for OpenGL and EGL functions for OpenGL ES.

> (EvasGL and EGL APIs can't be used in the same source files so I'll need to call the EGL functions from GC3DEfl.cpp)
> Do you think that will be okay? Or do you have another suggestion?

[Yael]

(In reply to comment #19)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > (From update of attachment 166164 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=166164&action=review
> I should set m_evasGLContext and m_surface to the current values, but Evas doesn't provide any APIs to query them :(
> I guess I'll just have to query the current context and surface using egl in GC3D and set the GC3DPrivate values with them.
> (EvasGL and EGL APIs can't be used in the same source files so I'll need to call the EGL functions from GC3DEfl.cpp)
> Do you think that will be okay? Or do you have another suggestion?
That sounds good to me.

[Regina Chung]

Created attachment 167522 [details]
Patch

[Regina Chung]

Comment on attachment 167522 [details]
Patch

Patch with suggested changes.

[Regina Chung]

Comment on attachment 167522 [details]
Patch

Patch with suggested changes.

[Kenneth Rohde Christiansen]

Comment on attachment 167522 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167522&action=review

> Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:56
> +    , m_compiler(isGLES2Compliant() ? SH_ESSL_OUTPUT : SH_GLSL_OUTPUT)
> +    , m_attrs(attrs)
> +    , m_texture(0)
> +    , m_compositorTexture(0)
> +    , m_fbo(0)
> +#if USE(OPENGL_ES_2)
> +    , m_depthBuffer(0)
> +    , m_stencilBuffer(0)
> +#endif

So some of the GLES2 support is build in using ifdefs and some is runtime? isGLES2Compliant?

[Gyuyoung Kim]

Comment on attachment 167522 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167522&action=review

> Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:25
> +#if USE(OPENGL_ES_2)

It looks WebKit prefers to place #if macro below include list as below,

#include "GraphicsContext3DPrivate.h"
#include "ImageData.h"
#include "NotImplemented.h"
#include "OpenGLShims.h"
#include "PlatformContextCairo.h"
#include <GL/glx.h>

#if USE(OPENGL_ES_2)
#include "Extensions3DOpenGLES.h"
#else
#include "Extensions3DOpenGL.h"
#endif

[Regina Chung]

(In reply to comment #25)
> (From update of attachment 167522 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167522&action=review
> > Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:56
> > +    , m_compiler(isGLES2Compliant() ? SH_ESSL_OUTPUT : SH_GLSL_OUTPUT)
> > +    , m_attrs(attrs)
> > +    , m_texture(0)
> > +    , m_compositorTexture(0)
> > +    , m_fbo(0)
> > +#if USE(OPENGL_ES_2)
> > +    , m_depthBuffer(0)
> > +    , m_stencilBuffer(0)
> > +#endif
> So some of the GLES2 support is build in using ifdefs and some is runtime? isGLES2Compliant?

The implementation of isGLES2Compliant() is actually defined using ifdefs.
I think the API exists to keep the ifdefs local (in platform/graphics).

[Regina Chung]

Created attachment 167671 [details]
Patch

[Regina Chung]

(In reply to comment #26)
> (From update of attachment 167522 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167522&action=review
> > Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:25
> > +#if USE(OPENGL_ES_2)
> It looks WebKit prefers to place #if macro below include list as below,
> #include "GraphicsContext3DPrivate.h"
> #include "ImageData.h"
> #include "NotImplemented.h"
> #include "OpenGLShims.h"
> #include "PlatformContextCairo.h"
> #include <GL/glx.h>
> #if USE(OPENGL_ES_2)
> #include "Extensions3DOpenGLES.h"
> #else
> #include "Extensions3DOpenGL.h"
> #endif

Changed the order if includes as suggested.

[Regina Chung]

Created attachment 167700 [details]
Patch

[Yael]

(In reply to comment #30)
> Created an attachment (id=167700) [details]
> Patch
This looks like a good step in the right direction.

[Simon Hausmann]

Comment on attachment 167700 [details]
Patch

LGTM - probably a good starting point for further fixes if required. Quite red this patch :)

[Kenneth Rohde Christiansen]

Comment on attachment 167700 [details]
Patch

Let's land this and take it from there

[WebKit Review Bot]

Comment on attachment 167700 [details]
Patch

Clearing flags on attachment: 167700

Committed r130966: <http://trac.webkit.org/changeset/130966>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Yael]

(In reply to comment #19)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > (From update of attachment 166164 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=166164&action=review
> > > 
> > > >> Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:43
> > > >> +    if (renderStyle == GraphicsContext3D::RenderToCurrentGLContext)
> > > > 
> > > > There seems to be an inconsistency here. GraphicsContext3D::createForCurrentGLContext() sets renderStyle to GraphicsContext3D::RenderToCurrentGLContext.
> > > > So we always return here without creating a GL context.
> > > 
> > > That seems generally correct, doesn't it? There's no surface, etc. to create in that case, no?
> > The context that we are supposed to create here is used as key for the hash table. Not creating one, as proposed by this patch, triggers ASSERT in debug builds.
> 
> I should set m_evasGLContext and m_surface to the current values, but Evas doesn't provide any APIs to query them :(
> I guess I'll just have to query the current context and surface using egl in GC3D and set the GC3DPrivate values with them.
> (EvasGL and EGL APIs can't be used in the same source files so I'll need to call the EGL functions from GC3DEfl.cpp)
> Do you think that will be okay? Or do you have another suggestion?

This additional code was lost in the final patch.
Filed https://bugs.webkit.org/show_bug.cgi?id=99325 to add it back.