Bug 95603 – [Gtk] allow building with css-shaders

• Component: WebKitGTK
• Status: RESOLVED FIXED

[arno.]

Hi,
WebCore supports css-shaders.
But currently, gtk build system does not support building it.

[arno.]

Created attachment 161758 [details]
patch proposal

With this patch, it's possible to build gtk port with css shaders: build-webkit --gtk --css-shaders --css-filters
For the settings, I've taken example on the qt port, and enable css shaders when webgl is enabled

[Martin Robinson]

Comment on attachment 161758 [details]
patch proposal

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

I think that Zan should double-check this patch to see if it fits well with the work that he is doing to handle eliminating configuration arguments and make the feature list autogenerated.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:3436
> +#if ENABLE(CSS_SHADERS)
> +    // For now, enable CSS shaders when WebGL is enabled.
> +    coreSettings->setCSSCustomFilterEnabled(settingsPrivate->enableWebgl);
> +#endif

I think this doesn't necessarily make sense because:

1. CSS shaders are still an experimental features, so I think that people would probably like a way to turn on WebGL (which isn't experimental) without enabling CSS shaders.
2. CSS shaders really depend on the TextureMapperGL backend at the moment, not WebGL.

> configure.ac:1174
> +# check whether to enable CSS Shaders support
> +AC_MSG_CHECKING([whether to enable CSS Shaders])
> +AC_ARG_ENABLE(css_shaders,
> +              AC_HELP_STRING([--enable-css-shaders],
> +                             [enable support for CSS Shaders [default=no]]),
> +              [],[enable_css_shaders="no"])
> +AC_MSG_RESULT([$enable_css_shaders])
> +

I think what we want to do here instead is to enable them when experimental features are enabled and otherwise not. I'd like to have them turned on eventually, but probably behind WebKitWebSettings::enable-css-shaders.

So instead of a configure option why not just enable them when experimental features are on and accelerated compositing is enabled.

[Martin Robinson]

CCing Zan as he's currently the person who knows the most about the direction the configuration options are moving.

[Zan Dobersek]

Comment on attachment 161758 [details]
patch proposal

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

> Source/WebCore/GNUmakefile.am:517
> +feature_defines_overrides += ENABLE_CSS_SHADERS=1

You'll also need to add ENABLE_CSS_SHADERS entry to Source/WebCore/GNUmakefile.features.am. It's value depends on whether it should be enabled in developer builds or not.

If yes, you'll also need to override the feature define with ENABLE_CSS_SHADERS=0 if unstable features are not to be enabled. That's done here:
http://trac.webkit.org/browser/trunk/Source/WebCore/GNUmakefile.am#L771
There's a bug here though, duplicate entries can pop up in feature_defines and webcore_cppflags variables. I'll post a fix for that soon.

Also, if the configuration flag in configure.ac is in the end not used, this overriding depending on the ENABLE_CSS_SHADERS conditional is useless and should be removed.

>> configure.ac:1174
>> +AC_MSG_RESULT([$enable_css_shaders])
>> +
> 
> I think what we want to do here instead is to enable them when experimental features are enabled and otherwise not. I'd like to have them turned on eventually, but probably behind WebKitWebSettings::enable-css-shaders.
> 
> So instead of a configure option why not just enable them when experimental features are on and accelerated compositing is enabled.

If I'm understanding things correctly, CSS Shaders rely on TextureMapperGL (i.e. the acceleration backend being OpenGL). In that regard it is the same as WebGL, which currently gets enabled by default if the acceleration backend is OpenGL and throws an error if not. Perhaps CSS Shaders should do the same, meaning they should be guarded by a configuration flag?

[Martin Robinson]

(In reply to comment #4)

> If I'm understanding things correctly, CSS Shaders rely on TextureMapperGL (i.e. the acceleration backend being OpenGL). In that regard it is the same as WebGL, which currently gets enabled by default if the acceleration backend is OpenGL and throws an error if not. Perhaps CSS Shaders should do the same, meaning they should be guarded by a configuration flag?

One potential difference from WebGL is that WebGL works even if accelerated compositing is turned off. I'm not sure if CSS shaders work if accelerated compositing is turned off, since I believe the implementation is in the actual TextureMapper itself.

[arno.]

(In reply to comment #2)

> So instead of a configure option why not just enable them when experimental features are on and accelerated compositing is enabled.

Do you mean having a --enable-experimental flags in configure.ac which would enable a few experimental flags ?

> One potential difference from WebGL is that WebGL works even if accelerated compositing is turned off. I'm not sure if CSS shaders work if accelerated compositing is turned off, since I believe the implementation is in the actual TextureMapper itself.

Indeed, css shaders cannot build without accelerated compositing, because it uses TilingData stuff.

[Zan Dobersek]

(In reply to comment #6)
> (In reply to comment #2)
> 
> > So instead of a configure option why not just enable them when experimental features are on and accelerated compositing is enabled.
> 
> Do you mean having a --enable-experimental flags in configure.ac which would enable a few experimental flags ?

The flag is already there and currently named --enable-unstable-features.

[arno.]

(In reply to comment #4)
> (From update of attachment 161758 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161758&action=review
> 
> > Source/WebCore/GNUmakefile.am:517
> > +feature_defines_overrides += ENABLE_CSS_SHADERS=1
> 
> You'll also need to add ENABLE_CSS_SHADERS entry to Source/WebCore/GNUmakefile.features.am. It's value depends on whether it should be enabled in developer builds or not.

ENABLE_CSS_SHADERS was already defined in GNUmakefile.features.am


> Also, if the configuration flag in configure.ac is in the end not used, this overriding depending on the ENABLE_CSS_SHADERS conditional is useless and should be removed.

I'm not sure what you mean here.

[arno.]

Created attachment 162568 [details]
updated patch

updated patch:

define enable_css_shaders in configure.ac if: experimental features and accelerated compositing and css filters are all enabled.

define a enable-css-shaders property in webkitwebsettings. At runtime, css shaders can work even if accelerated compositing setting is disabled

[arno.]

(In reply to comment #9)
> Created an attachment (id=162568) [details]
> updated patch
> 
> updated patch:
> 
> define enable_css_shaders in configure.ac if: experimental features and accelerated compositing and css filters are all enabled.


Unfortunately, css-filter support has just been removed in bug #87664

[arno.]

Created attachment 163422 [details]
updated patch: build on top of #87664 and reintroduce css-filters

[Martin Robinson]

Comment on attachment 163422 [details]
updated patch: build on top of #87664 and reintroduce css-filters

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

> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:948
> +    * Enable or disable support for css shaders (css custom filters)

It might be a good idea to link to the spec here as well as indicate whether or not accelerated compositing is required for this to work. A minor nit: you're missing a period here.

> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:950
> +    * Since: 1.9.7

This should be 2.0

> configure.ac:967
> +# check whether to enable CSS Filters support

You can just omit this comment.

> configure.ac:973
> +AC_ARG_ENABLE(css_filters,
> +              AC_HELP_STRING([--enable-css-filters],
> +                             [enable support for CSS Filters [default=no]]),
> +              [],[enable_css_filters="no"])
> +AC_MSG_RESULT([$enable_css_filters])

It still seems odd to expose a configure flag for a feature that isn't complete yet. Maybe you can just remove this.

> configure.ac:976
> +# enable css shaders if unstable_features, css_filters and
> +# accelerated_compositing are turned on

Nit: Missing a capital letter and a period at the end.

> configure.ac:983
> +AC_MSG_RESULT([$enable_css_shaders])

How about just doing this:

if test "$enable_unstable_features" = "yes" && test "$enable_accelerated_compositing" = "yes" && test "$with_acceleration_backend" = "opengl"; then
    enable_css_shaders=yes
    enable_css_filters=yes
fi

Does that make sense?

> configure.ac:1356
> + CSS Filters support                                      : $enable_css_filters
> + CSS Shaders support                                      : $enable_css_shaders

Probably best to not include this as well.

[arno.]

Created attachment 163434 [details]
updated patch to address comments

[Martin Robinson]

Comment on attachment 163434 [details]
updated patch to address comments

Okay. Let's give this a shot.

[WebKit Review Bot]

Comment on attachment 163434 [details]
updated patch to address comments

Clearing flags on attachment: 163434

Committed r128231: <http://trac.webkit.org/changeset/128231>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.