Bug 79143 – [Texmap] Consolidate the common parts of TextureMapperGL::drawTexture

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Yael]

We should not have duplicate code in those methods.

[Yael]

Created attachment 128288 [details]
Patch.

[Noam Rosenthal]

Comment on attachment 128288 [details]
Patch.

This could work a lot better if we just call something like TextureMapperShaderProgram::prepare that deals with the shader-specific aspects, and then do the drawing in a common drawTexture function.

[Yael]

Created attachment 128368 [details]
Patch.

Second try.

[Noam Rosenthal]

Comment on attachment 128368 [details]
Patch.

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

> Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:214
> +    glUniformMatrix4fv(m_matrixVariable, 1, GL_FALSE, m4);
> +    glUniformMatrix4fv(m_sourceMatrixVariable, 1, GL_FALSE, m4src);
> +    glUniform1i(m_sourceTextureVariable, 0);

Doesn't make sense that this part is here and not in the common function.
Let's have prepare() just receive opacity/mask.

> Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:217
> +    if (maskTexture && maskTexture->isValid()) {

Early return

> Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.h:55
> +    virtual void prepareToDraw(const GLfloat m4[], const GLfloat m4src[], float opacity, const BitmapTexture*) {}

Make it just prepare(opacity, mask)

[Yael]

(In reply to comment #4)
Thanks for your review
> (From update of attachment 128368 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=128368&action=review
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:214
> > +    glUniformMatrix4fv(m_matrixVariable, 1, GL_FALSE, m4);
> > +    glUniformMatrix4fv(m_sourceMatrixVariable, 1, GL_FALSE, m4src);
> > +    glUniform1i(m_sourceTextureVariable, 0);
> 
> Doesn't make sense that this part is here and not in the common function.
> Let's have prepare() just receive opacity/mask.

Doing that will result in either more typecasting in the common function, or making the variables part of the base class again.
Am I missing something?

[Noam Rosenthal]

(In reply to comment #5)
> (In reply to comment #4)
> Thanks for your review
> > (From update of attachment 128368 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=128368&action=review
> > 
> > > Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:214
> > > +    glUniformMatrix4fv(m_matrixVariable, 1, GL_FALSE, m4);
> > > +    glUniformMatrix4fv(m_sourceMatrixVariable, 1, GL_FALSE, m4src);
> > > +    glUniform1i(m_sourceTextureVariable, 0);
> > 
> > Doesn't make sense that this part is here and not in the common function.
> > Let's have prepare() just receive opacity/mask.
> 
> Doing that will result in either more typecasting in the common function, or making the variables part of the base class again.
> Am I missing something?

They should be part of the base class.

[Yael]

Created attachment 128578 [details]
Third try

[Noam Rosenthal]

Comment on attachment 128578 [details]
Third try

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

Almost LGTM :) one fix and we're done.

> Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:202
> +void TextureMapperShaderProgramOpacityAndMask::prepare(const BitmapTexture* maskTexture)

I want opacity to be part of the parameters as well; it's ok if it's duplicated.
That's because we might want to add another shader that does not use opacity.

[Yael]

Created attachment 128624 [details]
Patch.

Merge and address comment #8.

[WebKit Commit Bot]

Attachment 128624 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.h:55:  Missing space inside { }.  [whitespace/braces] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Noam Rosenthal]

Comment on attachment 128624 [details]
Patch.

LGTM
Please fix minor style issue.

[Yael]

Created attachment 128736 [details]
Patch for landing.

[WebKit Review Bot]

Comment on attachment 128736 [details]
Patch for landing.

Clearing flags on attachment: 128736

Committed r108810: <http://trac.webkit.org/changeset/108810>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Rafael Brandao]

Comment on attachment 128736 [details]
Patch for landing.

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

> Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.h:56
> +    GLint matrixVariable() { return m_matrixVariable; }

Would it make any difference to use "GLint matrixVariable() const { ... }" instead? It may be more code style friendly.

[Noam Rosenthal]

(In reply to comment #15)
> (From update of attachment 128736 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=128736&action=review
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.h:56
> > +    GLint matrixVariable() { return m_matrixVariable; }
> 
> Would it make any difference to use "GLint matrixVariable() const { ... }" instead? It may be more code style friendly.

I agree, my oversight.
Yael, can you clean up some of the constness in the new classes?

[Yael]

https://bugs.webkit.org/show_bug.cgi?id=79545