Bug 78674 - [Texmap] Better management of shaders in TextureMapperGL
Summary: [Texmap] Better management of shaders in TextureMapperGL
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yael
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-15 00:41 PST by Noam Rosenthal
Modified: 2012-02-21 14:44 PST (History)
4 users (show)

See Also:


Attachments
Patch (44.82 KB, patch)
2012-02-17 16:23 PST, Yael
noam: review-
noam: commit-queue-
Details | Formatted Diff | Diff
Patch (47.09 KB, patch)
2012-02-20 14:09 PST, Yael
noam: review-
noam: commit-queue-
Details | Formatted Diff | Diff
Patch. (44.20 KB, patch)
2012-02-21 10:01 PST, Yael
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Noam Rosenthal 2012-02-15 00:41:07 PST
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.
Comment 1 Yael 2012-02-17 16:23:58 PST
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 :)
Comment 2 Noam Rosenthal 2012-02-17 20:42:31 PST
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.
Comment 3 Yael 2012-02-20 14:09:28 PST
Created attachment 127848 [details]
Patch

Address No'am 's comments.
Comment 4 Noam Rosenthal 2012-02-20 14:56:35 PST
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; }

^ ??
Comment 5 Martin Robinson 2012-02-20 15:22:12 PST
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!
Comment 6 Yael 2012-02-21 10:01:14 PST
Created attachment 127991 [details]
Patch.

Next round of review.
Addressed comments #4 and #5.
Comment 7 Martin Robinson 2012-02-21 10:51:53 PST
Comment on attachment 127991 [details]
Patch.

The changes look good to me. :)
Comment 8 Noam Rosenthal 2012-02-21 10:57:00 PST
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.
Comment 9 Yael 2012-02-21 11:03:36 PST
Comment on attachment 127991 [details]
Patch.

Thanks for the review :)
Comment 10 WebKit Review Bot 2012-02-21 14:44:44 PST
Comment on attachment 127991 [details]
Patch.

Clearing flags on attachment: 127991

Committed r108402: <http://trac.webkit.org/changeset/108402>
Comment 11 WebKit Review Bot 2012-02-21 14:44:50 PST
All reviewed patches have been landed.  Closing bug.