[Cogl] [PATCH/RFC] Adds initial GLES2 integration support

Neil Roberts neil at linux.intel.com
Thu Mar 15 15:12:53 PDT 2012


Here's some minor feedback:

> +  context->gles2_context_stack = g_queue_new ();
> +

GQueue is documented as being OK to allocate on the stack so I guess you
could put it directly inline in the context and avoid this separate
allocation.

> +      (ctx->have_last_offscreen_allocate_flags &&
> +       try_creating_fbo (ctx,
> +                         offscreen->texture,
> +                         offscreen->texture_level,
> +                         offscreen->texture_level_width,
> +                         offscreen->texture_level_height,
> +                         &fb->config,
> +                         ctx->last_offscreen_allocate_flags,
> +                         gl_framebuffer)) ||

This needs to set flags = ctx->last_offscreen_allocate_flags, otherwise
flags will be left as 0 and later it will reset
lsat_offscreen_allocate_flags to 0 too.

> +/* When drawing to a CoglFramebuffer from separate context we have
> + * to be able to allocate ancillary buffers for that context...
> + */
> +static CoglGLES2Framebuffer *
> +_cogl_gles2_framebuffer_allocate (CoglFramebuffer *framebuffer,
> +                                  CoglGLES2Context *gles2_context,
> +                                  GError **error)
> +{
...
> +  gles2_framebuffer->original = cogl_object_ref (framebuffer);

It doesn't seem ideal that the context permanently keeps alive any
framebuffers that is has ever been used with. Could it instead register
a weak reference with cogl_object_set_user_data and remove the
framebuffer from the list when it is destroyed?

> +/**
> + * cogl_push_gles2_context:

We should maybe also document here that it's invalid to use regular Cogl
calls while a GLES2 context is pushed.

> +/* We wrap glReadPixels so when framebuffer 0 is bound then we can
> + * read from the read_framebuffer passed to cogl_push_gles2_context().
> + */
> +static void
> +gl_read_pixels_wrapper (GLint x,

We probably would also need to do this for glCopyTexImage2D and
glCopyTexSubImage2D, right?

I guess it would be nice to have a conformance test at some point.

Otherwise it looks really good to me.

Regards,
- Neil


More information about the Cogl mailing list