Bug 75778 - [Texmap] Enable css filters in TextureMapperGL
Summary: [Texmap] Enable css filters in TextureMapperGL
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Noam Rosenthal
URL:
Keywords:
Depends on: 75779
Blocks: 75918 75677
  Show dependency treegraph
 
Reported: 2012-01-07 09:37 PST by Noam Rosenthal
Modified: 2012-05-02 11:26 PDT (History)
5 users (show)

See Also:


Attachments
Patch (40.19 KB, patch)
2012-04-28 10:22 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (40.19 KB, patch)
2012-04-28 10:49 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (40.13 KB, patch)
2012-05-02 06:09 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch for landing (40.08 KB, patch)
2012-05-02 06:56 PDT, Noam Rosenthal
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch for Landing (38.78 KB, patch)
2012-05-02 07:23 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch for landing (1.37 KB, patch)
2012-05-02 10:35 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Noam Rosenthal 2012-01-07 09:37:19 PST
Hardware-accelerate CSS filters using shaders in TextureMapperGL.
This, for now, does not include CSS shaders.
Comment 1 Noam Rosenthal 2012-04-28 10:22:59 PDT
Created attachment 139364 [details]
Patch
Comment 2 Early Warning System Bot 2012-04-28 10:42:15 PDT
Comment on attachment 139364 [details]
Patch

Attachment 139364 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12558638
Comment 3 Early Warning System Bot 2012-04-28 10:44:30 PDT
Comment on attachment 139364 [details]
Patch

Attachment 139364 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12555673
Comment 4 Noam Rosenthal 2012-04-28 10:49:14 PDT
Created attachment 139365 [details]
Patch
Comment 5 Jocelyn Turcotte 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.
Comment 6 Noam Rosenthal 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.
Comment 7 Jocelyn Turcotte 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.
Comment 8 Noam Rosenthal 2012-05-02 06:09:01 PDT
Created attachment 139796 [details]
Patch
Comment 9 Jocelyn Turcotte 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
Comment 10 Noam Rosenthal 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.
Comment 11 Noam Rosenthal 2012-05-02 06:56:34 PDT
Created attachment 139806 [details]
Patch for landing
Comment 12 WebKit Review Bot 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
Comment 13 Noam Rosenthal 2012-05-02 07:23:53 PDT
Created attachment 139812 [details]
Patch for Landing
Comment 14 WebKit Review Bot 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>
Comment 15 WebKit Review Bot 2012-05-02 08:29:55 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Alexandru Chiculita 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".
Comment 17 Noam Rosenthal 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.
Comment 18 Noam Rosenthal 2012-05-02 10:35:40 PDT
Reopening to attach new patch.
Comment 19 Noam Rosenthal 2012-05-02 10:35:44 PDT
Created attachment 139836 [details]
Patch for landing
Comment 20 WebKit Review Bot 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>
Comment 21 WebKit Review Bot 2012-05-02 11:26:21 PDT
All reviewed patches have been landed.  Closing bug.