<br><br><div class="gmail_quote">On Thu, Mar 15, 2012 at 10:12 PM, Neil Roberts <span dir="ltr"><<a href="mailto:neil@linux.intel.com">neil@linux.intel.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Here's some minor feedback:<br> <div class="im"><br> > + context->gles2_context_stack = g_queue_new ();<br> > +<br> <br> </div>GQueue is documented as being OK to allocate on the stack so I guess you<br> could put it directly inline in the context and avoid this separate<br> allocation.<br></blockquote><div><br></div><div>sure, that makes sense.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div class="im"><br> > + (ctx->have_last_offscreen_allocate_flags &&<br> > + try_creating_fbo (ctx,<br> > + offscreen->texture,<br> > + offscreen->texture_level,<br> > + offscreen->texture_level_width,<br> > + offscreen->texture_level_height,<br> > + &fb->config,<br> > + ctx->last_offscreen_allocate_flags,<br> > + gl_framebuffer)) ||<br> <br> </div>This needs to set flags = ctx->last_offscreen_allocate_flags, otherwise<br> flags will be left as 0 and later it will reset<br> lsat_offscreen_allocate_flags to 0 too.<br></blockquote><div><br></div><div>ah woops good catch.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div class="im"><br> > +/* When drawing to a CoglFramebuffer from separate context we have<br> > + * to be able to allocate ancillary buffers for that context...<br> > + */<br> > +static CoglGLES2Framebuffer *<br> > +_cogl_gles2_framebuffer_allocate (CoglFramebuffer *framebuffer,<br> > + CoglGLES2Context *gles2_context,<br> > + GError **error)<br> > +{<br> </div>...<br> <div class="im">> + gles2_framebuffer->original = cogl_object_ref (framebuffer);<br> <br> </div>It doesn't seem ideal that the context permanently keeps alive any<br> framebuffers that is has ever been used with. Could it instead register<br> a weak reference with cogl_object_set_user_data and remove the<br> framebuffer from the list when it is destroyed?<br></blockquote><div><br></div><div>yeah I'll change this.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <br> > +/**<br> > + * cogl_push_gles2_context:<br> <br> We should maybe also document here that it's invalid to use regular Cogl<br> calls while a GLES2 context is pushed.<br></blockquote><div><br></div><div>right, I only stated that you can't use the gles2 outside of a push/pop pair.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div class="im"><br> > +/* We wrap glReadPixels so when framebuffer 0 is bound then we can<br> > + * read from the read_framebuffer passed to cogl_push_gles2_context().<br> > + */<br> > +static void<br> > +gl_read_pixels_wrapper (GLint x,<br> <br> </div>We probably would also need to do this for glCopyTexImage2D and<br> glCopyTexSubImage2D, right?</blockquote><div><br></div><div>yeah, I should add wrappers for those too.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> </blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <br> I guess it would be nice to have a conformance test at some point.<br></blockquote><div><br></div><div>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?</div> <div><br></div><div>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.</div> <div><br></div><div>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.</div> <div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <br> Otherwise it looks really good to me.</blockquote><div><br></div><div>cool, thanks for the review. </div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> </blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <br> Regards,<br> - Neil<br> _______________________________________________<br> Cogl mailing list<br> <a href="mailto:Cogl@lists.freedesktop.org">Cogl@lists.freedesktop.org</a><br> <a href="http://lists.freedesktop.org/mailman/listinfo/cogl" target="_blank">http://lists.freedesktop.org/mailman/listinfo/cogl</a><br> </blockquote></div><br>