[Mesa-dev] [PATCH] st/mesa: make unsupported renderbuffer formats always fail as FBO incomplete

[Brian Paul]

On Thu, Jun 14, 2012 at 7:11 PM, Marek Olšák <maraeo at gmail.com> wrote:
> instead of failing to allocate a renderbuffer. This also improves allocation
> of MSAA renderbuffers (untested).
>
> This also fixes piglit/get-renderbuffer-internalformat with non-renderable
> formats.
> ---
>  src/mesa/state_tracker/st_cb_fbo.c |   63 +++++++++++++++++++++++++++++++-----
>  1 file changed, 55 insertions(+), 8 deletions(-)
>
> diff --git a/src/mesa/state_tracker/st_cb_fbo.c b/src/mesa/state_tracker/st_cb_fbo.c
> index 953295c..5a21c8e 100644
> --- a/src/mesa/state_tracker/st_cb_fbo.c
> +++ b/src/mesa/state_tracker/st_cb_fbo.c
> @@ -72,8 +72,9 @@ st_renderbuffer_alloc_storage(struct gl_context * ctx,
>    struct pipe_context *pipe = st->pipe;
>    struct pipe_screen *screen = st->pipe->screen;
>    struct st_renderbuffer *strb = st_renderbuffer(rb);
> -   enum pipe_format format;
> +   enum pipe_format format = PIPE_FORMAT_NONE;
>    struct pipe_surface surf_tmpl;
> +   unsigned nr_samples = 0;
>
>    if (internalFormat == GL_RGBA16_SNORM && strb->software) {
>       /* Special case for software accum buffers.  Otherwise, if the
> @@ -84,12 +85,55 @@ st_renderbuffer_alloc_storage(struct gl_context * ctx,
>       format = PIPE_FORMAT_R16G16B16A16_SNORM;
>    }
>    else {
> -      format = st_choose_renderbuffer_format(screen, internalFormat,
> -                                             rb->NumSamples);
> -   }
> +      /* Handle multisample renderbuffers first.
> +       *
> +       * From ARB_framebuffer_object:
> +       *   If <samples> is zero, then RENDERBUFFER_SAMPLES is set to zero.
> +       *   Otherwise <samples> represents a request for a desired minimum
> +       *   number of samples. Since different implementations may support
> +       *   different sample counts for multisampled rendering, the actual
> +       *   number of samples allocated for the renderbuffer image is
> +       *   implementation dependent.  However, the resulting value for
> +       *   RENDERBUFFER_SAMPLES is guaranteed to be greater than or equal
> +       *   to <samples> and no more than the next larger sample count supported
> +       *   by the implementation.
> +       *
> +       * So let's find the supported number of samples closest to NumSamples.
> +       * (NumSamples == 1) is treated the same as (NumSamples == 0).
> +       */
> +      if (rb->NumSamples > 1) {
> +         unsigned i;
>
> -   if (format == PIPE_FORMAT_NONE) {
> -      return FALSE;
> +         for (i = rb->NumSamples; i < ctx->Const.MaxSamples; i++) {
> +            format = st_choose_renderbuffer_format(screen, internalFormat, i);
> +
> +            if (format != PIPE_FORMAT_NONE) {
> +               rb->NumSamples = i;
> +               nr_samples = i;
> +               break;
> +            }
> +         }
> +      } else {
> +         format = st_choose_renderbuffer_format(screen, internalFormat, 0);
> +      }
> +
> +      /* There is no supported renderable pipe format for internalformat.
> +       * AllocStorage shouldn't fail because of an unsupported format,
> +       * instead, unsupported formats should cause FBO incompleteness
> +       * later, so let's set the closest sampler format we can get.
> +       * This will also cause FBO incompleteness for unsupported sample
> +       * counts, because the validation uses rb->NumSamples.
> +       */
> +      if (format == PIPE_FORMAT_NONE) {
> +         format = st_choose_format(screen, internalFormat, GL_NONE, GL_NONE,
> +                                   PIPE_TEXTURE_2D, 0,
> +                                   PIPE_BIND_SAMPLER_VIEW);
> +      }
> +
> +      assert(format != PIPE_FORMAT_NONE);
> +      if (format == PIPE_FORMAT_NONE) {
> +         return FALSE; /* shouldn't happen unless there's a bug somewhere */
> +      }
>    }
>
>    /* init renderbuffer fields */
> @@ -134,7 +178,7 @@ st_renderbuffer_alloc_storage(struct gl_context * ctx,
>       template.depth0 = 1;
>       template.array_size = 1;
>       template.last_level = 0;
> -      template.nr_samples = rb->NumSamples;
> +      template.nr_samples = nr_samples;
>       if (util_format_is_depth_or_stencil(format)) {
>          template.bind = PIPE_BIND_DEPTH_STENCIL;
>       }
> @@ -455,9 +499,12 @@ st_validate_attachment(struct gl_context *ctx,
>       format = st_mesa_format_to_pipe_format(linearFormat);
>    }
>
> +   /* Check Renderbuffer->NumSamples (which is what the user expects) and
> +    * not pipe_resource::nr_samples (which might be zero if NumSamples is
> +    * unsupported). */
>    return screen->is_format_supported(screen, format,
>                                       PIPE_TEXTURE_2D,
> -                                      stObj->pt->nr_samples, bindings);
> +                                      att->Renderbuffer->NumSamples, bindings);
>  }

I don't think this last bit of code will be reached for non-texture
attachments (which are allocated with RenderbufferStorage()).  At the
beginning of st_validate_attachment() we check if the attachment type
is not a texture and return.  I guess we could remove that check, but
then we need to get rid of stObj and get the attachment's format from
st_renderbuffer::texture->format, etc.

Just curious: have you tried NVIDIA's or AMD's driver to see what
happens when you try to allocate an unsupported renderbuffer format?

-Brian