Re: [E-devel] [E-Devel][Review][Patch] Evas GL Extensions + a bug fix

[Carsten H. (T. R. <ra...@ra...>]

On Mon, 17 Oct 2011 17:25:17 +0900 "Sung W. Park" <su...@gm...> said:

-> IN <-

:)

> Hi Carsten,
> 
> Nice!!  My patch has seen the light finally =)
> 
> I'll respond to the comments below.
> 
> On Mon, Oct 17, 2011 at 3:16 PM, Carsten Haitzler <ra...@ra...>
> wrote:
> > On Thu, 13 Oct 2011 16:04:40 +0900 "Sung W. Park" <su...@gm...> said:
> >
> > comments:
> >
> > 1. after much thought i think keeping glgetprocaddess wrapping will be best.
> > people just are used to that and it makes "passthrough" mode (no virtual
> > context on top of the real gl) simpler and easier. for actual egl calls we
> > HAVE to provide wrapped func pointers though as evas takes over the job of
> > egl itself and may/does have a differing api as a result. providing actual
> > already wrapped and exposed function in the struct though is always useful
> > and makes life much easier/less work for programmers, so keep those, just
> > also allow getting of proc address for gl extensions (egl - no. need to
> > wrap here - stil have in strings though).
> 
> I agree with you on glgetprocaddress.  This will make things a lot easier in
> the future dealing with some random extensions that need to be supported on
> some specific hardware.
> 
> >
> > 2. some small formatting things in there like
> >
> > +typedef void*                         EvasGLImage;
> >
> > * next to Evas not void. :)
> 
> This has been fixed. I've noticed the same issue with Evas_GL_Func so I've
> changed that one too. I'll keep that in mind.=)
> 
> >
> > 3. why for 1 egl api "extension" you have:
> > +   void         (*glEvasGLImageTargetTexture2DOES) (GLenum target,
> > EvasGLImage image);
> > +   void         (*glEvasGLImageTargetRenderbufferStorageOES) (GLenum
> > target, EvasGLImage image);
> > vs
> > +   EvasGLImage  (*evasglCreateImage) (int target, void* buffer, int*
> > attrib_list);
> > +   void         (*evasglDestroyImage) (EvasGLImage image);
> >
> > notice glEvasGL vs evasgl naming... pick one and stick to it :)
> 
> This one was a little tricky but I purposely kept the naming like that.  in
> EGL, the extension APIs are eglCreateImage and glEGLImageTarget...  and I can
> see why they did it like that.  The reason is because eglCreateImage is an EGL
> extension that doesn't have direct impact on the gl APIs.  glEGLImageTarget...
> function, however, is pretty much a glBindTexture that gets applied to
> EGLImage.  So I thought that following the GL convention on those two APIs was
> reasonable.
> 
> >
> > that's actually about it. fix those and we're golden.
> 
> Ok, attaching a new patch.  Hopefully this email will go through a little
> quicker this time :)
> 
> cheers, Sung
> 
> >
> >> Hi again,
> >>
> >> Since my patch hasn't been reviewed yet, I'm sending in my latest version
> >> of the evasgl.  This way, it makes my repository more in sync with the
> >> trunk. :-)
> >>
> >> So, basically, you can ignore the previous two patches.
> >>
> >> Here's what the latest patches do:
> >>
> >> - Extension support in Evas GL.
> >>    After much discussion, we've done away with get_proc_address.  Instead,
> >> the extensions are listed in the Evas_GL_API struct.  There are
> >> definitely pros and
> >>    cons to both approaches.  I think I've highlighted some of that in
> >> the previous
> >>    email.  I'm also including a simple example program that uses KHR_image
> >>    extension.  you'll need to run in an egl environment with that extension
> >>    supported in order to see it running.  Also, the patch includes a bug
> >> fix. There was no NULL check for config in evas_gl_surface_create so that
> >> has been added.
> >>
> >>    - evas_extensions.patch
> >>    - evasgl_extensions1.c
> >>
> >> - Resource context per thread using TLS:
> >>    EvasGL uses textures/fbo for its render target surface.  Hence, if you
> >>    want to create a surface before you create a texture, you need a current
> >>    context to create it.  We took care of this by creating a resource
> >> context. Unfortunately, if you want to launch a thread for GL rendering,
> >> you need to have a separate context per thread since a context can't be
> >> made current in two separate threads at the same time.  So, I've created a
> >> resource context per thread using TLS.  Since there is no TLS support in
> >> Eina, I've added 4 APIs for that as well.  Another patch has been
> >> submitted but i'll just include it in here as well.
> >>
> >>    - eina_tls.patch
> >>
> >>
> >> Let me know what you think.
> >>
> >> cheers,
> >> Sung
> >>
> >>
> >> On Tue, Oct 4, 2011 at 8:19 PM, Sung W. Park <su...@gm...> wrote:
> >> > Hi all,
> >> >
> >> > I actually introduced a slight bug in my patch.  I forgot to check if for
> >> > NULL on two variables in the eng_make_current.  That's been fixed.  I'm
> >> > attaching a new one.  Sorry about that =)
> >> >
> >> > cheers,
> >> > Sung
> >> >
> >> >
> >> > On Fri, Sep 30, 2011 at 11:08 AM, Sung W. Park <su...@gm...> wrote:
> >> >> Hello all,
> >> >>
> >> >> Here's an initial attempt at the GL extensions issue for Evas GL.
> >> >>
> >> >> I have been in discussion with a few EFL developers regarding how we
> >> >> should provide extensions.  Essentially, there are two ways to go about
> >> >> doing this.
> >> >>
> >> >> 1. provide evas_gl_proc_address_get() function as it is done in other
> >> >> glue layers
> >> >>
> >> >> 2. provide all the extension functions in the EVAS_GL_API struct.
> >> >>
> >> >> #1 approach is how it's done in other glue layers and the driver
> >> >> #implementor can
> >> >> provide new extensions easily.  It is however pretty annoying to get the
> >> >> function prototypes right and use the function pointers and etc.
> >> >>
> >> >> #2 approach provides all the extension functions in the struct so it's
> >> >> definitely easier to use.  Adding new extensions can be a pain as people
> >> >> may have to wait for new version releases.
> >> >>
> >> >> For now, we thought it was OK to just throw them in the struct as in #2
> >> >> and try it out.  So, I've implemented this approach.  As for the
> >> >> extensions, I've basically included all the extensions in gl2ext.h as
> >> >> EvasGL currently provides all the GLES 2.0 functions.   In order to
> >> >> display the right information, I had to override glGetString() with
> >> >> GL_EXTENSIONS as parameter to properly display the supported extensions.
> >> >>
> >> >> Also, I've added a few EGL extensions that have been
> >> >> modified for EvasGL use.  For example, eglCreateImage/eglDestroyImage
> >> >> has been defined as folllows.
> >> >>
> >> >>   EvasGLImage  (*evasglCreateImage) (int target, void* buffer, int*
> >> >>                                      attrib_list); void
> >> >>   (*evasglDestroyImage) (EvasGLImage image);
> >> >>
> >> >> const char *evas_gl_string_query() function was added to return a
> >> >> string of supported EvasGL extensions.  So essentially, a user can
> >> >> search this string to see
> >> >> if the desired extension is supported.  if it is, he can use the
> >> >> functions.  He can
> >> >> always check if the function pointers are NULL as well.
> >> >>
> >> >> Take a look at the pach and let me know what you think.
> >> >>
> >> >> ______________
> >> >>
> >> >> While I was adding the extension code, I've added a few fixes/ changes
> >> >> to the EvasGL.
> >> >>
> >> >> 1. glDeletBuffers bug
> >> >> - When I wad destroying evasgl context, I was deleting the context FBO
> >> >> with glDeleteBuffers instead of glDeleteFramebuffers. This code in
> >> >> effect was deleting BOs in other contexts and we had some funky
> >> >> behaviors as a result.  The bug has been fixed.
> >> >>
> >> >> 2. make_current
> >> >> - I've made some changes to the make current code and also added a
> >> >> resource context to the engine data.  the resource context is used for
> >> >> creating surface texture/ fbos when surface/ context is created.
> >> >> Before, i was using evas' context but thought it'd be a good idea to
> >> >> use a separate context.
> >> >>
> >> >> ______________
> >> >>
> >> >>
> >> >> cheers,
> >> >> Sung
> >> >>
> >> >
> >
> >
> > --
> > ------------- Codito, ergo sum - "I code, therefore I am" --------------
> > The Rasterman (Carsten Haitzler)    ra...@ra...
> >
> >


-- 
------------- Codito, ergo sum - "I code, therefore I am" --------------
The Rasterman (Carsten Haitzler)    ra...@ra...