Bug 75778 – [Texmap] Enable css filters in TextureMapperGL

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Noam Rosenthal]

Hardware-accelerate CSS filters using shaders in TextureMapperGL.
This, for now, does not include CSS shaders.

[Noam Rosenthal]

Created attachment 139364 [details]
Patch

[Early Warning System Bot]

Comment on attachment 139364 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 139364 [details]
Patch

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

[Noam Rosenthal]

Created attachment 139365 [details]
Patch

[Jocelyn Turcotte]

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]

> > 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]

(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]

Created attachment 139796 [details]
Patch

[Jocelyn Turcotte]

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]

> > 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]

Created attachment 139806 [details]
Patch for landing

[WebKit Review Bot]

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]

Created attachment 139812 [details]
Patch for Landing

[WebKit Review Bot]

Comment on attachment 139812 [details]
Patch for Landing

Clearing flags on attachment: 139812

Committed r115838: <http://trac.webkit.org/changeset/115838>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alexandru Chiculita]

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]

(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]

Reopening to attach new patch.

[Noam Rosenthal]

Created attachment 139836 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 139836 [details]
Patch for landing

Clearing flags on attachment: 139836

Committed r115855: <http://trac.webkit.org/changeset/115855>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.