[Mesa-dev] [PATCH 01/10] swrast: Add LUMINANCE, INTENSITY, LUMINANCE_ALPHA to span asserts.

[Brian Paul]

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 Anholt<eric at anholt.net>  wrote:
>>> On Mon, 18 Apr 2011 16:16:37 -0700, Ian Romanick<idr at 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