Bug 86048 – [Texmap] Refactor TextureMapperShaderManager to be clearer

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Noam Rosenthal]

[Texmap] Refactor TextureMapperShaderManager to be clearer

[Noam Rosenthal]

Created attachment 141068 [details]
Patch

[Early Warning System Bot]

Comment on attachment 141068 [details]
Patch

Attachment 141068 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12664193

[Early Warning System Bot]

Comment on attachment 141068 [details]
Patch

Attachment 141068 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12649898

[Igor Trindade Oliveira]

Would be interesting update the GTK build system.

[Noam Rosenthal]

Comment on attachment 141068 [details]
Patch

Clearing flag, some file is missing...

[Noam Rosenthal]

Created attachment 141180 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 141180 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:-54
> -#define GL_CMD(...) do { __VA_ARGS__; ASSERT_ARG(__VA_ARGS__, !glGetError()); } while (0)

Nothing important, but you shouldn't need to move that line inside the namespace.

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:617
> +        glActiveTexture(GL_TEXTURE1);
> +        glBindTexture(GL_TEXTURE_2D, maskTextureGL->id());
> +        glUniform1i(shader->getUniformLocation(MaskedShaderDefinition::Mask), 1);
> +        glActiveTexture(GL_TEXTURE0);

GL_CMD(...)?

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:767
> +

