[Mesa-dev] [PATCH 1/2] mesa: Fix texture target validation for glFramebufferTexture()

[Ian Romanick]

On 11/19/2013 11:02 PM, Paul Berry wrote:
> Previously we were using the code path for validating
> glFramebufferTextureLayer().  But glFramebufferTexture() allows
> additional texture types.
> 
> Fixes piglit tests:
> - spec/!OpenGL 3.2/layered-rendering/gl-layer-cube-map
> - spec/!OpenGL 3.2/layered-rendering/framebuffertexture
> 
> Cc: "10.0" <mesa-stable at lists.freedesktop.org>
> ---
>  src/mesa/main/fbobject.c | 50 ++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 40 insertions(+), 10 deletions(-)
> 
> diff --git a/src/mesa/main/fbobject.c b/src/mesa/main/fbobject.c
> index 9dd7161..2feb4c3 100644
> --- a/src/mesa/main/fbobject.c
> +++ b/src/mesa/main/fbobject.c
> @@ -2334,16 +2334,46 @@ framebuffer_texture(struct gl_context *ctx, const char *caller, GLenum target,

As an aside, it's confusing, but correct, that layered is set to true by
a caller without Layer in the name.

>        texObj = _mesa_lookup_texture(ctx, texture);
>        if (texObj != NULL) {
>           if (textarget == 0) {
> -            /* If textarget == 0 it means we're being called by
> -             * glFramebufferTextureLayer() and textarget is not used.
> -             * The only legal texture types for that function are 3D and
> -             * 1D/2D arrays textures.
> -             */

So this comment was just wrong?  What does textarget == 0 mean?

> -            err = (texObj->Target != GL_TEXTURE_3D) &&
> -                (texObj->Target != GL_TEXTURE_1D_ARRAY_EXT) &&
> -                (texObj->Target != GL_TEXTURE_2D_ARRAY_EXT) &&
> -                (texObj->Target != GL_TEXTURE_CUBE_MAP_ARRAY) &&
> -                (texObj->Target != GL_TEXTURE_2D_MULTISAMPLE_ARRAY);
> +            if (layered) {
> +               /* We're being called by glFramebufferTexture() and textarget
> +                * is not used.
> +                */
> +               switch (texObj->Target) {
> +               case GL_TEXTURE_3D:
> +               case GL_TEXTURE_1D_ARRAY_EXT:
> +               case GL_TEXTURE_2D_ARRAY_EXT:
> +               case GL_TEXTURE_CUBE_MAP:
> +               case GL_TEXTURE_CUBE_MAP_ARRAY:
> +               case GL_TEXTURE_2D_MULTISAMPLE_ARRAY:
> +                  err = false;
> +                  break;
> +               case GL_TEXTURE_1D:
> +               case GL_TEXTURE_2D:
> +               case GL_TEXTURE_RECTANGLE:
> +               case GL_TEXTURE_2D_MULTISAMPLE:
> +                  /* These texture types are valid to pass to
> +                   * glFramebufferTexture(), but since they aren't layered, it
> +                   * is equivalent to calling glFramebufferTexture{1D,2D}().
> +                   */
> +                  err = false;

If this is changed to

    err = !layered;

then...

> +                  layered = false;
> +                  textarget = texObj->Target;
> +                  break;
> +               default:
> +                  err = true;
> +                  break;
> +               }
> +            } else {
> +               /* We're being called by glFramebufferTextureLayer() and
> +                * textarget is not used.  The only legal texture types for
> +                * that function are 3D and 1D/2D arrays textures.
> +                */
> +               err = (texObj->Target != GL_TEXTURE_3D) &&
> +                  (texObj->Target != GL_TEXTURE_1D_ARRAY_EXT) &&
> +                  (texObj->Target != GL_TEXTURE_2D_ARRAY_EXT) &&
> +                  (texObj->Target != GL_TEXTURE_CUBE_MAP_ARRAY) &&
> +                  (texObj->Target != GL_TEXTURE_2D_MULTISAMPLE_ARRAY);

I think this could be handled by the switch-statement above.

If that happens, I'd also recommend dropping _EXT from some of the case
labels.

> +            }
>           }
>           else {
>              /* Make sure textarget is consistent with the texture's type */
>