Bug 78674 – [Texmap] Better management of shaders in TextureMapperGL

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Noam Rosenthal]

Currently GL creates all the shaders right when it starts; Instead, it should generate the appropriate shader when it's requested, allowing for new shaders to be added efficiently, such as the shaders needed for CSS filters.

[Yael]

Created attachment 127673 [details]
Patch

Split TextureMapperGL.cpp to 2 files.
Shader programs are created on demand instead of on startup.

This patch has one bogus style error, please ignore it :)

[Noam Rosenthal]

Comment on attachment 127673 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127673&action=review

Great progress! Still needs work.

> Source/WebCore/GNUmakefile.list.am:5778
> +        Source/WebCore/platform/graphics/opengl/TextureMapperShaderManager.cpp \
> +        Source/WebCore/platform/graphics/opengl/TextureMapperShaderManager.h \
> +        Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp \

I think they use tabs in this file and not spaces.

> Source/WebCore/platform/graphics/opengl/TextureMapperShaderManager.cpp:163
> +#if !defined(TEXMAP_OPENGL_ES_2) && !PLATFORM(QT) && !PLATFORM(GTK)
> +extern "C" {
> +    void glUniform1f(GLint, GLfloat);
> +    void glUniform1i(GLint, GLint);
> +    void glVertexAttribPointer(GLuint, GLint, GLenum, GLboolean, GLsizei, const GLvoid*);
> +    void glUniform4f(GLint, GLfloat, GLfloat, GLfloat, GLfloat);
> +    void glShaderSource(GLuint, GLsizei, const char**, const GLint*);
> +    GLuint glCreateShader(GLenum);
> +    void glShaderSource(GLuint, GLsizei, const char**, const GLint*);
> +    void glCompileShader(GLuint);
> +    void glDeleteShader(GLuint);
> +    void glUniformMatrix4fv(GLint, GLsizei, GLboolean, const GLfloat*);
> +    GLuint glCreateProgram();
> +    void glAttachShader(GLuint, GLuint);
> +    void glLinkProgram(GLuint);
> +    void glUseProgram(GLuint);
> +    void glDisableVertexAttribArray(GLuint);
> +    void glEnableVertexAttribArray(GLuint);
> +    void glBindFramebuffer(GLenum target, GLuint framebuffer);
> +    void glDeleteFramebuffers(GLsizei n, const GLuint* framebuffers);
> +    void glGenFramebuffers(GLsizei n, GLuint* framebuffers);
> +    void glFramebufferTexture2D(GLenum target, GLenum attachment, GLenum textarget, GLuint texture, GLint level);
> +    void glGetFramebufferAttachmentParameteriv(GLenum target, GLenum attachment, GLenum pname, GLint* params);
> +    void glBindBuffer(GLenum, GLuint);
> +    void glDeleteBuffers(GLsizei, const GLuint*);
> +    void glGenBuffers(GLsizei, GLuint*);
> +    void glBufferData(GLenum, GLsizeiptr, const GLvoid*, GLenum);
> +    void glBufferSubData(GLenum, GLsizeiptr, GLsizeiptr, const GLvoid*);
> +    void glGetProgramInfoLog(GLuint, GLsizei, GLsizei*, GLchar*);
> +    void glGetShaderInfoLog(GLuint, GLsizei, GLsizei*, GLchar*);
> +    void glGenRenderbuffers(GLsizei n, GLuint* ids);
> +    void glDeleteRenderbuffers(GLsizei n, const GLuint* ids);
> +    void glBindRenderbuffer(GLenum target, GLuint id);
> +    void glRenderbufferStorage(GLenum target, GLenum internalFormat, GLsizei width, GLsizei height);
> +    void glFramebufferRenderbuffer(GLenum target, GLenum attachmentPoint, GLenum renderbufferTarget, GLuint renderbufferId);
> +    GLenum glCheckFramebufferStatus(GLenum target);
> +    GLint glGetAttribLocation(GLuint program, const GLchar* name);
> +#if !OS(MAC_OS_X)
> +    GLint glGetUniformLocation(GLuint, const GLchar*);
> +    GLint glBindAttribLocation(GLuint, GLuint, const GLchar*);
> +#endif
> +}
> +#endif

I think we don't use most of these in the shader-manager. Let's just keep the ones we use (though this only applies to the EFL port I think).

> Source/WebCore/platform/graphics/opengl/TextureMapperShaderManager.h:83
> +    virtual const char** vertexShaderSource() = 0;
> +    virtual const char** fragmentShaderSource() = 0;

Why not just return a const char*, and then pass a reference to the pointer?
const char* vSource = vertexShaderSource();
glCompileShader(shader, 1, &vSource, &length);

> Source/WebCore/platform/graphics/opengl/TextureMapperShaderManager.h:95
> +    GLint inMatrixVariable() { return m_inMatrixVariable; }
> +    GLint inSourceMatrixVariable() { return m_inSourceMatrixVariable; }

Let's get rid of the word "in" in the public getters; This is valid for the other classes as well of course.

> Source/WebCore/platform/graphics/opengl/TextureMapperGL.h:45
>      virtual void drawTexture(uint32_t texture, bool opaque, const FloatSize&, const FloatRect&, const TransformationMatrix&, float opacity, const BitmapTexture* maskTexture, bool flip);
> +    virtual void drawTextureWithMaskAndOpacity(uint32_t texture, bool opaque, const FloatSize&, const FloatRect&, const TransformationMatrix&, float opacity, const BitmapTexture* maskTexture, bool flip);
> +    virtual void drawTextureSimple(uint32_t texture, bool opaque, const FloatSize&, const FloatRect&, const TransformationMatrix&, float opacity, const BitmapTexture* maskTexture, bool flip);

