[Texmap] Refactor TextureMapperShaderManager to be clearer
Created attachment 141068 [details] Patch
Comment on attachment 141068 [details] Patch Attachment 141068 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12664193
Comment on attachment 141068 [details] Patch Attachment 141068 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12649898
Would be interesting update the GTK build system.
Comment on attachment 141068 [details] Patch Clearing flag, some file is missing...
Created attachment 141180 [details] Patch
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.
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.
(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.
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.
Created attachment 167614 [details] Patch Trying to remove some of the TextureMapperShaderManager broiler-plate...
Created attachment 167615 [details] Patch
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.
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*?
Created attachment 167641 [details] Patch
Created attachment 167645 [details] Patch
Created attachment 167648 [details] Patch
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.
Comment on attachment 167648 [details] Patch Attachment 167648 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14229319
Comment on attachment 167648 [details] Patch Attachment 167648 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14205849
Created attachment 167666 [details] Patch
Comment on attachment 167666 [details] Patch Attachment 167666 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14221482
Comment on attachment 167666 [details] Patch Attachment 167666 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14238095
Created attachment 167669 [details] Patch
Comment on attachment 167669 [details] Patch Attachment 167669 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14228447
Comment on attachment 167669 [details] Patch Attachment 167669 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14214553
Created attachment 167672 [details] Patch
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.
Created attachment 168989 [details] Patch for landing
Comment on attachment 168989 [details] Patch for landing Clearing flags on attachment: 168989 Committed r131485: <http://trac.webkit.org/changeset/131485>
All reviewed patches have been landed. Closing bug.
(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.)