[Mesa-dev] GLX_USE_APPLEGL

[Jeremy Huddleston]

Hi Kristian,

Thanks for your comments.  I sent out a few requests to the mailing list a while back.  I just took the silence as "whatever.  it doesn't really affect us."  ... that being said, I actually share your concern about this #ifdef-crud.  My main reason for integrating it as-is was to avoid spending days rebasing it the next time something big happens (like the directory change).  Unfortunately, my focus has been diverted elsewhere over the past few months, so I wasn't able to clean this all up as I had hoped by now.

The main problem was that George forked from Mesa-7.2 and didn't preserve any non-Apple code.  It took me quite a while to re-integrate these changes to this point, and hopefully things will get a bit smoother as I refactor it.

George decided to use a separate dispatch table for the gl functions, which has served us in the short term, but now that I'm re-integrating these changes, I want to rewrite our dispatch code to use the existing dispatch.  This should then let us eliminate more of these ifdefs and also start including support for indirect.

I like the idea of using a similar approach for the glX functions, but for now, I'm going to try making the GLX_USE_APPLEGL way more similar to the !GLX_USE_APPLEGL way.

I'm sorry for any difficulty this has caused.  Feel free to "break" Apple and ping me to fix our case.

Thanks,
Jeremy

On May 20, 2010, at 06:24, Kristian Høgsberg wrote:

> Hi Jeremy,
> 
> First of all, let me say that I'm sorry I didn't review your work
> before it was committed, that would have been the right time to
> address this.  Anyway, my problem is that the addition of even more
> #ifdef's to the glx client code makes an already tangled code base
> harder to read and work with.  Even worse, a lot of the new #ifndef
> GLX_USE_APPLEGL is of the form
> 
> PUBLIC void
> glXCopyContext(Display * dpy, GLXContext source,
>               GLXContext dest, unsigned long mask)
> {
> #ifdef GLX_USE_APPLEGL
>  /* Apple GL code */
> #else
>  /* What was there before */
> #endif
> }
> 
> which is just screaming out for a vtable implementation.  There are
> places where code is shared, but it's often just a small snippet like
> if (!dpy) return NULL; or it's a bigger piece of code that can be put
> in a common function and called from the different implementations.
> 
> Of course, the existing code already did this to some extent with the
> direct/indirect code paths.  Most of these cases look like
> 
> #if defined(GLX_DIRECT_RENDERING)
>   if (gc->driContext) {
>      /* direct rendering case */
>   }
>   /* fall through to indirect case */
> #endif
> 
>   /* indirect case */
> 
> which would be handled just fine by the vtable approach as well: at
> context creation time, we set the context vtable pointer to the
> direct, indirect or applegl functions and this will just work.  I
> wanted to do some of this in the past, but I didn't feel like sinking
> too much work into the glx code before the license change.
> 
> Now, I'm not saying that we need to do this refactoring now - it's
> certainly not a priority for me right now.  But as a minimum, I'd like
> us to be on the same page with where this code should be, and
> hopefully we can work towards a cleaner code base.  FIguring out
> what's going on in any of the glx entrypoints is just too hard right
> now with all the #ifdef spaghetti.
> 
> Kristian