Bug 98733 – [WK2] Add CustomFilterOperation serialization in ArgumentCoder.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Dongseong Hwang]

Add ArgumentCoders for CustomFilterOperation and all dependent classes.
Coordinated graphics will use this css shaders serialization when it enables CSS Shaders.

[Dongseong Hwang]

Created attachment 167718 [details]
Patch

[Early Warning System Bot]

Comment on attachment 167718 [details]
Patch

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

[Dongseong Hwang]

Created attachment 167722 [details]
Patch

[Noam Rosenthal]

Comment on attachment 167722 [details]
Patch

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

> Source/WebCore/GNUmakefile.list.am:4362
> +	Source/WebCore/platform/graphics/filters/StillCustomFilterProgram.h \

How about we call it WebCustomFilterProgram, and put it in WebKit2/Shared?

> Source/WebCore/rendering/style/StyleCustomFilterProgram.h:60
> +    virtual bool isStyleCustomFilterProgram() const OVERRIDE { return true; }
> +

Do you over have a StillCustomFilterProgram and a StyleCustomFilterProgram in the same process?
I don't think these comparisons are necessary.

[Dongseong Hwang]

(In reply to comment #4)
> How about we call it WebCustomFilterProgram, and put it in WebKit2/Shared?

Thanks for suggestions. I actually did not know where is the best place. :)
 
> Do you over have a StillCustomFilterProgram and a StyleCustomFilterProgram in the same process?
> I don't think these comparisons are necessary.

No, always lay in separate processes.
Absolutely!

[Dongseong Hwang]

Created attachment 167854 [details]
Patch

[Anders Carlsson]

Comment on attachment 167854 [details]
Patch

I don't think we want to allow sending arbitrary shader strings from the web process to the UI process.

[Noam Rosenthal]

(In reply to comment #7)
> (From update of attachment 167854 [details])
> I don't think we want to allow sending arbitrary shader strings from the web process to the UI process.

We want to allow this for coordinated graphics (Qt/EFL).
Any opposition to doing this if it's guarded with USE(COORDINATED_GRAPHICS)?

[Noam Rosenthal]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 167854 [details] [details])
> > I don't think we want to allow sending arbitrary shader strings from the web process to the UI process.
> 
> We want to allow this for coordinated graphics (Qt/EFL).
> Any opposition to doing this if it's guarded with USE(COORDINATED_GRAPHICS)?

Though in any case we would want to validate the shader first, and then serialize the CustomFilterValidatedProgram.

[Dongseong Hwang]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > (From update of attachment 167854 [details] [details] [details])
> > > I don't think we want to allow sending arbitrary shader strings from the web process to the UI process.
> > 
> > We want to allow this for coordinated graphics (Qt/EFL).
> > Any opposition to doing this if it's guarded with USE(COORDINATED_GRAPHICS)?
CSS_FILTERS and CSS_SHADERS guard are already in the USE(COORDINATED_GRAPHICS).

> 
> Though in any case we would want to validate the shader first, and then serialize the CustomFilterValidatedProgram.

Thank Anders and Noam for very valid concerns.

The most critical requirement is that UI TexMap never execute author's arbitrary shaders.

I think there are two solutions:
1. As Noam mentioned, validate the shader first, and then serialize the CustomFilterValidatedProgram.
2. Serialize CustomFilterProgram, and validate the shader using CustomFilterValidatedProgram.

I prefer the second solution, because :
1. Texmap will receive CustomFilterOperation that has CustomFilterProgram as a member variable.
2. We can validate arbitrary shaders using ANGLE validator with the SH_CSS_SHADERS_SPEC flag like CustomFilterValidatedProgram does.
3. Texmap can reuse many custom filter classes without huge refactoring.

I support the third reason.
createCustomFilterEffect() in FilterEffectRenderer makes CustomFilterValidatedProgram and FECustomFilter from CustomFilterOperation. FECustomFilter plays a all role to render css shaders.
I think TexMap reuse FECustomFilter to render css shaders because TexMap can have CustomFilterProgram though we need to refactor FECustomFilter little. In conclusion, if we choose the second solution, we can enable css shaders in TexMap reusing existing custom filter classes.

