Re: [PATCH 3/3] gl: add support for OpenGL ES 3.0

Again, I don't know OpenGL, so feel free to ignore anything stupid that
I say.

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?

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

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

[...]
> 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?

[...]
> @@ -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.

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

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

Hopefully something from this was useful.

Uli
-- 
Happiness can't be found -- it finds you.
 - Majic
-- 
cairo mailing list
[email protected]
https://lists.cairographics.org/mailman/listinfo/cairo