Re: [Mesa-dev] [PATCH 01/10] swrast: Add LUMINANCE, INTENSITY, LUMINANCE_ALPHA to span asserts.
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 04/18/2011 08:43 PM, Brian Paul wrote: On Mon, Apr 18, 2011 at 8:10 PM, Eric Anholt e...@anholt.net wrote: On Mon, 18 Apr 2011 16:16:37 -0700, Ian Romanick i...@freedesktop.org wrote: -BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 04/18/2011 01:37 PM, Eric Anholt wrote: Fixes ARB_texture_float/fbo-alphatest-formats. --- src/mesa/swrast/s_readpix.c |3 +++ src/mesa/swrast/s_span.c|3 +++ 2 files changed, 6 insertions(+), 0 deletions(-) diff --git a/src/mesa/swrast/s_readpix.c b/src/mesa/swrast/s_readpix.c index 5604c2e..a201a63 100644 --- a/src/mesa/swrast/s_readpix.c +++ b/src/mesa/swrast/s_readpix.c @@ -195,6 +195,9 @@ fast_read_rgba_pixels( struct gl_context *ctx, rb-_BaseFormat == GL_RGB || rb-_BaseFormat == GL_RG || rb-_BaseFormat == GL_RED || + rb-_BaseFormat == GL_LUMINANCE || + rb-_BaseFormat == GL_INTENSITY || + rb-_BaseFormat == GL_LUMINANCE_ALPHA || rb-_BaseFormat == GL_ALPHA); At this point would it be easier to just assert the formats that are not allowed? Is there even anything that's left as a valid _BaseFormat that isn't allowed here? I keep wanting to remove asserts like this, and Brian says he likes them. I'm sorry you feel inconvenienced, but I'm a big believer in assertions. I don't think the problem is the existence of assertions. I think the problem is that the format assertions have gotten a bit out of control. They're increasingly difficult to maintain, and we often get false negatives. This diminishes their value significantly. That was my point in the initial review comment. Once the assertion is expanded to be correct, it basically asserts that _BaseFormat must have a value. Since this should have been handled elsewhere, does having it here add anything other than maintenance burden? I also think that many of these assertions could be improved by checking for the negative condition instead of the positive condition. There are far fewer values that cannot be used than there are values that can be used. When someone is adding support for formats GL_FOO, it is more clear that assert(x != GL_FOO x != GL_BAR); needs to be changed than assert(x == GL_ASDF || x == GL_QWERTY); In these recent cases, perhaps we just need a better, extendable assertion. Something like assert(_mesa_is_base_color_format(rb-_BaseFormat)); That would certainly be more maintainable for this case, but I don't think it's a general fix. I think someone will have to look over this code and come up with a plan. 1, 2, 3, not it! :) -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/ iEYEARECAAYFAk2t07YACgkQX1gOwKyEAw8IKACcCNkd99BcCttmiBq4ekDezSED fjkAniwnDIZPPjW1VPQNwKZyVO4xv1TK =mzoJ -END PGP SIGNATURE- ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 01/10] swrast: Add LUMINANCE, INTENSITY, LUMINANCE_ALPHA to span asserts.
On 04/19/2011 12:25 PM, Ian Romanick wrote: -BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 04/18/2011 08:43 PM, Brian Paul wrote: On Mon, Apr 18, 2011 at 8:10 PM, Eric Anholte...@anholt.net wrote: On Mon, 18 Apr 2011 16:16:37 -0700, Ian Romanicki...@freedesktop.org wrote: -BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 04/18/2011 01:37 PM, Eric Anholt wrote: Fixes ARB_texture_float/fbo-alphatest-formats. --- src/mesa/swrast/s_readpix.c |3 +++ src/mesa/swrast/s_span.c|3 +++ 2 files changed, 6 insertions(+), 0 deletions(-) diff --git a/src/mesa/swrast/s_readpix.c b/src/mesa/swrast/s_readpix.c index 5604c2e..a201a63 100644 --- a/src/mesa/swrast/s_readpix.c +++ b/src/mesa/swrast/s_readpix.c @@ -195,6 +195,9 @@ fast_read_rgba_pixels( struct gl_context *ctx, rb-_BaseFormat == GL_RGB || rb-_BaseFormat == GL_RG || rb-_BaseFormat == GL_RED || + rb-_BaseFormat == GL_LUMINANCE || + rb-_BaseFormat == GL_INTENSITY || + rb-_BaseFormat == GL_LUMINANCE_ALPHA || rb-_BaseFormat == GL_ALPHA); At this point would it be easier to just assert the formats that are not allowed? Is there even anything that's left as a valid _BaseFormat that isn't allowed here? I keep wanting to remove asserts like this, and Brian says he likes them. I'm sorry you feel inconvenienced, but I'm a big believer in assertions. I don't think the problem is the existence of assertions. I think the problem is that the format assertions have gotten a bit out of control. They're increasingly difficult to maintain, and we often get false negatives. This diminishes their value significantly. Yes, I understand the downside of that. That was my point in the initial review comment. Once the assertion is expanded to be correct, it basically asserts that _BaseFormat must have a value. I'm sorry to be pedantic but that's not accurate. Illegal values for _BaseFormat at this point include GL_STENCIL_INDEX, GL_DEPTH_COMPONENT, GL_DEPTH_STENCIL, GL_COLOR_INDEX and GL_DUDV_ATI. That's 5 illegal cases vs. 8 legal so it's not a huge difference. Since this should have been handled elsewhere, does having it here add anything other than maintenance burden? That's just it, trying to read a non-color renderbuffer as a color format _should_ have been caught earlier (GL_INVALID_OPERATION). The point of the assertion is to double-check that proper error detection was done earlier - and, to tell the reader that the subsequent code is prepared to handle these formats only. If that assertions fails, the subsequent code should be reviewed to see if it can handle the new format. I also think that many of these assertions could be improved by checking for the negative condition instead of the positive condition. There are far fewer values that cannot be used than there are values that can be used. When someone is adding support for formats GL_FOO, it is more clear that assert(x != GL_FOO x != GL_BAR); needs to be changed than assert(x == GL_ASDF || x == GL_QWERTY); It could go either way. In the case above, if GL_FOO is a new non-color format you'd still need to update assertions. In these recent cases, perhaps we just need a better, extendable assertion. Something like assert(_mesa_is_base_color_format(rb-_BaseFormat)); That would certainly be more maintainable for this case, but I don't think it's a general fix. I think someone will have to look over this code and come up with a plan. 1, 2, 3, not it! :) I'd rather not make a mountain out of a molehill. We've already spent more time typing emails about the topic than just fixing the assertions. -Brian ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 01/10] swrast: Add LUMINANCE, INTENSITY, LUMINANCE_ALPHA to span asserts.
Fixes ARB_texture_float/fbo-alphatest-formats.
---
src/mesa/swrast/s_readpix.c |3 +++
src/mesa/swrast/s_span.c|3 +++
2 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/src/mesa/swrast/s_readpix.c b/src/mesa/swrast/s_readpix.c
index 5604c2e..a201a63 100644
--- a/src/mesa/swrast/s_readpix.c
+++ b/src/mesa/swrast/s_readpix.c
@@ -195,6 +195,9 @@ fast_read_rgba_pixels( struct gl_context *ctx,
rb-_BaseFormat == GL_RGB ||
rb-_BaseFormat == GL_RG ||
rb-_BaseFormat == GL_RED ||
+ rb-_BaseFormat == GL_LUMINANCE ||
+ rb-_BaseFormat == GL_INTENSITY ||
+ rb-_BaseFormat == GL_LUMINANCE_ALPHA ||
rb-_BaseFormat == GL_ALPHA);
/* clipping should have already been done */
diff --git a/src/mesa/swrast/s_span.c b/src/mesa/swrast/s_span.c
index b0f8e49..f0524e0 100644
--- a/src/mesa/swrast/s_span.c
+++ b/src/mesa/swrast/s_span.c
@@ -1352,6 +1352,9 @@ _swrast_read_rgba_span( struct gl_context *ctx, struct
gl_renderbuffer *rb,
rb-_BaseFormat == GL_RGB ||
rb-_BaseFormat == GL_RG ||
rb-_BaseFormat == GL_RED ||
+rb-_BaseFormat == GL_LUMINANCE ||
+rb-_BaseFormat == GL_INTENSITY ||
+rb-_BaseFormat == GL_LUMINANCE_ALPHA ||
rb-_BaseFormat == GL_ALPHA);
if (rb-DataType == dstType) {
--
1.7.4.4
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 01/10] swrast: Add LUMINANCE, INTENSITY, LUMINANCE_ALPHA to span asserts.
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
On 04/18/2011 01:37 PM, Eric Anholt wrote:
Fixes ARB_texture_float/fbo-alphatest-formats.
---
src/mesa/swrast/s_readpix.c |3 +++
src/mesa/swrast/s_span.c|3 +++
2 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/src/mesa/swrast/s_readpix.c b/src/mesa/swrast/s_readpix.c
index 5604c2e..a201a63 100644
--- a/src/mesa/swrast/s_readpix.c
+++ b/src/mesa/swrast/s_readpix.c
@@ -195,6 +195,9 @@ fast_read_rgba_pixels( struct gl_context *ctx,
rb-_BaseFormat == GL_RGB ||
rb-_BaseFormat == GL_RG ||
rb-_BaseFormat == GL_RED ||
+ rb-_BaseFormat == GL_LUMINANCE ||
+ rb-_BaseFormat == GL_INTENSITY ||
+ rb-_BaseFormat == GL_LUMINANCE_ALPHA ||
rb-_BaseFormat == GL_ALPHA);
At this point would it be easier to just assert the formats that are not
allowed? Is there even anything that's left as a valid _BaseFormat that
isn't allowed here?
/* clipping should have already been done */
diff --git a/src/mesa/swrast/s_span.c b/src/mesa/swrast/s_span.c
index b0f8e49..f0524e0 100644
--- a/src/mesa/swrast/s_span.c
+++ b/src/mesa/swrast/s_span.c
@@ -1352,6 +1352,9 @@ _swrast_read_rgba_span( struct gl_context *ctx, struct
gl_renderbuffer *rb,
rb-_BaseFormat == GL_RGB ||
rb-_BaseFormat == GL_RG ||
rb-_BaseFormat == GL_RED ||
+ rb-_BaseFormat == GL_LUMINANCE ||
+ rb-_BaseFormat == GL_INTENSITY ||
+ rb-_BaseFormat == GL_LUMINANCE_ALPHA ||
rb-_BaseFormat == GL_ALPHA);
if (rb-DataType == dstType) {
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
iEYEARECAAYFAk2sxlUACgkQX1gOwKyEAw8CFQCfVFntgCi2e3E97sQvxbJfTeNg
wbcAnjxmN88BbETnE5P73CpJrhG6xsLT
=14pX
-END PGP SIGNATURE-
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 01/10] swrast: Add LUMINANCE, INTENSITY, LUMINANCE_ALPHA to span asserts.
On Mon, 18 Apr 2011 16:16:37 -0700, Ian Romanick i...@freedesktop.org wrote: -BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 04/18/2011 01:37 PM, Eric Anholt wrote: Fixes ARB_texture_float/fbo-alphatest-formats. --- src/mesa/swrast/s_readpix.c |3 +++ src/mesa/swrast/s_span.c|3 +++ 2 files changed, 6 insertions(+), 0 deletions(-) diff --git a/src/mesa/swrast/s_readpix.c b/src/mesa/swrast/s_readpix.c index 5604c2e..a201a63 100644 --- a/src/mesa/swrast/s_readpix.c +++ b/src/mesa/swrast/s_readpix.c @@ -195,6 +195,9 @@ fast_read_rgba_pixels( struct gl_context *ctx, rb-_BaseFormat == GL_RGB || rb-_BaseFormat == GL_RG || rb-_BaseFormat == GL_RED || + rb-_BaseFormat == GL_LUMINANCE || + rb-_BaseFormat == GL_INTENSITY || + rb-_BaseFormat == GL_LUMINANCE_ALPHA || rb-_BaseFormat == GL_ALPHA); At this point would it be easier to just assert the formats that are not allowed? Is there even anything that's left as a valid _BaseFormat that isn't allowed here? I keep wanting to remove asserts like this, and Brian says he likes them. pgpCgm1GbhQnP.pgp Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 01/10] swrast: Add LUMINANCE, INTENSITY, LUMINANCE_ALPHA to span asserts.
On Mon, Apr 18, 2011 at 8:10 PM, Eric Anholt e...@anholt.net wrote: On Mon, 18 Apr 2011 16:16:37 -0700, Ian Romanick i...@freedesktop.org wrote: -BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 04/18/2011 01:37 PM, Eric Anholt wrote: Fixes ARB_texture_float/fbo-alphatest-formats. --- src/mesa/swrast/s_readpix.c | 3 +++ src/mesa/swrast/s_span.c | 3 +++ 2 files changed, 6 insertions(+), 0 deletions(-) diff --git a/src/mesa/swrast/s_readpix.c b/src/mesa/swrast/s_readpix.c index 5604c2e..a201a63 100644 --- a/src/mesa/swrast/s_readpix.c +++ b/src/mesa/swrast/s_readpix.c @@ -195,6 +195,9 @@ fast_read_rgba_pixels( struct gl_context *ctx, rb-_BaseFormat == GL_RGB || rb-_BaseFormat == GL_RG || rb-_BaseFormat == GL_RED || + rb-_BaseFormat == GL_LUMINANCE || + rb-_BaseFormat == GL_INTENSITY || + rb-_BaseFormat == GL_LUMINANCE_ALPHA || rb-_BaseFormat == GL_ALPHA); At this point would it be easier to just assert the formats that are not allowed? Is there even anything that's left as a valid _BaseFormat that isn't allowed here? I keep wanting to remove asserts like this, and Brian says he likes them. I'm sorry you feel inconvenienced, but I'm a big believer in assertions. In these recent cases, perhaps we just need a better, extendable assertion. Something like assert(_mesa_is_base_color_format(rb-_BaseFormat)); -Brian ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
