On 08/23/2012 12:00 AM, Kenneth Graunke wrote:
On 08/22/2012 07:26 PM, Ian Romanick wrote:
From: Ian Romanick <ian.d.roman...@intel.com>
Signed-off-by: Ian Romanick <ian.d.roman...@intel.com>
---
src/mesa/main/varray.c | 28 +++++++++++++++++++---------
1 files changed, 19 insertions(+), 9 deletions(-)
diff --git a/src/mesa/main/varray.c b/src/mesa/main/varray.c
index 327fabb..36d0eb8 100644
--- a/src/mesa/main/varray.c
+++ b/src/mesa/main/varray.c
@@ -133,16 +133,26 @@ update_array(struct gl_context *ctx,
GLsizei elementSize;
GLenum format = GL_RGBA;
- if (ctx->API != API_OPENGLES && ctx->API != API_OPENGLES2) {
- /* fixed point arrays / data is only allowed with OpenGL ES 1.x/2.0 */
+ if (ctx->API == API_OPENGLES || ctx->API == API_OPENGLES2) {
if (_mesa_is_gles(ctx)) {
Right. Most of the patches in this series are pretty old, so they
predate the existence of that function. I converted most, but I
obviously missed some.
+ /* Once Mesa gets support for GL_OES_vertex_half_float this mask will
+ * change. Adding support for this extension isn't quite as trivial as
+ * we'd like because ES uses a different enum value for GL_HALF_FLOAT.
+ */
+ legalTypesMask &= ~(FIXED_GL_BIT
+ | UNSIGNED_INT_2_10_10_10_REV_BIT
+ | INT_2_10_10_10_REV_BIT
+ | HALF_BIT | DOUBLE_BIT);
+ } else {
+ assert(ctx->API == API_OPENGL);
I believe this assertion is wrong. Both VertexAttribPointer and
VertexAttribIPointer call update_arrays, and neither of those functions
are deprecated. So it's possible to get here from API_OPENGL_CORE.
I would just remove it.
Yeah. When I wrote that assertion there was no API_OPENGL_CORE.
+
legalTypesMask &= ~FIXED_ES_BIT;
- }
- if (!ctx->Extensions.ARB_ES2_compatibility) {
- legalTypesMask &= ~FIXED_GL_BIT;
- }
- if (!ctx->Extensions.ARB_vertex_type_2_10_10_10_rev) {
- legalTypesMask &= ~(UNSIGNED_INT_2_10_10_10_REV_BIT |
- INT_2_10_10_10_REV_BIT);
+
+ if (!ctx->Extensions.ARB_ES2_compatibility)
+ legalTypesMask &= ~FIXED_GL_BIT;
+
+ if (!ctx->Extensions.ARB_vertex_type_2_10_10_10_rev)
+ legalTypesMask &= ~(UNSIGNED_INT_2_10_10_10_REV_BIT |
+ INT_2_10_10_10_REV_BIT);
}
typeBit = type_to_bit(ctx, type);
This is also not a mere refactor---unless I'm reading something wrong,
it changes behavior rather substantially.
Consider the old code:
if (ctx->API != API_OPENGLES && ctx->API != API_OPENGLES2) {
/* fixed point arrays / data is only allowed with OpenGL ES 1.x/2.0 */
legalTypesMask &= ~FIXED_ES_BIT;
}
if (!ctx->Extensions.ARB_ES2_compatibility) {
legalTypesMask &= ~FIXED_GL_BIT;
}
In the API_OPENGL case, the first conditional would mask out
FIXED_ES_BIT, disallowing it even if ARB_ES2_compatibility was
supported. Your new code allows it, which is a bug fix. (The
ARB_ES2_compatibility spec explicitly allows GL_FIXED with
VertexAttribPointer.)
Not exactly. FIXED_ES_BIT and FIXED_GL_BIT are generated depending on
the context API.
case GL_FIXED:
return _mesa_is_desktop_gl(ctx) ? FIXED_GL_BIT : FIXED_ES_BIT;
In a non-ES context, FIXED_ES_BIT should be impossible, and likewise for
FIXED_GL_BIT in an ES context. It's weird, and I don't understand why
it is the way it is. I didn't want to change it in this series, and I
forgot about it after writing these patches.
In the API_OPENGLES (1 or 2) case, if the driver supports
ARB_ES2_compatibility, the second conditional would /still/ mask out
FIXED_ES_BIT, disallowing it even on ES! Your new code correctly allows
that (another serious bug fix).
So unless I'm misreading it, the old code was just completely broken
regarding GL_FIXED data, and your new code is reasonable and fixes it.
I believe at least a comment in the commit message is in order; maybe
even a mark for stable? (Unless GL_FIXED is just totally broken
elsewhere and allowing it through here causes worse bugs...)
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
