Bug 106508

Created attachment 182022 [details] WIP

Initial Approach: what the patch does is basically check for the availability of the blit extension. If the extension is unavailable it falls back to use shaders.

It seems the WIP patch is mostly based on what GraphicsSurfaceWin does. Do you think it would be possible to adopt this code instead of duplicating it? If it is just about naming, we could probably rename it to GraphicsSurfaceEGL or similar. ;)

(In reply to comment #3) > It seems the WIP patch is mostly based on what GraphicsSurfaceWin does. Do you think it would be possible to adopt this code instead of duplicating it? > If it is just about naming, we could probably rename it to GraphicsSurfaceEGL or similar. ;) It does do something similar to what is done in GraphicsSurfaceWin. I would be more than happy to get the common elements (copytexture parts out) and create something common to be used by all surface implementations. I can get rid of the Angle part from this patch. some differences: 1)This patch tries to make use of OES_vertex_array_object if supported. 2)Take Sampler objects into use(if supported). 3)Save and restore previous VBO state, program etc.

Created attachment 182603 [details] WIP

Created attachment 185304 [details] WIP

Created attachment 187316 [details] WIP Trying to use TextureMapperGL to achieve the same. Doesn't work yet

(In reply to comment #7) > Created an attachment (id=187316) [details] > WIP > > Trying to use TextureMapperGL to achieve the same. Doesn't work yet k, having it working now. Issue with TextureMapperGL is that it doesn't keep track of vertex array object. This was k as on it was mostly used in UI process. With WebGL apps this is needed, we could do this in our side so it doesn't add any overhead for other use cases.

Created attachment 187435 [details] patch

Made changes to use TextureMapperGL to draw texture content to surface.

Created attachment 187475 [details] patchv2

Created attachment 187480 [details] patchv3 minor cleanup.

Attachment 187480 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp', u'Source/WebCore/platform/graphics/opengl/GLPlatformSurface.cpp', u'Source/WebCore/platform/graphics/opengl/GLPlatformSurface.h']" exit_code: 1 Source/WebCore/platform/graphics/opengl/GLPlatformSurface.cpp:72: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 187483 [details] patchv4 style fix

Comment on attachment 187483 [details] patchv4 View in context: https://bugs.webkit.org/attachment.cgi?id=187483&action=review > Source/WebCore/platform/graphics/opengl/GLPlatformSurface.cpp:167 > + // Save any current bound vertex array to restore after updating surface with texture content. > + // This is needed as TextureMapperGL doesn't track and restore current bound vertex array. > + ScopedStateSaver stateSaver; > + ::glBindFramebuffer(GL_FRAMEBUFFER, 0); > + ::glColorMask(true, true, true, true); > + m_textureBlitter->beginPainting(); > + m_textureBlitter->drawTexture(texture, m_flags, textureSize, m_rect, m_identityMatrix, 1.0, 0); > + m_textureBlitter->endPainting(); This looks flaky. TextureMapperGL may change other attributes in the future and this would break. Why not use TextureMapperShaderProgram directly? That class has a more deterministic API than TMGL. > Source/WebCore/platform/graphics/opengl/GLPlatformSurface.cpp:169 > + ::glFlush(); You probably don't want to flush with every frame...

(In reply to comment #15) > (From update of attachment 187483 [details]) > TextureMapperGL may change other attributes in the future and this would break. > Why not use TextureMapperShaderProgram directly? That class has a more deterministic API than TMGL. Good point. Will try to do that. > > Source/WebCore/platform/graphics/opengl/GLPlatformSurface.cpp:169 > > + ::glFlush(); > > You probably don't want to flush with every frame... It is needed for certain backends. (I.e using EGL and EGLImage). I do agree, this might not be the right place for it as on GLX you don't need to do this. I will get rid of it.

Created attachment 187728 [details] patchv5 Removes dependency on TextureMapperGL

Comment on attachment 187728 [details] patchv5 View in context: https://bugs.webkit.org/attachment.cgi?id=187728&action=review > Source/WebCore/platform/graphics/opengl/GLPlatformSurface.cpp:73 > +class GLStateTracker { I am somewhat skeptical to the concept of tracking gl states. How do you make sure that you track all the necessary states? Or in other words, how do you know that there is nothing else you have to save and restore? I know we have different concepts here between EFL and Qt. So just to make it clear, I am not objecting... just raising a concern, that might need to be adressed in future. > Source/WebCore/platform/graphics/opengl/GLPlatformSurface.cpp:234 > + else I would remove the "else" here. It seems not logical to have a conditional push() but a non-conditional pop(). But i see that it would not matter in case m_shaderProgram has not been initialized before.

(In reply to comment #18) > (From update of attachment 187728 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=187728&action=review > I know we have different concepts here between EFL and Qt. So just to make it clear, I am not objecting... just raising a concern, that might need to be adressed in future. We only track things which we manipulate on our side(i.e disable blend, depth, vbo etc) and not everything. I do agree that it might not be needed with a shared context in place. Support for that is not yet there, we can get rid of state tracking once we have it in place.

Comment on attachment 187728 [details] patchv5 Removing request for review till 109988 is done.