Bug 79143

Summary: [Texmap] Consolidate the common parts of TextureMapperGL::drawTexture
Product: WebKit Reporter: Yael <yael>
Component: Layout and RenderingAssignee: Yael <yael>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, mrobinson, noam, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch.
noam: review-
Patch.
noam: review-
Third try
none
Patch.
noam: review+
Patch for landing. none

Description Yael 2012-02-21 12:47:39 PST
We should not have duplicate code in those methods.
Comment 1 Yael 2012-02-22 14:14:19 PST
Created attachment 128288 [details]
Patch.
Comment 2 Noam Rosenthal 2012-02-22 14:46:42 PST
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.
Comment 3 Yael 2012-02-22 19:05:37 PST
Created attachment 128368 [details]
Patch.

Second try.
Comment 4 Noam Rosenthal 2012-02-23 10:12:57 PST
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)
Comment 5 Yael 2012-02-23 10:24:16 PST
(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?
Comment 6 Noam Rosenthal 2012-02-23 11:39:26 PST
(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.
Comment 7 Yael 2012-02-23 15:53:39 PST
Created attachment 128578 [details]
Third try
Comment 8 Noam Rosenthal 2012-02-23 16:58:28 PST
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.
Comment 9 Yael 2012-02-23 18:16:08 PST
Created attachment 128624 [details]
Patch.

Merge and address comment #8.
Comment 10 WebKit Commit Bot 2012-02-24 02:20:23 PST
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.
Comment 11 Noam Rosenthal 2012-02-24 06:38:10 PST
Comment on attachment 128624 [details]
Patch.

LGTM
Please fix minor style issue.
Comment 12 Yael 2012-02-24 07:54:51 PST
Created attachment 128736 [details]
Patch for landing.
Comment 13 WebKit Review Bot 2012-02-24 09:03:04 PST
Comment on attachment 128736 [details]
Patch for landing.

Clearing flags on attachment: 128736

Committed r108810: <http://trac.webkit.org/changeset/108810>
Comment 14 WebKit Review Bot 2012-02-24 09:03:13 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 Rafael Brandao 2012-02-24 10:03:24 PST
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.
Comment 16 Noam Rosenthal 2012-02-24 13:13:38 PST
(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?