[Mesa-dev] [PATCH 3/3 v3] mesa: Fix GL error generation in glBlitFramebuffer()

[Kenneth Graunke]

On 01/08/2013 02:03 AM, Anuj Phogat wrote:
> On Tue, Jan 8, 2013 at 5:25 AM, Kenneth Graunke <kenneth at whitecape.org> wrote:
>> From: Anuj Phogat <anuj.phogat at gmail.com>
>>
>> V2:
>> If mask has GL_STENCIL_BUFFER_BIT set, the depth formats for readRenderBuffer
>> and drawRenderBuffer must match unless one of the two buffers doesn't have
>> depth, in which case it's not blitted, so the format check should be ignored.
>> Same comment goes for stencil formats in depth renderbuffers if mask has
>> GL_DEPTH_BUFFER_BIT set.
>>
>> v3 (Kayden): Refactor code to be a bit more readable.
>>
>> Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
>> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
>> ---
>>   src/mesa/main/fbobject.c | 59 ++++++++++++++++++++++++++++++++++++------------
>>   1 file changed, 44 insertions(+), 15 deletions(-)
>>
>> Anuj,
>>
>> This looks correct, but the run-on conditionals are pretty hard to read...
>> just too much going on in a single if-statement.
>>
>> How about splitting them up a bit?  Here's my proposed version.  It also
>> gives slightly more descriptive messages.
>>
> I'm happy with the refactored code except a small mistake I explained below.
>> Untested.
>>
>> diff --git a/src/mesa/main/fbobject.c b/src/mesa/main/fbobject.c
>> index 281cdd0..bf613ff 100644
>> --- a/src/mesa/main/fbobject.c
>> +++ b/src/mesa/main/fbobject.c
>> @@ -2833,14 +2833,28 @@ _mesa_BlitFramebuffer(GLint srcX0, GLint srcY0, GLint srcX1, GLint srcY1,
>>         if ((readRb == NULL) || (drawRb == NULL)) {
>>           mask &= ~GL_STENCIL_BUFFER_BIT;
>>         }
>> -      else if (_mesa_get_format_bits(readRb->Format, GL_STENCIL_BITS) !=
>> -              _mesa_get_format_bits(drawRb->Format, GL_STENCIL_BITS)) {
>> -        /* There is no need to check the stencil datatype here, because
>> -         * there is only one: GL_UNSIGNED_INT.
>> -         */
>> -         _mesa_error(ctx, GL_INVALID_OPERATION,
>> -                     "glBlitFramebufferEXT(stencil buffer size mismatch)");
>> -         return;
>> +      else {
>> +         if ((_mesa_get_format_bits(readRb->Format, GL_STENCIL_BITS) !=
>> +              _mesa_get_format_bits(drawRb->Format, GL_STENCIL_BITS)) ||
>> +             (_mesa_get_format_datatype(readRb->Format) !=
>> +              _mesa_get_format_datatype(drawRb->Format))) {
> Matching the data type is not required here as data type for stencil buffer is
> always GL_UNSIGNED_INT. But, data type of GL_DEPTH24_STENCIL8 is
> GL_UNSIGNED_NORMALIZED. So, matching the data types here will disallow
> blitting between GL_STENCIL_INDEX8 and GL_DEPTH24_STENCIL8 which is a
> valid blitting case. This caused 7 gles3 conformance blitframebuffer test cases
> fail.

Oops.  I thought that would be harmless, since it'd always be the same 
in the stencil-only case, and needed to match in the depth-only case. 
But you're right.  Sorry for the trouble.

I'm fine with you pushing your series now (other than my comments on 7.1/7).