Bug 144105 – [GTK] Add one single option to control all OpenGL-related options

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Michael Catanzaro]

The current set of OpenGL-related options is really confusing. Let's clean them up so that we expose only one new all-or-nothing option to control whether OpenGL features are used, and also ENABLE_GLES2.

[Michael Catanzaro]

I'm almost satisfied with this patch, but I had some trouble with ENABLE_ACCELERATED_2D_CANVAS. We expect this option to be disabled for most users, but if CairoGL is available (it's not generally) then we really do want to use it automatically. But to do this in a non-broken way, the default has to be set before WEBKIT_OPTION_END is called (otherwise the value will never make it into the FEATURE_DEFINES). But to decide if CairoGL is available, we need to first test if GLX or EGL is available. But we should not test if GLX is available before we know if ENABLE_X11_TARGET is set, which we can't test until after WEBKIT_OPTION_END is called (because I plan to make ENABLE_X11_TARGET a proper option). So that's impossible to do properly.

So this patch just turns off ENABLE_ACCELERATED_2D_CANVAS, and expects users to enable it manually. I don't see any way around this unless we give up on making ENABLE_X11_TARGET and ENABLE_WAYLAND_TARGET proper options. But those seem more important than this, IMO.

[Michael Catanzaro]

Created attachment 251512 [details]
Patch

[Michael Catanzaro]

Created attachment 251523 [details]
Patch

[Michael Catanzaro]

This patch names the option ENABLE_3D_GRAPHICS. We also discussed ENABLE_OPENGL as a possible name. I think anything we choose will be confusing.

[Martin Robinson]

(In reply to comment #4)
> This patch names the option ENABLE_3D_GRAPHICS. We also discussed
> ENABLE_OPENGL as a possible name. I think anything we choose will be
> confusing.

Let's not add ENABLE_3D_GRAPHICS when WTF_USE_3D_GRAPHICS still exists. Instead I propose that we first rename WTF_USE_3D_GRAPHICS to ENABLE_GRAPHICS_CONTEXT_3D.

[Michael Catanzaro]

(In reply to comment #5)
> Let's not add ENABLE_3D_GRAPHICS when WTF_USE_3D_GRAPHICS still exists.
> Instead I propose that we first rename WTF_USE_3D_GRAPHICS to
> ENABLE_GRAPHICS_CONTEXT_3D.

I'll do this in bug #144193.

(In reply to comment #1)
> I'm almost satisfied with this patch, but I had some trouble with
> ENABLE_ACCELERATED_2D_CANVAS. We expect this option to be disabled for most
> users, but if CairoGL is available (it's not generally) then we really do
> want to use it automatically. 

Martin, I'm not sure this is correct. In Fedora we compile cairo with --enable-gl, so our WebKitGTK+ build indeed uses ENABLE_ACCELERATED_2D_CANVAS. This makes me suspect it should perhaps be enabled by default....

[Martin Robinson]

(In reply to comment #6)

> Martin, I'm not sure this is correct. In Fedora we compile cairo with
> --enable-gl, so our WebKitGTK+ build indeed uses
> ENABLE_ACCELERATED_2D_CANVAS. This makes me suspect it should perhaps be
> enabled by default....

This surprises me, but I guess we should have it turn on and off automatically like Geoclue2. As you say, many distributions have it enabled by default. The main issue is that the binary will break if the API of CairoGL changes.

[Michael Catanzaro]

Created attachment 251809 [details]
Patch

[Michael Catanzaro]

Created attachment 251811 [details]
Patch

[Martin Robinson]

Comment on attachment 251811 [details]
Patch

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

This seems like a reasonable direction.

> Source/cmake/OptionsGTK.cmake:154
> +# Normally we do not set the value of options automatically. However, CairoGL is special. Currently
> +# most major distros compile Cario with --enable-gl, but Debian and derivitives are a major
> +# exception. You very probably want accelerated 2D canvas if Cario has been compiled with CarioGL,
> +# and very probably do not want to recompile Cario otherwise. So we expect some major distros will
> +# enable this feature, and others will not, and that is just fine for the time being. Once Debian
> +# enables CairoGL, then it will be time to force this ON by default. Note that if GLX is installed,
> +# EGL is not, and ENABLE_X11_TARGET is OFF, this guess is wrong and the user must override it. We
> +# can't check ENABLE_X11_TARGET at this point because we don't know whether it's enabled until
> +# WEBKIT_OPTION_END has been called, and at that point it's too late to change default values.
> +#
> +# FIXME The above is not quite true yet because ENABLE_X11_TARGET is not yet an option, bug #144106.
> +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_ACCELERATED_2D_CANVAS PUBLIC ${CAIRO_GL_FOUND})

This maintains the status quo for the moment, but later I think we should just consider turning it off by default because of API/ABI stability issues.

[Michael Catanzaro]

I dunno. How unstable is CairoGL really? Judging from [1], the API/ABI hasn't changed much since 2009. My guess it it is "unstable" in the same sense that libnotify is still unstable (nothing ever changes there).

But if CairoGL is *unreliable* unstable and ENABLE_ACCELERATED_2D_CANVAS is just a bad idea, then we should turn it off indeed. But it's evidently been enabled everywhere except Debian/Ubuntu for years without any issues being tied to it, so I guess it's not.

[1] http://cgit.freedesktop.org/cairo/log/src/cairo-gl.h

[WebKit Commit Bot]

Comment on attachment 251811 [details]
Patch

Clearing flags on attachment: 251811

Committed r183452: <http://trac.webkit.org/changeset/183452>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.