Bug 106508 – [EFL] [WebGL] Add support for bliting texture contents to FBO.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Kalyan]

We currently rely on EXT_framebuffer_blit to copy contents from one FBO to another. This extension is not exposed on GLES1.1 or 2.0. ANGLE_framebuffer_blit seems to provide similar interactions. I am not sure how well it is supported.

Does Angle included as part of WebKit include support for the extension ??
 
One possibility is to use this extension when available and provide a fall back mechanism.

[Kalyan]

Created attachment 182022 [details]
WIP

[Kalyan]

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.

[Zeno Albisser]

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

[Kalyan]

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

[Kalyan]

Created attachment 182603 [details]
WIP

[Kalyan]

Created attachment 185304 [details]
WIP

[Kalyan]

Created attachment 187316 [details]
WIP

Trying to use TextureMapperGL to achieve the same. Doesn't work yet

[Kalyan]

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

[Kalyan]

Created attachment 187435 [details]
patch

[Kalyan]

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

[Kalyan]

Created attachment 187475 [details]
patchv2

[Kalyan]

Created attachment 187480 [details]
patchv3

minor cleanup.

[WebKit Review Bot]

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.

[Kalyan]

Created attachment 187483 [details]
patchv4

style fix

[Noam Rosenthal]

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

[Kalyan]

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

[Kalyan]

Created attachment 187728 [details]
patchv5

Removes dependency on TextureMapperGL

[Zeno Albisser]

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.

[Kalyan]

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

[Kalyan]

Comment on attachment 187728 [details]
patchv5

Removing request for review till 109988 is done.