Summary: | [EFL] Use the shareable GraphicsContext3DOpenGL* implementation. | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Regina Chung <heejin.r.chung> | ||||||||||||||
Component: | WebKit EFL | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | dino, gyuyoung.kim, hausmann, joone, laszlo.gombos, lucas.de.marchi, rakuco, webkit.review.bot, yael | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Other | ||||||||||||||||
OS: | Linux | ||||||||||||||||
Bug Depends on: | |||||||||||||||||
Bug Blocks: | 97651, 97652 | ||||||||||||||||
Attachments: |
|
Description
Regina Chung
2012-09-13 03:00:53 PDT
Created attachment 164049 [details]
Patch
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? Created attachment 164343 [details]
Patch
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(). Kim, do the Evas GL and other EFL bits look okay to you? 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 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 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); (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. (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 :) Created attachment 166164 [details]
Patch
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 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 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 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? (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. (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! (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. (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? (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? (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. Created attachment 167522 [details]
Patch
Comment on attachment 167522 [details]
Patch
Patch with suggested changes.
Comment on attachment 167522 [details]
Patch
Patch with suggested changes.
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 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 (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). Created attachment 167671 [details]
Patch
(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. Created attachment 167700 [details]
Patch
(In reply to comment #30) > Created an attachment (id=167700) [details] > Patch This looks like a good step in the right direction. Comment on attachment 167700 [details]
Patch
LGTM - probably a good starting point for further fixes if required. Quite red this patch :)
Comment on attachment 167700 [details]
Patch
Let's land this and take it from there
Comment on attachment 167700 [details] Patch Clearing flags on attachment: 167700 Committed r130966: <http://trac.webkit.org/changeset/130966> All reviewed patches have been landed. Closing bug. (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. |