Unneeded extra line.

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:794
> +    if (filter.getOperationType() < FilterOperation::GRAYSCALE || filter.getOperationType() > FilterOperation::CONTRAST) {

Please don't use < or > on enums, use a isColorizeFilter or something function returning a bool with a switch instead.

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:802
> +    GLuint vertexAttrib = shader->getAttributeLocation(ColorizeShaderDefinition::Position);
> +    GLuint texCoordAttrib = shader->getAttributeLocation(ColorizeShaderDefinition::TexCoord);

I'm not sure I understand the move of flattening all the shader logic in this method instead of something like having them inherit TextureMapperShader like before and let them initialize/cleanup their uniforms from parameters given to the object. Can you say what you mean by "tightly integrated"?
Isn't this going to become messy as we add more implemented shaders?

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:809
> +    glUniform1i(shader->getUniformLocation(ColorizeShaderDefinition::Texture), 0);
> +    glUniform1i(shader->getUniformLocation(ColorizeShaderDefinition::FilterType), filter.getOperationType());
> +    glUniform1f(shader->getUniformLocation(ColorizeShaderDefinition::FilterAmount), extractFilterAmount(filter));

GL_CMD(...)?

> Source/WebCore/platform/graphics/texmap/TextureMapperShader.cpp:26
> +#include "stdio.h"

<stdio.h> instead of "stdio.h (even though I'm not sure of what it concretely changes)

> Source/WebCore/platform/graphics/texmap/TextureMapperShader.cpp:48
> +    if (len)
> +        printf("Vertex shader log: %s\n", log);

Does this prints anything other than errors?
If yes please protect it by something like TEXTUREMAPPER_SHADER_LOG instead of NDEBUG with a define at the top, commented by default.

[Noam Rosenthal]

Comment on attachment 141180 [details]
Patch

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

>> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:802
>> +    GLuint texCoordAttrib = shader->getAttributeLocation(ColorizeShaderDefinition::TexCoord);
> 
> I'm not sure I understand the move of flattening all the shader logic in this method instead of something like having them inherit TextureMapperShader like before and let them initialize/cleanup their uniforms from parameters given to the object. Can you say what you mean by "tightly integrated"?
> Isn't this going to become messy as we add more implemented shaders?

It was becoming really messy to do filters in the old way... The problem was that for every addition in the shader's uniform etc. we had to add support in the different draw functions, and then add inappropriate abstractions like prepare().
The problem is that you can't really separate a shader and the command that draws it.

>> Source/WebCore/platform/graphics/texmap/TextureMapperShader.cpp:48
>> +        printf("Vertex shader log: %s\n", log);
> 
> Does this prints anything other than errors?
> If yes please protect it by something like TEXTUREMAPPER_SHADER_LOG instead of NDEBUG with a define at the top, commented by default.

This only prints errors.

[Jocelyn Turcotte]

(In reply to comment #8)
> It was becoming really messy to do filters in the old way... The problem was that for every addition in the shader's uniform etc. we had to add support in the different draw functions, and then add inappropriate abstractions like prepare().
> The problem is that you can't really separate a shader and the command that draws it.

Ah ok.
How about something like letting shaders draw() themselves using the given source textures?
Else I guess we can add drawFiltered, drawColorized, drawBlured, etc. in the future.

[Noam Rosenthal]

Comment on attachment 141180 [details]
Patch

Clearning flags - I need to rethink this a bit... I'd hate to rewrite this abstraction yet again, so I'd rather come up with the right approach now.

[Noam Rosenthal]

Created attachment 167614 [details]
Patch

Trying to remove some of the TextureMapperShaderManager broiler-plate...

[Noam Rosenthal]

Created attachment 167615 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 167615 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:732
> +    default:
> +        return TextureMapperShaderManager::Invalid;
> +    }

ASSERT?

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:780
> +    // Normalize the kernel

nit

> Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:33
> +TextureMapperShaderProgram::TextureMapperShaderProgram(PassRefPtr<GraphicsContext3D> context, const String& vertex, const String& fragment)

vertexSource? fragmentSource?

> Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:49
> +    m_context->linkProgram(m_id);
> +    if (m_context->getError() == GraphicsContext3D::NO_ERROR)

newline before if , I would say, it is already hard to read the above code with no blocking

> Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:53
> +    String log = m_context->getShaderInfoLog(m_vertexShader);
> +    printf("Vertex shader log: %s\n", log.utf8().data());

maybe in debug only?

> Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:99
> +        : vertexShader(vertex ? String(ASCIILiteral(vertex)) : String())

Doesn't string have a constructor taking null? I guess CString does.

[Martin Robinson]

Comment on attachment 167615 [details]
Patch

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

Just a couple nits after a quick look:

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:758
> +#define GAUSSIAN_KERNEL_HALF_WIDTH 11
> +#define GAUSSIAN_KERNEL_STEP 0.2

These should be static const int and float.

> Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:62
> +    HashMap<const char*, GC3Duint>::iterator it = m_variables.find(name);

Perhaps this HashMap should hold AtomicString rather than const char*?

[Noam Rosenthal]

Created attachment 167641 [details]
Patch

[Noam Rosenthal]

Created attachment 167645 [details]
Patch

[Noam Rosenthal]

Created attachment 167648 [details]
Patch

[WebKit Review Bot]

Attachment 167648 [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:30:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:70:  Missing space before ( in switch(  [whitespace/parens] [5]
Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:274:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:275:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:279:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:759:  GAUSSIAN_KERNEL_HALF_WIDTH is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:760:  GAUSSIAN_KERNEL_STEP is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 7 in 4 files


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

[Early Warning System Bot]

Comment on attachment 167648 [details]
Patch

Attachment 167648 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14229319

[Early Warning System Bot]

Comment on attachment 167648 [details]
Patch

Attachment 167648 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14205849

[Noam Rosenthal]

Created attachment 167666 [details]
Patch

[Early Warning System Bot]

Comment on attachment 167666 [details]
Patch

Attachment 167666 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14221482

[Early Warning System Bot]

Comment on attachment 167666 [details]
Patch

Attachment 167666 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14238095

[Noam Rosenthal]

Created attachment 167669 [details]
Patch

[Early Warning System Bot]

Comment on attachment 167669 [details]
Patch

Attachment 167669 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14228447

[Early Warning System Bot]

Comment on attachment 167669 [details]
Patch

Attachment 167669 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14214553

[Noam Rosenthal]

Created attachment 167672 [details]
Patch

[Martin Robinson]

Comment on attachment 167672 [details]
Patch

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

Nice, I just have a few small suggestions.

> Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:56
> +    String log = m_context->getShaderInfoLog(m_vertexShader);
> +    printf("Vertex shader log: %s\n", log.utf8().data());
> +    log = m_context->getShaderInfoLog(m_fragmentShader);
> +    printf("Fragment shader log: %s\n", log.utf8().data());
> +    log = m_context->getProgramInfoLog(m_id);
> +    printf("Program log: %s\n", log.utf8().data());

It probably makes sense to use the WebCore log facilities here.

> Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:109
> +#define STRINGIFY(...) #__VA_ARGS__

This is a nit, but it would look a little bit cleaner if this was at the top of the file.

[Noam Rosenthal]

Created attachment 168989 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 168989 [details]
Patch for landing

Clearing flags on attachment: 168989

Committed r131485: <http://trac.webkit.org/changeset/131485>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #30)
> (From update of attachment 168989 [details])
> Clearing flags on attachment: 168989
> 
> Committed r131485: <http://trac.webkit.org/changeset/131485>

It broke the build - https://bugs.webkit.org/show_bug.cgi?id=99499
Could you fix it please? (Sorry, but I have better todos than fixing
build issues before midnight.)