While we're at it, let's change the booleans opaque & flip to be enum-based flags.

[Yael]

Created attachment 127848 [details]
Patch

Address No'am 's comments.

[Noam Rosenthal]

Comment on attachment 127848 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127848&action=review

> Source/WebCore/ChangeLog:8
> +        Split ShaderMapperGL.cpp into two files.

You mean TextureMapperGL?

> Source/WebCore/platform/graphics/opengl/TextureMapperGL.h:49
> +    enum TextureTransparency {
> +        Opaque,
> +        Transparent
> +    };
> +
> +    enum Flip {
> +        ShouldFlip,
> +        NoFlip
> +    };

Just make this into one enum called Flag with
0x01 = SupportsBlending,
0x02 = ShouldFlipTexture

and then pass it as an integer mask.

> Source/WebCore/platform/graphics/opengl/TextureMapperShaderManager.cpp:23
> +#if USE(ACCELERATED_COMPOSITING)

&& USE(TEXTURE_MAPPER)

> Source/WebCore/platform/graphics/opengl/TextureMapperShaderManager.cpp:53
> +const char* fragmentShaderSourceOpacityAndMask =

These should all be declared static, otherwise they take up symbols.

> Source/WebCore/platform/graphics/opengl/TextureMapperShaderManager.h:54
> +inline static void debugGLCommand(const char* command, int line)
> +{
> +    const GLenum err = glGetError();
> +    if (!err)
> +        return;
> +    WTFReportError(__FILE__, line, WTF_PRETTY_FUNCTION, "[TextureMapper GL] Command failed: %s (%x)\n", command, err);
> +    ASSERT_NOT_REACHED();
> +}
> +

I don't like this here. It should be inside TextureMapperGL.cpp, and we should simply not use GL_CMD from inside TextureMapperShaderManager.

> Source/WebCore/platform/graphics/texmap/TextureMapper.h:82
> +    inline bool isOpaque() const { return m_flags ^ SupportsAlpha; }

^ ??

[Martin Robinson]

Comment on attachment 127848 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127848&action=review

This looks like a nice change to me. I think that all the ports that use TextureMapper now use OpenGLShims, so it might be a good idea to just ditch those #ifdefs and dead code.

> Source/WebCore/platform/graphics/opengl/TextureMapperShaderManager.cpp:35
> +#if PLATFORM(QT) || USE(CAIRO)
> +#include "OpenGLShims.h"
> +#elif defined(TEXMAP_OPENGL_ES_2)
> +#include <GLES2/gl2.h>
> +#include <GLES2/gl2ext.h>
> +#elif OS(MAC_OS_X)
> +#include <gl.h>
> +#else
> +#include <GL/gl.h>
> +#endif
> +

Aren't QT and Cairo are the only platforms using TextureMapper? Odds are that if the EFL port starts using htis they are going to want the OpenGLShims as well (since they use them for WebGL).

> Source/WebCore/platform/graphics/opengl/TextureMapperShaderManager.cpp:134
> +#if !defined(TEXMAP_OPENGL_ES_2) && !PLATFORM(QT) && !PLATFORM(GTK)
> +extern "C" {
> +    void glShaderSource(GLuint, GLsizei, const char**, const GLint*);
> +    void glCompileShader(GLuint);
> +    void glDeleteShader(GLuint);
> +    GLuint glCreateProgram();
> +    void glAttachShader(GLuint, GLuint);
> +    void glLinkProgram(GLuint);
> +    GLint glGetAttribLocation(GLuint program, const GLchar* name);
> +#if !OS(MAC_OS_X)
> +    GLint glGetUniformLocation(GLuint, const GLchar*);
> +#endif
> +}
> +#endif

Similar comment here. If neither Qt nor GTK+ is using this, it seems like dead code. Perhaps I'm missing something here?

> Source/WebCore/platform/graphics/opengl/TextureMapperShaderManager.h:42
> +#if PLATFORM(QT) || USE(CAIRO)
> +#include "OpenGLShims.h"
> +#elif defined(TEXMAP_OPENGL_ES_2)
> +#include <GLES2/gl2.h>
> +#include <GLES2/gl2ext.h>
> +#elif OS(MAC_OS_X)
> +#include <gl.h>
> +#else
> +#include <GL/gl.h>
> +#endif

Ditto.

> Source/WebCore/platform/graphics/opengl/TextureMapperShaderManager.h:80
> +    void getUniformLocation(GLint& var, const char *name);

Your asterisk is in the wrong place here. I'm suprrised the style bot did not catch this!

[Yael]

Created attachment 127991 [details]
Patch.

Next round of review.
Addressed comments #4 and #5.

[Martin Robinson]

Comment on attachment 127991 [details]
Patch.

The changes look good to me. :)

[Noam Rosenthal]

Comment on attachment 127991 [details]
Patch.

Looks good!
We might want to move the different paint functions into the shader-manager later on, but it can wait to a different patch.
Leaving the cq flag at ?, you can commit when you can track the bots etc.

[Yael]

Comment on attachment 127991 [details]
Patch.

Thanks for the review :)

[WebKit Review Bot]

Comment on attachment 127991 [details]
Patch.

Clearing flags on attachment: 127991

Committed r108402: <http://trac.webkit.org/changeset/108402>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.