[Mesa-dev] [PATCH V3 11/19] mesa: support multisample textures in framebuffer completeness check

[Chris Forbes]

Yikes, apparently I missed one. Thanks for spotting that.

-- Chris

On Tue, Feb 12, 2013 at 3:35 PM, Eric Anholt <eric at anholt.net> wrote:
> Chris Forbes <chrisf at ijw.co.nz> writes:
>
>> - sample count must be the same on all attachments
>> - fixedsamplepositions must be the same on all attachments
>> (renderbuffers have fixedsamplepositions=true implicitly; only
>> multisample textures can choose to have it false)
>>
>> V2: - fix wrapping to 80 columns, debug message, fix for state moving
>>       from texobj to image.
>>     - stencil texturing tweaks tidied up and folded in here.
>>
>> V3: - Removed silly stencil hacks entirely; the extension doesn't
>>       actually make stencil-only textures legal at all.
>>     - Moved sample count / fixed sample locations checks into
>>       existing attachment-type-specific blocks, as suggested by Eric
>>
>> Signed-off-by: Chris Forbes <chrisf at ijw.co.nz>
>> [V2] Reviewed-by: Paul Berry <stereotype441 at gmail.com>
>> ---
>>  src/mesa/main/fbobject.c | 60 ++++++++++++++++++++++++++++++++++--------------
>>  1 file changed, 43 insertions(+), 17 deletions(-)
>>
>> diff --git a/src/mesa/main/fbobject.c b/src/mesa/main/fbobject.c
>> index d9fd78e..46663dd 100644
>> --- a/src/mesa/main/fbobject.c
>> +++ b/src/mesa/main/fbobject.c
>> @@ -664,8 +664,11 @@ test_attachment_completeness(const struct gl_context *ctx, GLenum format,
>>               baseFormat == GL_DEPTH_STENCIL_EXT) {
>>              /* OK */
>>           }
>> +         else if (ctx->Extensions.ARB_texture_multisample &&
>> +               baseFormat == GL_STENCIL_INDEX) {
>> +            /* OK */
>> +         }
>>           else {
>> -            /* no such thing as stencil-only textures */
>>              att_incomplete("illegal stencil texture");
>>              att->Complete = GL_FALSE;
>>              return;
>
> Hey!  You said you removed the silly stencil hacks!  :)
>
>> @@ -745,6 +748,7 @@ _mesa_test_framebuffer_completeness(struct gl_context *ctx,
>>     GLenum intFormat = GL_NONE; /* color buffers' internal format */
>>     GLuint minWidth = ~0, minHeight = ~0, maxWidth = 0, maxHeight = 0;
>>     GLint numSamples = -1;
>> +   GLint fixedSampleLocations = -1;
>>     GLint i;
>>     GLuint j;
>>
>> @@ -766,6 +770,7 @@ _mesa_test_framebuffer_completeness(struct gl_context *ctx,
>>        struct gl_renderbuffer_attachment *att;
>>        GLenum f;
>>        gl_format attFormat;
>> +      struct gl_texture_image *texImg = NULL;
>>
>>        /*
>>         * XXX for ARB_fbo, only check color buffers that are named by
>> @@ -805,8 +810,7 @@ _mesa_test_framebuffer_completeness(struct gl_context *ctx,
>>        /* get width, height, format of the renderbuffer/texture
>>         */
>>        if (att->Type == GL_TEXTURE) {
>> -         const struct gl_texture_image *texImg =
>> -            _mesa_get_attachment_teximage(att);
>> +         texImg = _mesa_get_attachment_teximage(att);
>>           minWidth = MIN2(minWidth, texImg->Width);
>>           maxWidth = MAX2(maxWidth, texImg->Width);
>>           minHeight = MIN2(minHeight, texImg->Height);
>
> I think the texImg declaration change can be reverted now.
>
>> @@ -814,12 +818,29 @@ _mesa_test_framebuffer_completeness(struct gl_context *ctx,
>>           f = texImg->_BaseFormat;
>>           attFormat = texImg->TexFormat;
>>           numImages++;
>> +
>>           if (!is_format_color_renderable(ctx, attFormat, texImg->InternalFormat) &&
>>               !is_legal_depth_format(ctx, f)) {
>>              fb->_Status = GL_FRAMEBUFFER_INCOMPLETE_FORMATS_EXT;
>>              fbo_incomplete("texture attachment incomplete", -1);
>>              return;
>>           }
>> +
>> +         if (numSamples < 0)
>> +            numSamples = texImg->NumSamples;
>> +         else if (numSamples != texImg->NumSamples) {
>> +            fb->_Status = GL_FRAMEBUFFER_INCOMPLETE_MULTISAMPLE;
>> +            fbo_incomplete("inconsistent sample count", -1);
>> +            return;
>> +         }
>> +
>> +         if (fixedSampleLocations < 0)
>> +            fixedSampleLocations = texImg->FixedSampleLocations;
>> +         else if (fixedSampleLocations != texImg->FixedSampleLocations) {
>> +            fb->_Status = GL_FRAMEBUFFER_INCOMPLETE_MULTISAMPLE;
>> +            fbo_incomplete("inconsistent fixed sample locations", -1);
>> +            return;
>> +         }
>>        }
>>        else if (att->Type == GL_RENDERBUFFER_EXT) {
>>           minWidth = MIN2(minWidth, att->Renderbuffer->Width);
>> @@ -829,24 +850,35 @@ _mesa_test_framebuffer_completeness(struct gl_context *ctx,
>>           f = att->Renderbuffer->InternalFormat;
>>           attFormat = att->Renderbuffer->Format;
>>           numImages++;
>> +
>> +         if (numSamples < 0)
>> +            numSamples = att->Renderbuffer->NumSamples;
>> +         else if (numSamples != att->Renderbuffer->NumSamples) {
>> +            fb->_Status = GL_FRAMEBUFFER_INCOMPLETE_MULTISAMPLE;
>> +            fbo_incomplete("inconsistent sample count", -1);
>> +            return;
>> +         }
>> +
>> +         /* RENDERBUFFER has fixedSampleLocations implicitly true */
>> +         if (fixedSampleLocations < 0)
>> +            fixedSampleLocations = GL_TRUE;
>> +         else if (fixedSampleLocations != GL_TRUE) {
>> +            fb->_Status = GL_FRAMEBUFFER_INCOMPLETE_MULTISAMPLE;
>> +            fbo_incomplete("inconsistent fixed sample locations", -1);
>> +            return;
>> +         }
>
> This looks like what I was thinking.
>
> Other than the 2 trivial comments,
>
> Reviewed-by: Eric Anholt <eric at anholt.net>