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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
