[webkit-reviews] review denied: [Bug 106508] [EFL] [WebGL] Add support for bliting texture contents to FBO. : [Attachment 187483] patchv4

[bugzilla-daemon at webkit.org]

Noam Rosenthal <noam at webkit.org> has denied Kalyan
<kalyan.kondapally at intel.com>'s request for review:
Bug 106508: [EFL] [WebGL] Add support for bliting texture contents to FBO.
https://bugs.webkit.org/show_bug.cgi?id=106508

Attachment 187483: patchv4
https://bugs.webkit.org/attachment.cgi?id=187483&action=review

------- Additional Comments from Noam Rosenthal <noam at webkit.org>
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...