If you don't mind, I want to enable css shaders in TexMap soon.

[Noam Rosenthal]

Thank you for the explanation!
How about we guard all this with USE(COORDINATED_GRAPHICS), and move it out of WebCoreArgumentCoders.[h|cpp] tp WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.[h|cpp],

This way there would be no confusion about what this is used for.
Andersca, any issues with this?

[Dongseong Hwang]

(In reply to comment #11)
> Thank you for the explanation!
> How about we guard all this with USE(COORDINATED_GRAPHICS), and move it out of WebCoreArgumentCoders.[h|cpp] tp WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.[h|cpp],

It's good idea. I can do this in this bug.

[Dongseong Hwang]

Created attachment 167967 [details]
Patch

[Noam Rosenthal]

Comment on attachment 167967 [details]
Patch

Please rename StillCustomFilterProgram to WebCustomFilterProgram.

[Dongseong Hwang]

Created attachment 168539 [details]
Patch

[Dongseong Hwang]

(In reply to comment #14)
> (From update of attachment 167967 [details])
> Please rename StillCustomFilterProgram to WebCustomFilterProgram.

Done.

I filed Bug 98990 to enable CSS Shaders in TexMap. If you don't mind, I want to enable css shaders in TexMap. I'm afraid that I filed that bug by myself thought your team or you may be working on now.

[Noam Rosenthal]

Comment on attachment 168539 [details]
Patch

Please use CoordinatedGraphicsArgumentCoders and not CoordinatedArgumentCoders (only noticed that now)

[Noam Rosenthal]

(In reply to comment #16)
> (In reply to comment #14)
> > (From update of attachment 167967 [details] [details])
> > Please rename StillCustomFilterProgram to WebCustomFilterProgram.
> 
> Done.
> 
> I filed Bug 98990 to enable CSS Shaders in TexMap. If you don't mind, I want to enable css shaders in TexMap. I'm afraid that I filed that bug by myself thought your team or you may be working on now.

Nobody is currently working on it. If you get it running it would be awesome!

[Dongseong Hwang]

Created attachment 168595 [details]
Patch

[Dongseong Hwang]

(In reply to comment #18)
> Nobody is currently working on it. If you get it running it would be awesome!

Thanks! I'm very glad that I can contribute useful patches to TexMap soon.

(In reply to comment #17)
> (From update of attachment 168539 [details])
> Please use CoordinatedGraphicsArgumentCoders and not CoordinatedArgumentCoders (only noticed that now)

Oops. I'm sorry for causing this redundant iteration.

[Noam Rosenthal]

Comment on attachment 168595 [details]
Patch

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

Great!
Please fix nit-picks before committing...

> Source/WebKit2/ChangeLog:10
> +        Coordinated Graphics will use this css shaders serialization when it enables CSS
> +        Shaders.

Please remove these lines.

> Source/WebKit2/ChangeLog:30
> +            WebCustomFilterProgram is made from serialized data of
> +            StyleCustomFilterProgram in UI Process. UI Process uses this class
> +            instead of StyleCustomFilterProgram because UI Process can not get a
> +            custom filter program via network.

Just say "WebCustomFilterProgram is made to serialize the data of a StyleCustomFilterProgram to the UI process."

> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:29
> +#if USE(COORDINATED_GRAPHICS)

Put this line after the main include

> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:150
> +                encoder->encode(parameters[i]->name());
> +                encoder->encodeEnum(parameters[i]->parameterType());

Please save parameters[i] in a local variable.

[Noam Rosenthal]

> Oops. I'm sorry for causing this redundant iteration.
Thank you for helping with developing this feature! It's highly appreciated. Iterations are absolutely welcome as long as you're open to feedback :)

[Dongseong Hwang]

Comment on attachment 168595 [details]
Patch

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

>> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:29
>> +#if USE(COORDINATED_GRAPHICS)
> 
> Put this line after the main include

Do I mean like following code? I don't understand fully what do you say.
#include "config.h"
#include "CoordinatedGraphicsArgumentCoders.h"
#if USE(COORDINATED_GRAPHICS)


These code are similar to GraphicsContext3D.
#include "config.h"

#if USE(3D_GRAPHICS)

#include "GraphicsContext3D.h"

[Dongseong Hwang]

Created attachment 168619 [details]
Patch

[Dongseong Hwang]

Comment on attachment 168595 [details]
Patch

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

>> Source/WebKit2/ChangeLog:10
>> +        Shaders.
> 
> Please remove these lines.

Done.

>> Source/WebKit2/ChangeLog:30
>> +            custom filter program via network.
> 
> Just say "WebCustomFilterProgram is made to serialize the data of a StyleCustomFilterProgram to the UI process."

Done. Thanks for rephrasing

>>> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:29
>>> +#if USE(COORDINATED_GRAPHICS)
>> 
>> Put this line after the main include
> 
> Do I mean like following code? I don't understand fully what do you say.
> #include "config.h"
> #include "CoordinatedGraphicsArgumentCoders.h"
> #if USE(COORDINATED_GRAPHICS)
> 
> 
> These code are similar to GraphicsContext3D.
> #include "config.h"
> 
> #if USE(3D_GRAPHICS)
> 
> #include "GraphicsContext3D.h"

Done as I understood. Check please.

>> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:150
>> +                encoder->encodeEnum(parameters[i]->parameterType());
> 
> Please save parameters[i] in a local variable.

Done.

[Dongseong Hwang]

(In reply to comment #22)
> Thank you for helping with developing this feature! It's highly appreciated. Iterations are absolutely welcome as long as you're open to feedback :)

Thank you! and I think I'm open :)

[Noam Rosenthal]

Comment on attachment 168619 [details]
Patch

Looks good, but patch doesn't apply on EWS

[Noam Rosenthal]

Comment on attachment 168619 [details]
Patch

Looks fine, but patch doesn't apply...

[Dongseong Hwang]

Created attachment 168625 [details]
Patch

[Dongseong Hwang]

(In reply to comment #28)
> (From update of attachment 168619 [details])
> Looks fine, but patch doesn't apply...

I don't know what's happen. I post the patch again.

[Dongseong Hwang]

Created attachment 168668 [details]
Patch

[Dongseong Hwang]

Comment on attachment 168625 [details]
Patch

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

> Source/WebKit2/Shared/CoordinatedGraphics/WebCustomFilterProgram.h:61
> +        , m_fragmentShaderString(m_fragmentShaderString)

There is typo to initialize m_fragmentShaderString from m_fragmentShaderString.
It is why I posted new patch.

[Dongseong Hwang]

Created attachment 168670 [details]
Patch

[Dongseong Hwang]

Created attachment 169382 [details]
Patch

[Dongseong Hwang]

(In reply to comment #34)
> Created an attachment (id=169382) [details]
> Patch

Rebase upstream.

[WebKit Review Bot]

Comment on attachment 169382 [details]
Patch

Clearing flags on attachment: 169382

Committed r131741: <http://trac.webkit.org/changeset/131741>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #36)
> (From update of attachment 169382 [details])
> Clearing flags on attachment: 169382
> 
> Committed r131741: <http://trac.webkit.org/changeset/131741>

It broke the Qt --minimal build:

/ramdisk/qt-linux-release-minimal/build/Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:66: error: 'WebKit' is not a namespace-name
/ramdisk/qt-linux-release-minimal/build/Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:66: error: expected namespace-name before ';' token

Could you fix it, please?

[Csaba Osztrogonác]

after commenting the using namespace line, I got one more error:
/home/oszi/WebKit/Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:260: error: 'WebCustomFilterProgram' has not been declared

[Dongseong Hwang]

(In reply to comment #39)
> after commenting the using namespace line, I got one more error:
> /home/oszi/WebKit/Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:260: error: 'WebCustomFilterProgram' has not been declared

I'm sorry. I'll fix ASAP.

[Dongseong Hwang]

(In reply to comment #38)
> Could you fix it, please?

Thanks for reporting! I filed a Bug 99775 and posted a patch.