[Mesa-dev] [PATCH 4/5] mesa: Implement functions for clear_buffer_object extensions

[Pi Tabred]

On 11.12.2013 16:51, Brian Paul wrote:
> On 12/11/2013 05:21 AM, Pi Tabred wrote:
>>
>>
>> On 10.12.2013 17:44, Brian Paul wrote:
>>> On 12/10/2013 06:13 AM, Pi Tabred wrote:
>>>> ...
>>>> +      }
>>>> +   }
>>>> +
>>>> +   if (!_mesa_is_color_format(format)) {
>>>> +      _mesa_error(ctx, GL_INVALID_ENUM,
>>>> +                  "glClearBufferData(no color format)");
>>>> +   }
>>>
>>> Are you sure about that error?  Where is this mentioned in the spec?
>>> Plus, you're missing a 'return' statement.
>>
>> I am not sure, but is it possible to convert data from a depth/stencil
>> format to a color format? If not, the statement
>>
>> "INVALID_ENUM is generated by ClearBufferSubData if <format> or <type>
>> is not one of the supported format or type tokens."
>>
>> could be interpreted as to include this error as only certain color
>> formats are allowed for the internalformat.
> 
> OK, I've read the spec, and I think you're right.  But the code as-is
> may need a few more changes.
> 
> Table 3.15 of the 4.2 spec lists a subset of internal formats compared
> to what we check for in get_texbuffer_format().  The 3.15 table doesn't
> contain luminance or intensity values.  Our code might be wrong.  It
> looks like luminance/intensity formats aren't legal for texture buffers
> in core profiles.
> 
> I think the call to _mesa_is_color_format() is too lenient since it
> accepts compressed formats, etc.  _mesa_error_check_format_and_type()
> might not catch all the invalid formats either.  <sigh> format/type
> error checking is such a hassle.  I think you should write some piglit
> tests that exercise invalid format/type/internalformat combinations to
> make sure that we really catch these invalid combinations.
> 
> -Brian
> 

all luminance, intensity and alpha formats are removed in core profiles.
It's good that you mention this, that's something I wanted to ask about.
My solution would be to check in get_texbuffer_format() if it's a core
context and if that's the case return MESA_FORMAT_NONE for these formats.

with regards to your second point, I have to disagree. you are right
that _mesa_is_color_format() is too lenient, but the combination with
_mesa_error_check_format_and_type makes it work as compressed formats
are not allowed for the format parameter.
I already wrote some piglit tests for the format/type combination (see
piglit mailing list) but I can extend them.