[Cogl] [PATCH 1/7] Add support for setting up stereo CoglOnscreens

[Robert Bragg]

On Thu, Jun 12, 2014 at 7:11 PM,  <otaylor at redhat.com> wrote:
> From: "Owen W. Taylor" <otaylor at fishsoup.net>
...
> diff --git a/cogl/winsys/cogl-winsys-glx-feature-functions.h b/cogl/winsys/cogl-winsys-glx-feature-functions.h
> index 1e2cec1..7e24638 100644
> --- a/cogl/winsys/cogl-winsys-glx-feature-functions.h
> +++ b/cogl/winsys/cogl-winsys-glx-feature-functions.h
> @@ -183,7 +183,12 @@ COGL_WINSYS_FEATURE_BEGIN (255, 255,
>                             "INTEL\0",
>                             "swap_event\0",
>                             0,
> +#if 1
>                             COGL_WINSYS_FEATURE_SYNC_AND_COMPLETE_EVENT)
> +#else
> +                           0)
> +#endif

guess you didn't mean to leave this in.

> +
>  COGL_WINSYS_FEATURE_END ()
>
>  COGL_WINSYS_FEATURE_BEGIN (255, 255,
> diff --git a/cogl/winsys/cogl-winsys-glx.c b/cogl/winsys/cogl-winsys-glx.c
> index 3095acf..f9c50bd 100644
> --- a/cogl/winsys/cogl-winsys-glx.c
> +++ b/cogl/winsys/cogl-winsys-glx.c
> @@ -880,6 +880,8 @@ update_winsys_features (CoglContext *context, CoglError **error)
>    return TRUE;
>  }
>
> +#include <stdio.h>
> +
>  static void
>  glx_attributes_from_framebuffer_config (CoglDisplay *display,
>                                          CoglFramebufferConfig *config,
> @@ -909,6 +911,11 @@ glx_attributes_from_framebuffer_config (CoglDisplay *display,
>    attributes[i++] = 1;
>    attributes[i++] = GLX_STENCIL_SIZE;
>    attributes[i++] = config->need_stencil ? 1: GLX_DONT_CARE;
> +  if (config->stereo_enabled)
> +    {
> +      attributes[i++] = GLX_STEREO;
> +      attributes[i++] = TRUE;
> +    }
>
>    if (glx_renderer->glx_major == 1 && glx_renderer->glx_minor >= 4 &&
>        config->samples_per_pixel)
> @@ -948,6 +955,21 @@ find_fbconfig (CoglDisplay *display,
>                                               xscreen_num,
>                                               attributes,
>                                               &n_configs);
> +  if ((!config || n_configs == 0) && config->stereo_enabled)
> +    {
> +      CoglFramebufferConfig modified = *config;
> +      modified.stereo_enabled = FALSE;
> +
> +      glx_attributes_from_framebuffer_config (display, &modified, attributes);
> +
> +      configs = glx_renderer->glXChooseFBConfig (xlib_renderer->xdpy,
> +                                                xscreen_num,
> +                                                attributes,
> +                                                &n_configs);
> +
> +      XFree (configs);
> +    }

This bit doesn't seem right; I don't think we should be automatically
falling back like this. If the user requested stereo support and we
can't find a suitable config then that's simply a runtime error we
should report. This would also imply updating the api docs.

The intention for handling renderer + display configuration is that by
default they should aim to Just Work™, picking sensible defaults
according to the driver/winsys, but when the user explicitly
configures something, then that should behave like a strict
constraint. If something the user explicitly requires can't be
supported then we should report an error so that they know they need
to setup a fallback configuration.

We used to have automatic fallbacks a bit like this in the EGL winsys
since some EGL platforms couldn't support onscreen framebuffers with
an alpha component, but we removed that in the end since it made it
difficult for applications to really trust what they had configured.

> +
>    if (!configs || n_configs == 0)
>      {
>        _cogl_set_error (error, COGL_WINSYS_ERROR,
> @@ -1286,6 +1308,17 @@ _cogl_winsys_onscreen_init (CoglOnscreen *onscreen,
>        framebuffer->samples_per_pixel = samples;
>      }
>
> +  if (framebuffer->config.stereo_enabled)
> +    {
> +      int stereo;
> +      int status = glx_renderer->glXGetFBConfigAttrib (xlib_renderer->xdpy,
> +                                                       fbconfig,
> +                                                       GLX_STEREO,
> +                                                       &stereo);
> +      g_return_val_if_fail (status == Success, TRUE);
> +      framebuffer->is_stereo = stereo != 0;
> +    }

We shouldn't use a g_return_val_if_fail debug assertion for something
that is allowed to fail. If we go with the behaviour described above
where we report a CoglError if stereo_enabled is set but we didn't
find a suitable config then we should be able to remove this chunk and
remove the separate is_stereo member from CoglFramebuffer.

The code just above this, that I guess you were using as a reference,
is for a slightly different kind of config option where it should have
already returned with a CoglError if the user wanted to enable
multisamping and no config with a GLX_SAMPLES attribute was found.
This code is just a follow up to see exactly how many
samples-per-pixel will be made.

> +
>    /* FIXME: We need to explicitly Select for ConfigureNotify events.
>     * For foreign windows we need to be careful not to mess up any
>     * existing event mask.
> @@ -1856,7 +1889,7 @@ _cogl_winsys_onscreen_swap_region (CoglOnscreen *onscreen,
>                                        rect[0], rect[1], x2, y2,
>                                        GL_COLOR_BUFFER_BIT, GL_NEAREST);
>          }
> -      context->glDrawBuffer (GL_BACK);
> +      context->glDrawBuffer (context->current_gl_draw_buffer);
>      }
>
>    /* NB: unlike glXSwapBuffers, glXCopySubBuffer and
> diff --git a/examples/Makefile.am b/examples/Makefile.am
> index b7cccec..01cf003 100644
> --- a/examples/Makefile.am
> +++ b/examples/Makefile.am
> @@ -33,7 +33,7 @@ cogl_info_SOURCES = cogl-info.c
>  cogl_info_LDADD = $(common_ldadd)
>
>  if USE_GLIB
> -programs += cogl-hello cogl-msaa cogl-gles2-context cogl-point-sprites
> +programs += cogl-hello cogl-msaa cogl-gles2-context cogl-point-sprites cogl-stereo
>  examples_datadir = $(pkgdatadir)/examples-data
>  examples_data_DATA =
>
> @@ -43,6 +43,8 @@ cogl_msaa_SOURCES = cogl-msaa.c
>  cogl_msaa_LDADD = $(common_ldadd)
>  cogl_point_sprites_SOURCES = cogl-point-sprites.c
>  cogl_point_sprites_LDADD = $(common_ldadd)
> +cogl_stereo_SOURCES = cogl-stereo.c
> +cogl_stereo_LDADD = $(common_ldadd)

It looks like this is meant to be split out in a different patch


On the whole this patch seems fairly straightforward and ok to me.

--
Thanks
Robert