RESOLVED FIXED 75778
[Texmap] Enable css filters in TextureMapperGL
https://bugs.webkit.org/show_bug.cgi?id=75778
Summary [Texmap] Enable css filters in TextureMapperGL
Noam Rosenthal
Reported 2012-01-07 09:37:19 PST
Hardware-accelerate CSS filters using shaders in TextureMapperGL. This, for now, does not include CSS shaders.
Attachments
Patch (40.19 KB, patch)
2012-04-28 10:22 PDT, Noam Rosenthal
no flags
Patch (40.19 KB, patch)
2012-04-28 10:49 PDT, Noam Rosenthal
no flags
Patch (40.13 KB, patch)
2012-05-02 06:09 PDT, Noam Rosenthal
no flags
Patch for landing (40.08 KB, patch)
2012-05-02 06:56 PDT, Noam Rosenthal
webkit.review.bot: commit-queue-
Patch for Landing (38.78 KB, patch)
2012-05-02 07:23 PDT, Noam Rosenthal
no flags
Patch for landing (1.37 KB, patch)
2012-05-02 10:35 PDT, Noam Rosenthal
no flags
Noam Rosenthal
Comment 1 2012-04-28 10:22:59 PDT
Early Warning System Bot
Comment 2 2012-04-28 10:42:15 PDT
Early Warning System Bot
Comment 3 2012-04-28 10:44:30 PDT
Noam Rosenthal
Comment 4 2012-04-28 10:49:14 PDT
Jocelyn Turcotte
Comment 5 2012-05-02 05:37:03 PDT
Comment on attachment 139365 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=139365&action=review Can't say much about the OpenGL code, but it looks alright. The rest are mostly nitpicks. > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:541 > + if (!filter) > + continue; In which case can this happen? Ideally this shouldn't be there. > Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:212 > m_textureMapperShaderProgramMap.clear(); > +#if ENABLE(CSS_FILTERS) > + m_filterMap.clear(); > +#endif Wouldn't those be implicit anyway? > Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:66 > +#if USE(UI_SIDE_COMPOSITING) && ENABLE(CSS_FILTERS) > +#include <WebCore/FilterOperations.h> > +#endif Could you limit the guards in WebCoreArgumentCoders to ENABLE(CSS_FILTERS) only? The Animation coders aren't guarded, you also wouldn't need to guard the include in that case.
Noam Rosenthal
Comment 6 2012-05-02 05:56:06 PDT
> > Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:66 > > +#if USE(UI_SIDE_COMPOSITING) && ENABLE(CSS_FILTERS) > > +#include <WebCore/FilterOperations.h> > > +#endif > > Could you limit the guards in WebCoreArgumentCoders to ENABLE(CSS_FILTERS) only? > The Animation coders aren't guarded, you also wouldn't need to guard the include in that case. The animation coders are guarded with PLATFORM(QT), to smfr's request. The above guard seems reasonable. If another ports wants to unguard them, they can go ahead.
Jocelyn Turcotte
Comment 7 2012-05-02 06:02:40 PDT
(In reply to comment #6) > The animation coders are guarded with PLATFORM(QT), to smfr's request. The above guard seems reasonable. If another ports wants to unguard them, they can go ahead. Ok I didn't see that, makes sense.
Noam Rosenthal
Comment 8 2012-05-02 06:09:01 PDT
Jocelyn Turcotte
Comment 9 2012-05-02 06:23:07 PDT
Comment on attachment 139796 [details] Patch (In reply to comment #5) > > Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:212 > > m_textureMapperShaderProgramMap.clear(); > > +#if ENABLE(CSS_FILTERS) > > + m_filterMap.clear(); > > +#endif > > Wouldn't those be implicit anyway? Any comment on this? For the rest, r=me
Noam Rosenthal
Comment 10 2012-05-02 06:28:28 PDT
> > Wouldn't those be implicit anyway? > > Any comment on this? > For the rest, r=me Oh, thought I'd removed them. I'll remove this before committing.
Noam Rosenthal
Comment 11 2012-05-02 06:56:34 PDT
Created attachment 139806 [details] Patch for landing
WebKit Review Bot
Comment 12 2012-05-02 06:59:35 PDT
Comment on attachment 139806 [details] Patch for landing Rejecting attachment 139806 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/Source/WebKit2/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/12595809
Noam Rosenthal
Comment 13 2012-05-02 07:23:53 PDT
Created attachment 139812 [details] Patch for Landing
WebKit Review Bot
Comment 14 2012-05-02 08:29:49 PDT
Comment on attachment 139812 [details] Patch for Landing Clearing flags on attachment: 139812 Committed r115838: <http://trac.webkit.org/changeset/115838>
WebKit Review Bot
Comment 15 2012-05-02 08:29:55 PDT
All reviewed patches have been landed. Closing bug.
Alexandru Chiculita
Comment 16 2012-05-02 09:42:07 PDT
Comment on attachment 139812 [details] Patch for Landing View in context: https://bugs.webkit.org/attachment.cgi?id=139812&action=review Sorry for adding comments so late. I'm not a reviewer, so feel free to ignore them :) > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:512 > + RefPtr<StandardFilterProgram> program = data().sharedGLData().textureMapperShaderManager.getShaderForFilter(filter); Not an issue right now, but I suppose you will need multiple passes for the blur and drop-shadow (ie. vertical, horizontal) > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:521 > + glUniform1i(program->textureUniform(), 0); Some GL commands have GL_CMD and some don't. Is there a reason not to use it here? > Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:214 > + glDetachShader(m_id, m_vertexShader); Do you need to make the GL context current here? What thread is going to run the destructor? I suppose it's the same as the one for ~TextureMapperShaderManager. > Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:345 > + glGetShaderInfoLog(fragmentShader, 100, &len, log); The "log" seems to be required only for debugging reasons. > Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:418 > + program->prepare(filter); It seems weird to call prepare in a method called "getShaderForFilter".
Noam Rosenthal
Comment 17 2012-05-02 09:48:58 PDT
(In reply to comment #16) > (From update of attachment 139812 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=139812&action=review > > Sorry for adding comments so late. I'm not a reviewer, so feel free to ignore them :) Already committed, but you comments are always welcome! > > > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:512 > > + RefPtr<StandardFilterProgram> program = data().sharedGLData().textureMapperShaderManager.getShaderForFilter(filter); > > Not an issue right now, but I suppose you will need multiple passes for the blur and drop-shadow (ie. vertical, horizontal) Yes. The idea would be that this function would get a "pass" number. > > > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:521 > > + glUniform1i(program->textureUniform(), 0); > > Some GL commands have GL_CMD and some don't. Is there a reason not to use it here? Just an oversight, no reason :) > > > Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:214 > > + glDetachShader(m_id, m_vertexShader); > > Do you need to make the GL context current here? What thread is going to run the destructor? I suppose it's the same as the one for ~TextureMapperShaderManager. Yes. TextureMapperGL is only created and destroyed in a thread where the context is available. > > > Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:345 > > + glGetShaderInfoLog(fragmentShader, 100, &len, log); > > The "log" seems to be required only for debugging reasons. Oh, forgot it in. > > > Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:418 > > + program->prepare(filter); > > It seems weird to call prepare in a method called "getShaderForFilter". I think we need to refactor TextureMapperShaderManager at some point... not sure how to go about it.
Noam Rosenthal
Comment 18 2012-05-02 10:35:40 PDT
Reopening to attach new patch.
Noam Rosenthal
Comment 19 2012-05-02 10:35:44 PDT
Created attachment 139836 [details] Patch for landing
WebKit Review Bot
Comment 20 2012-05-02 11:26:15 PDT
Comment on attachment 139836 [details] Patch for landing Clearing flags on attachment: 139836 Committed r115855: <http://trac.webkit.org/changeset/115855>
WebKit Review Bot
Comment 21 2012-05-02 11:26:21 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.