[webkit-reviews] review denied: [Bug 78674] [Texmap] Better management of shaders in TextureMapperGL : [Attachment 127673] Patch

[bugzilla-daemon at webkit.org]

Noam Rosenthal <noam.rosenthal at nokia.com> has denied Yael
<yael.aharon at nokia.com>'s request for review:
Bug 78674: [Texmap] Better management of shaders in TextureMapperGL
https://bugs.webkit.org/show_bug.cgi?id=78674

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

------- Additional Comments from Noam Rosenthal <noam.rosenthal at nokia.com>
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.