[Mesa-dev] [PATCH 02/15] mesa: Share common code between ARB_debug_output and KHR_debug functions

[Timothy Arceri]

On Wed, 2013-09-04 at 19:58 -0700, Ian Romanick wrote:
> On 09/04/2013 04:46 PM, Timothy Arceri wrote:
> >> Since the calling functions all should alias, all of this should
> >> be removed too.
> > 
> > The functions have to do different validations on types they are not 
> > exactly the same. If there is some way to signal this to be done 
> > using an alias can you please give me some advice on how to do this 
> > so I can learn.
> 
> The general guideline is that if an extension (or core GL version) only
> adds functionality, the functions should alias.  In other words, alias
> if the new thing is a proper superset of the old thing.

Ok makes sense. I have a half finished document about how to implement
an extension in Mesa based on my experience with this extension.
Basically the idea is to give a brief introduction to the Mesa codebase
to enable potential Mesa contributors to get a quicker overview of the
infrastructure stuff and enable them to get to work faster on the lower
level (fun) stuff (at least that's the idea). Anyway this information is
something useful I can add thanks.

> 
> In this case, all of the functionality in the ARB_debug_output is
> unchanged in KHR_debug.  Both
> 
>     glDebugMessageControl(GL_DEBUG_SOURCE_THIRD_PARTY,
>                           GL_DONT_CARE, GL_DONT_CARE, 0,
>                           NULL, GL_FALSE);
> 
> and
> 
>     glDebugMessageControlARB(GL_DEBUG_SOURCE_THIRD_PARTY_ARB,
>                              GL_DONT_CARE, GL_DONT_CARE, 0,
>                              NULL, GL_FALSE);
> 
> behave identically.
> 
> In addition, notice that GL_DEBUG_CALLBACK_FUNCTION_ARB is the same as
> GL_DEBUG_CALLBACK_FUNCTION, and GL_DEBUG_CALLBACK_USER_PARAM_ARB is the
> same as GL_DEBUG_CALLBACK_USER_PARAM.  If glDebugMessageCallbackARB and
> glDebugMessageCallback are different, which state does
> glGetPointerv(GL_DEBUG_CALLBACK_FUNCTION) return?

Well the spec says "Only one debug callback can be specified for the
current context, and further calls overwrite the previous callback." 

So I took that to mean that we should only keep track of a single
callback regardless of which extension set it.

> 
> It used to be easier in the days when specs still defined GLX protocol.
>  If two functions had the same protocol opcode assigned, they had to alias.
> 
> We (I) have made mistakes the other direction in the past.  In Mesa 9.1
> and earlier, glBindFramebufferEXT and glBindFramebuffer alias, but they
> should not.
> 
> Let's be cautious... let's poke at the AMD and NVIDIA drivers to see
> what they do.  It should be easy enough to figure out if they use the
> same implementation for the KHR and ARB functions.

Sounds like a good idea. I was going to check what AMD to begin with but
as they only seem to enable these functions when the context flag is set
I didn't bother as I couldn't figure out how to easily get piglit to set
this flag. 

> 
> There's a Khronos face-to-face next week, and I'll raise this as a bug.
>  KHR_debug really should have listed interactions with ARB_debug.
> 
> >> I think this entire patch should be removed.
> > 
> > I'm trying to stay positive be its hard when your emails and review
> > have an obvious overtone of annoyance.
> 
> Several people reviewed the patch series, and yet a bunch of obvious
> problems slipped through.  *That* is very annoying.
> 
> > The entire patch is not junk. For example the changes to the
> 
> Well... I don't think I said "junk."  I don't think there's anything in
> the series that is junk, per se.  There are a bunch of little things
> that should have been noticed and fixed before the code landed.
> 
> > validation function are still valid as mentioned above however you
> > want to call them, and the generic message_insert function is reused
> > by the push/pop implementation. If answer my question from above I
> > can create a new patch.
> 
> To me, this sounds like the patch does a bunch of different things...
> and it should have been split into multiple patches.  Can I add that to
> the list of things the reviewers didn't catch? :)