[Mesa-dev] [PATCH] mesa: initialize renderbuffer fields even if AllocStorage fails

[Brian Paul]

On 05/12/2012 10:11 AM, Marek Olšák wrote:
> It may fail with an unsupported format, but that's not an allowed case where
> RenderbufferStorage may fail.

I've read your comment several times but I still can't quite 
understand the whole issue.

The intention was that AllocStorage() should never fail because of the 
internalFormat value.  We do error checking of internalFormat earlier 
in renderbuffer_storage().  Then the driver chooses a hw format that 
best matches the requested internalFormat.

If the driver's AllocStorage() function fails because it ran out of 
memory, it should record GL_OUT_OF_MEMORY.  Though, it looks like 
swrast does that but st_renderbuffer_alloc_storage() doesn't. :(

The GL spec says that the behaviour of a function is undefined only 
for the GL_OUT_OF_MEMORY error (so I guess the Rb's 
width/height/format fields could be set to anything in that situation, 
but setting them to zero seems more reasonable to me).  But when other 
GL errors are generated it should be as if the function is a no-op.

 From section 2.5 GL Errors:
"""
     Table 2.3 summarizes GL errors. Currently, when an error flag is 
set, results of
GL operation are undefined only if OUT OF MEMORY has occurred. In 
other cases,
the command generating the error is ignored so that it has no effect 
on GL state or
framebuffer contents.
"""

So an error related to the internalFormat shouldn't change the Rb's 
width/height/depth.


> And on success, we should return valid values
> from GetRenderbufferParameteriv.

Of course. :)


>
> This fixes piglit: get-renderbuffer-internalformat

Which driver/format?  It passes for llvmpipe and svga for me.

-Brian


> ---
>   src/mesa/main/fbobject.c |   18 +++++++++++++-----
>   1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/src/mesa/main/fbobject.c b/src/mesa/main/fbobject.c
> index 777783e..c67b5be 100644
> --- a/src/mesa/main/fbobject.c
> +++ b/src/mesa/main/fbobject.c
> @@ -1402,13 +1402,21 @@ renderbuffer_storage(GLenum target, GLenum internalFormat,
>         assert(rb->_BaseFormat != 0);
>      }
>      else {
> -      /* Probably ran out of memory - clear the fields */
> -      rb->Width = 0;
> -      rb->Height = 0;
> +      /* Probably ran out of memory - set the fields anyway, because
> +       * the ARB_fbo spec says:
> +       *
> +       *   "Upon success, ... RENDERBUFFER_WIDTH is set to<width>,
> +       *    RENDERBUFFER_HEIGHT is set to<height>, and
> +       *    RENDERBUFFER_INTERNAL_FORMAT is set to<internalformat>."
> +       *
> +       * Assuming the "success" means no GL errors.
> +       */
> +      rb->Width = width;
> +      rb->Height = height;
>         rb->Format = MESA_FORMAT_NONE;
> -      rb->InternalFormat = GL_NONE;
> +      rb->InternalFormat = internalFormat;
>         rb->_BaseFormat = GL_NONE;
> -      rb->NumSamples = 0;
> +      rb->NumSamples = samples;
>      }
>
>      /* Invalidate the framebuffers the renderbuffer is attached in. */