[Mesa-dev] [PATCHES] implement GL_ARB_robustness

[Brian Paul]

On 04/19/2011 02:43 PM, Ian Romanick wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 04/19/2011 12:23 PM, nobled wrote:
>> spec file:
>> http://www.opengl.org/registry/specs/ARB/robustness.txt
>>
>> The first four parts of this series add infrastructure to support
>> bounds-checking client memory buffers by re-using the PBO
>> bounds-checking code; the fifth patch adds the actual functions of the
>> extension. However, the series is incomplete because I'm not sure how
>> to turn the spec file into an xml file for
>> src/mapi/glapi/gen/ARB_robustness.xml, and then generate the dispatch
>> stuff from it with the python script(*which* python script?) -- the
>> docs on this point seem to be a classic combination of completely
>> outdated and "hey, can you vague that up for me?":
>>> http://cgit.freedesktop.org/mesa/mesa/tree/docs/devinfo.html
>>>    In the src/mesa/glapi/ directory, add the new extension functions and
>>>    enums to the gl_API.xml file.
>>>    Then, a bunch of source files must be regenerated by executing the
>>>    corresponding Python scripts.

It's not too hard to write the xml file.  Just use another extension 
as an example (ex: ARB_sampler_objects.xml which was just recently 
added).  Then add the new filename to the Makefile and in gl_API.xml, 
then run the Makefile.  Make commits for the stuff you added/changed 
and another for the regenerated files.


>> I think I covered all the other steps, though. At least enough to be
>> faithful to the spec, as long as these two aren't exposed yet on the
>> GLX side of things:
>> http://www.opengl.org/registry/specs/ARB/glx_create_context.txt
>> http://www.opengl.org/registry/specs/ARB/glx_create_context_robustness.txt
>>
>> Still todo: adding piglit tests.
>
> It's *REALLY* hard to review patches sent as attachments.  It is much
> better and easier for reviewers to put comments alongside the code they
> are commenting about.  In the future, could you please send patches
> using git-send-email?
>
> Patch 1/5 looks like it's doing to separate things.  It renames / alters
> _mesa_validate_pbo_access and it starts using _mesa_is_bufferobj in some
> places.  That should probably be split into two patches.  First change
> over to using _mesa_is_bufferobj, then make the other changes.
>
> I'm also not sure _mesa_validate_buffer_access is a particularly good
> name.   This loses the information that the validation is for pixel
> reads / writes.  This also applies to _mesa_map_validate_buffer_dest and
> friends in later patches.

I agree.  I'd like to keep pbo in the name to indicate that we're 
talking about pixel/image buffers.  OpenGL has many kinds of buffers 
and it's good to clarify what kind of buffer we're talking about here.


> For the internal Mesa functions, I think I might also change the
> parameter bufSize to clientMemorySize or similar.  Using bufSize in the
> API functions (added in patch 5/5) matches the API definition and is fine.
>
> The changes to readpix.c in patch 4/5 looks odd.  If the access is
> out-of-bounds and there is PBO, nothing happens but no error is
> generated.  Is that what the spec says to do?
>
> It also looks like a lot of the checking for out-of-bounds followed by
> checking for being mapped could be refactored.
>
> Patch 5/5 also does two separate things.  It adds data structure
> infrastructure for the extension and it adds a bunch of entry points.
> Usually gl_context::ResetStatus and gl_constants::ResetStratgy would be
> added in a separate patch from adding all the new functions.

I agree with these items too.


> In patch 5/5 why
>
> +_mesa_GetMinmax(GLenum target, GLboolean reset, GLenum format, GLenum type,
> +                GLvoid *values)
> +{
> +   const GLsizei bufSize = INT_MAX;
> +   _mesa_GetnMinmaxARB(target, reset, format, type, bufSize, values);
> +}
>
> instead of
>
> +_mesa_GetMinmax(GLenum target, GLboolean reset, GLenum format, GLenum type,
> +                GLvoid *values)
> +{
> +   _mesa_GetnMinmaxARB(target, reset, format, type, INT_MAX, values);
> +}

I sometimes do that to convey what the argument is without having to 
look at the prototype.  In this case though, it doesn't add a lot.

-Brian