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

[Robert Bragg]

On Thu, Mar 15, 2012 at 10:12 PM, Neil Roberts <neil at linux.intel.com> wrote:

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

sure, that makes sense.


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

ah woops good catch.


>
> > +/* 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?
>

yeah I'll change this.


>
> > +/**
> > + * 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.
>

right, I only stated that you can't use the gles2 outside of a push/pop
pair.


>
> > +/* 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?


yeah, I should add wrappers for those too.


>


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

Yeah I'm not entirely sure about how to do this. I was thinking that
ideally we would be able to use the Khronos GLES2 conformance tests. I was
looking at their test code the other day and wondering if that would be
doable. Sadly it would mean that others wouldn't be able to reproduce our
test environment unless they were also Khronos members. I wonder if piglit
has good gles2 api coverage?

For either of these options we would probably need to create a cogl-gles2
sub-library that exposes the full GLES2 api as public symbols to ease being
able to run existing test code. The only thing we'd have to adapt is how
EGL is used.

Whether we want this fronted cogl-gles2 sub-library/api or not is something
we should think about before landing this patch probably, because if we do
then maybe it should be re-factored now to live under a cogl-gles2
directory and we should possible adapt the naming of the new symbols to be
in a cogl_gles2_ namespace.


> Otherwise it looks really good to me.


cool, thanks for the review.




> Regards,
> - Neil
> _______________________________________________
> Cogl mailing list
> Cogl at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/cogl
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/cogl/attachments/20120316/6069400e/attachment-0001.htm>