Bug 96627 - [EFL] Use the shareable GraphicsContext3DOpenGL* implementation.
Summary: [EFL] Use the shareable GraphicsContext3DOpenGL* implementation.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 97651 97652
  Show dependency treegraph
 
Reported: 2012-09-13 03:00 PDT by Regina Chung
Modified: 2012-10-15 07:54 PDT (History)
9 users (show)

See Also:


Attachments
Patch (76.71 KB, patch)
2012-09-13 22:36 PDT, Regina Chung
no flags Details | Formatted Diff | Diff
Patch (76.72 KB, patch)
2012-09-16 23:34 PDT, Regina Chung
no flags Details | Formatted Diff | Diff
Patch (76.81 KB, patch)
2012-09-28 01:01 PDT, Regina Chung
no flags Details | Formatted Diff | Diff
Patch (78.10 KB, patch)
2012-10-08 04:01 PDT, Regina Chung
no flags Details | Formatted Diff | Diff
Patch (78.09 KB, patch)
2012-10-08 19:14 PDT, Regina Chung
no flags Details | Formatted Diff | Diff
Patch (77.77 KB, patch)
2012-10-09 00:01 PDT, Regina Chung
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Regina Chung 2012-09-13 03:00:53 PDT
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.
Comment 1 Regina Chung 2012-09-13 22:36:47 PDT
Created attachment 164049 [details]
Patch
Comment 2 Simon Hausmann 2012-09-14 23:08:14 PDT
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?
Comment 3 Regina Chung 2012-09-16 23:34:50 PDT
Created attachment 164343 [details]
Patch
Comment 4 Regina Chung 2012-09-16 23:46:17 PDT
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().
Comment 5 Simon Hausmann 2012-09-26 02:54:50 PDT
Kim, do the Evas GL and other EFL bits look okay to you?
Comment 6 Kenneth Rohde Christiansen 2012-09-26 02:56:12 PDT
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!
Comment 7 Kenneth Rohde Christiansen 2012-09-26 03:11:48 PDT
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?
Comment 8 Gyuyoung Kim 2012-09-26 03:30:28 PDT
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);
Comment 9 Regina Chung 2012-09-28 00:45:07 PDT
(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.
Comment 10 Regina Chung 2012-09-28 00:52:51 PDT
(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 :)
Comment 11 Regina Chung 2012-09-28 01:01:27 PDT
Created attachment 166164 [details]
Patch
Comment 12 Simon Hausmann 2012-09-28 03:29:28 PDT
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 :)
Comment 13 Yael 2012-09-28 09:40:33 PDT
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.
Comment 14 Yael 2012-09-28 11:21:51 PDT
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.
Comment 15 Simon Hausmann 2012-10-01 04:02:22 PDT
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?
Comment 16 Yael 2012-10-01 06:43:30 PDT
(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.
Comment 17 Regina Chung 2012-10-03 22:10:26 PDT
(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!
Comment 18 Regina Chung 2012-10-03 22:11:22 PDT
(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.
Comment 19 Regina Chung 2012-10-04 00:11:09 PDT
(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?
Comment 20 Regina Chung 2012-10-04 18:22:12 PDT
(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?
Comment 21 Yael 2012-10-05 19:23:14 PDT
(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.
Comment 22 Regina Chung 2012-10-08 04:01:23 PDT
Created attachment 167522 [details]
Patch
Comment 23 Regina Chung 2012-10-08 04:02:09 PDT
Comment on attachment 167522 [details]
Patch

Patch with suggested changes.
Comment 24 Regina Chung 2012-10-08 04:02:11 PDT
Comment on attachment 167522 [details]
Patch

Patch with suggested changes.
Comment 25 Kenneth Rohde Christiansen 2012-10-08 06:32:17 PDT
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?
Comment 26 Gyuyoung Kim 2012-10-08 17:55:15 PDT
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
Comment 27 Regina Chung 2012-10-08 18:50:57 PDT
(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).
Comment 28 Regina Chung 2012-10-08 19:14:02 PDT
Created attachment 167671 [details]
Patch
Comment 29 Regina Chung 2012-10-08 19:14:45 PDT
(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.
Comment 30 Regina Chung 2012-10-09 00:01:57 PDT
Created attachment 167700 [details]
Patch
Comment 31 Yael 2012-10-10 14:22:46 PDT
(In reply to comment #30)
> Created an attachment (id=167700) [details]
> Patch
This looks like a good step in the right direction.
Comment 32 Simon Hausmann 2012-10-10 14:34:02 PDT
Comment on attachment 167700 [details]
Patch

LGTM - probably a good starting point for further fixes if required. Quite red this patch :)
Comment 33 Kenneth Rohde Christiansen 2012-10-10 14:56:00 PDT
Comment on attachment 167700 [details]
Patch

Let's land this and take it from there
Comment 34 WebKit Review Bot 2012-10-10 15:07:41 PDT
Comment on attachment 167700 [details]
Patch

Clearing flags on attachment: 167700

Committed r130966: <http://trac.webkit.org/changeset/130966>
Comment 35 WebKit Review Bot 2012-10-10 15:07:47 PDT
All reviewed patches have been landed.  Closing bug.
Comment 36 Yael 2012-10-15 07:54:21 PDT
(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.