[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 #9 from Regina Chung <heejin.r.chung at samsung.com>  2012-09-28 00:45:07 PST ---
(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.

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