[cairo] [PATCH 3/3] gl: add support for OpenGL ES 3.0
Bryce Harrington
bryce at osg.samsung.com
Wed Dec 14 20:21:44 UTC 2016
On Sat, Dec 10, 2016 at 04:22:05PM +0100, Uli Schlachter wrote:
> Again, I don't know OpenGL, so feel free to ignore anything stupid that
> I say.
Hi Uli, thanks I appreciate the review. This is an area I'm not super
deep in myself so value the second set of eyes.
> On 08.12.2016 00:46, Bryce Harrington wrote:
> > @@ -87,7 +89,7 @@ _cairo_boilerplate_egl_create_surface (const char *name,
> > EGL_SURFACE_TYPE, EGL_PBUFFER_BIT,
> > #if CAIRO_HAS_GL_SURFACE
> > EGL_RENDERABLE_TYPE, EGL_OPENGL_BIT,
> > -#elif CAIRO_HAS_GLESV2_SURFACE
> > +#elif CAIRO_HAS_GLESV2_SURFACE || CAIRO_HAS_GLESV3_SURFACE
> > EGL_RENDERABLE_TYPE, EGL_OPENGL_ES2_BIT,
> > #endif
> > EGL_NONE
>
> Why ES2_BIT instead of ES3_BIT?
It looks like there is not an EGL_OPENGL_ES2_BIT defined anywhere. The
docs don't mention it:
https://www.khronos.org/registry/egl/sdk/docs/man/html/eglChooseConfig.xhtml
I do see there is a EGL_OPENGL_ES3_BIT_KHR defined in the headers
(eglext.h). This is described here:
https://www.khronos.org/registry/egl/extensions/KHR/EGL_KHR_create_context.txt
Offhand, it looks like EGL_OPENGL_ES3_BIT_KHR was added late in the
GLESv3 definition, and some GLESv3 implementations might not have had it
in the past. I might guess that at the time Henry implemented the
patchset it was not available on the systems he was testing on; there's
no mention of it in his cairoglesv3 tree.
>From the second kronos link (see Issue 8) it sounds like a proper
implementation would try to create the context with
EGL_OPENGL_ES3_BIT_KHR and then if it fails with a EGL_BAD_ATTIBUTE
error, we should re-try with ES2_BIT.
I think you're probably right that ES3_BIT would be more proper, but
I'll do a bit more investigation first.
> [...]
> > diff --git a/cairo-glesv3-uninstall.pc b/cairo-glesv3-uninstall.pc
> > new file mode 100644
> > index 0000000..00ea39a
> > --- /dev/null
> > +++ b/cairo-glesv3-uninstall.pc
> > @@ -0,0 +1,7 @@
> > +Name: cairo-glesv3
> > +Description: OpenGLESv3 surface backend for cairo graphics library
> > +Version: 1.12.14
> > +
> > +Requires: cairo glesv3
> > +Libs:
> > +Cflags: -I${pc_top_builddir}/${pcfiledir}/./src
>
> What's up with this file? There are no other such files in git (but
> build/configure.ac.features generates such files at configure-time) and
> the file name is wrong (it would need to be "uninstalled" instead of
> "uninstall" so that pkg-config picks it up).
>
> Giving the above, I'd say this file can just be dropped.
Ok.
> > diff --git a/configure.ac b/configure.ac
> > index 93953a7..ea71ad6 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -386,6 +386,24 @@ CAIRO_ENABLE_SURFACE_BACKEND(glesv2, OpenGLESv2, no, [
> > ])
> >
> > dnl ===========================================================================
> > +CAIRO_ENABLE_SURFACE_BACKEND(glesv3, OpenGLESv3, no, [
> > + glesv3_REQUIRES="glesv2"
> > + PKG_CHECK_MODULES(glesv3, $glesv3_REQUIRES,, [
> > + dnl Fallback to searching for headers
> > + AC_CHECK_HEADER(GLES3/gl3.h,, [use_glesv3="no (glesv2.pc nor OpenGL ES 3.0 headers not found)"])
> > + if test "x$use_glesv3" = "xyes"; then
> > + glesv3_NONPKGCONFIG_CFLAGS=
> > + glesv3_NONPKGCONFIG_LIBS="-lGLESv2"
> > + fi])
>
> Really? The pkg-config file for glesv3 is calles glesv2 and the library
> is called libGLESv2? Shouldn't there be more "3"s in there?
Yeah, that does look odd. Maybe it's attempting to fallback to v2 if
the v3 stuff is missing? But this isn't the right way to do that. I'll
rework this.
> [...]
> > diff --git a/src/cairo-gl-composite.c b/src/cairo-gl-composite.c
> > index a95712e..76bc1ef 100644
> > --- a/src/cairo-gl-composite.c
> > +++ b/src/cairo-gl-composite.c
> > @@ -52,6 +52,91 @@
> [...]
> > +static cairo_int_status_t
> > +_blit_texture_to_renderbuffer (cairo_gl_surface_t *surface)
> > +{
> > + cairo_gl_context_t *ctx = NULL;
> > + cairo_gl_composite_t setup;
> > + cairo_surface_pattern_t pattern;
> > + cairo_rectangle_int_t extents;
> > + cairo_int_status_t status;
> > +
> > + /* FIXME: we need to take care of certain glesv2 extension too */
>
> That FIXME is quite vague to me. Could something like "for example FOO"
> be added, or anything to make this more precise?
Yes, I can fix that. There's actually a subsequent patch that fills
this in, but it's mixed in with some work adding an ANGLE feature. I'll
see if I can extract the fix for this, or if not at least improve the
comment.
> [...]
> > @@ -624,14 +656,19 @@ bind_multisample_framebuffer (cairo_gl_context_t *ctx,
> > cairo_bool_t scissor_test_enabled;
> >
> > assert (surface->supports_msaa);
> > - assert (ctx->gl_flavor == CAIRO_GL_FLAVOR_DESKTOP);
> > + assert (ctx->gl_flavor == CAIRO_GL_FLAVOR_DESKTOP ||
> > + ctx->gl_flavor == CAIRO_GL_FLAVOR_ES3);
> >
> > _cairo_gl_ensure_framebuffer (ctx, surface);
> > _cairo_gl_ensure_multisampling (ctx, surface);
> >
> > if (surface->msaa_active) {
> > +#if CAIRO_HAS_GL_SURFACE
> > glEnable (GL_MULTISAMPLE);
> > +#endif
>
> This seems fishy to me. Shouldn't there be something like ctx->gl_flavor
> == CAIRO_GL_FLAVOR_DESKTOP around this call to glEnable()? Otherwise it
> will be done with GLESv3 if and only if the "desktop gl" stuff was enabled.
That could be; I gather that GLESv3 automatically enables multisampling
so the glEnable call isn't necessary there. So perhaps testing for
CAIRO_GL_FLAVOR_DESKTOP would be more proper here. The ANGLE patch I
mentioned earlier tweaks this logic a bit, so perhaps I need to tease
some of that out and squash it here.
> > ctx->dispatch.BindFramebuffer (GL_FRAMEBUFFER, surface->msaa_fb);
> > + if (ctx->gl_flavor == CAIRO_GL_FLAVOR_ES3)
> > + surface->content_in_texture = FALSE;
> > return;
> > }
> >
> > @@ -642,7 +679,9 @@ bind_multisample_framebuffer (cairo_gl_context_t *ctx,
> > glDisable (GL_STENCIL_TEST);
> > glDisable (GL_SCISSOR_TEST);
> >
> > +#if CAIRO_HAS_GL_SURFACE
> > glEnable (GL_MULTISAMPLE);
> > +#endif
>
> Same as above.
> [...]
> > @@ -669,11 +710,15 @@ bind_singlesample_framebuffer (cairo_gl_context_t *ctx,
> > cairo_bool_t stencil_test_enabled;
> > cairo_bool_t scissor_test_enabled;
> >
> > - assert (ctx->gl_flavor == CAIRO_GL_FLAVOR_DESKTOP);
> > + assert (ctx->gl_flavor == CAIRO_GL_FLAVOR_DESKTOP ||
> > + ctx->gl_flavor == CAIRO_GL_FLAVOR_ES3);
> > _cairo_gl_ensure_framebuffer (ctx, surface);
> >
> > if (! surface->msaa_active) {
> > +#if CAIRO_HAS_GL_SURFACE
> > glDisable (GL_MULTISAMPLE);
> > +#endif
> > +
>
> Same.
>
> > ctx->dispatch.BindFramebuffer (GL_FRAMEBUFFER, surface->fb);
> > return;
> > }
> > @@ -685,7 +730,9 @@ bind_singlesample_framebuffer (cairo_gl_context_t *ctx,
> > glDisable (GL_STENCIL_TEST);
> > glDisable (GL_SCISSOR_TEST);
> >
> > +#if CAIRO_HAS_GL_SURFACE
> > glDisable (GL_MULTISAMPLE);
> > +#endif
>
> Same.
>
> [...]
> > diff --git a/src/cairo-gl-info.c b/src/cairo-gl-info.c
> > index 39541aa..29b8768 100644
> > --- a/src/cairo-gl-info.c
> > +++ b/src/cairo-gl-info.c
> > @@ -67,6 +67,14 @@ _cairo_gl_get_flavor (void)
> > flavor = CAIRO_GL_FLAVOR_NONE;
> > else if (strstr (version, "OpenGL ES 2") != NULL)
> > flavor = CAIRO_GL_FLAVOR_ES2;
> > + else if (strstr (version, "OpenGL ES 3") != NULL)
> > +#if CAIRO_HAS_GLESV3_SURFACE
> > + flavor = CAIRO_GL_FLAVOR_ES3;
> > +#elif CAIRO_HAS_GLESV2_SURFACE
> > + flavor = CAIRO_GL_FLAVOR_ES2;
> > +#else
> > + flavor = CAIRO_GL_FLAVOR_NONE;
> > +#endif
> > else
> > flavor = CAIRO_GL_FLAVOR_DESKTOP;
>
> This patch is about adding support for OpenGL ES 3. But the above
> changes the behaviour for GLES 2 if the new GLES 3 support is disabled.
> Is that intended?
Guessing it probably has no effect in practice, but you're right, the
fallback for undefined CAIRO_HAS_GLESV2_SURFACE should be the same
regardless of whether the system's ES support is version 2 or 3.
I'll fix this and look at the following.
> > diff --git a/src/cairo-gl-msaa-compositor.c b/src/cairo-gl-msaa-compositor.c
> > index 507459d..fcd4144 100644
> > --- a/src/cairo-gl-msaa-compositor.c
> > +++ b/src/cairo-gl-msaa-compositor.c
> > @@ -280,8 +280,10 @@ can_use_msaa_compositor (cairo_gl_surface_t *surface,
> > /* Multisampling OpenGL ES surfaces only maintain one multisampling
> > framebuffer and thus must use the spans compositor to do non-antialiased
> > rendering. */
> > - if (((cairo_gl_context_t *) surface->base.device)->gl_flavor == CAIRO_GL_FLAVOR_ES2
> > + if ((((cairo_gl_context_t *) surface->base.device)->gl_flavor == CAIRO_GL_FLAVOR_ES2 ||
> > + ((cairo_gl_context_t *) surface->base.device)->gl_flavor == CAIRO_GL_FLAVOR_ES3)
> > && surface->supports_msaa
> > + && surface->num_samples > 1
> > && antialias == CAIRO_ANTIALIAS_NONE)
> > return FALSE;
>
> The above also changes things even if ES3 is disabled, doesn't it?
>
> [...]
> > @@ -1374,10 +1408,14 @@ _cairo_gl_surface_resolve_multisampling (cairo_gl_surface_t *surface)
> > if (unlikely (status))
> > return status;
> >
> > - ctx->current_target = surface;
> > + _cairo_gl_composite_flush (ctx);
> > +
> > + ctx->current_target = NULL;
> [...]
>
> Things like this look weird to me since the code will behave differently
> even if GLES3 is not enabled. This is the only such place I mention,
> because I do not know the code to say more about it...
I'll comb through and try and remove more of these, and if the change
seems justifyable will submit as a discrete patch.
> Hopefully something from this was useful.
Definitely, thanks again!
Bryce
> Uli
> --
> Happiness can't be found -- it finds you.
> - Majic
More information about the cairo
mailing list