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

[Paul Berry]

On 20 November 2013 11:13, Ian Romanick <idr at freedesktop.org> wrote:

> 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.
>

Yeah, that's unfortunate.  I'm open to suggestions about how to make the
code less confusing.


>
> >        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?
>

The comment was correct before support for layered framebuffers was added.
Now, it's only correct if layered is false.  That's why I moved it (with a
slight amount of rewording) to the else block below.

As to what textarget == 0 means: the way this function works is that
framebuffer_texture() is called by all the various glFramebufferTexture*()
functions.  textarget == 0 means that it was called from a function that
doesn't take a textarget parameter (glFramebufferTextureLayer() or
glFramebufferTexture()).


>
> > -            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.
>

Yeah, but it gets a little ugly beccause GL_TEXTURE_CUBE_MAP is accepted by
glFramebufferTexture() but not glFramebufferTextureLayer().  I'm also not
thrilled about the fact that it mixes together the error checking for two
functions in the same switch statement, so it's a little harder to follow
what's going on.

Here's what the switch statement looks like if I coalesce it together, like
you suggest:

            /* If textarget == 0 it means we're either being called by
             * glFramebufferTextureLayer() (layered == false) or
             * glFramebufferTexture() (layered == true) and textarget is not
             * used.
             */
            switch (texObj->Target) {
            case GL_TEXTURE_3D:
            case GL_TEXTURE_1D_ARRAY:
            case GL_TEXTURE_2D_ARRAY:
            case GL_TEXTURE_CUBE_MAP_ARRAY:
            case GL_TEXTURE_2D_MULTISAMPLE_ARRAY:
               /* These texture types are accepted by both
                * glFramebufferTexture() and glFramebufferTextureLayer().
                */
               err = false;
               break;
            case GL_TEXTURE_CUBE_MAP:
               /* GL_TEXTURE_CUBE_MAP is accepted by glFramebufferTexture()
                * but not glFramebufferTextureLayer().
                */
               err = !layered;
               break;
            case GL_TEXTURE_1D:
            case GL_TEXTURE_2D:
            case GL_TEXTURE_RECTANGLE:
            case GL_TEXTURE_2D_MULTISAMPLE:
               /* These texture types are accepted by
glFramebufferTexture(),
                * but since they aren't layered, it is equivalent to calling
                * glFramebufferTexture{1D,2D}().  They aren't accepted by
                * glFramebufferTextureLayer().
                */
               err = !layered;
               layered = false;
               textarget = texObj->Target;
               break;
            default:
               err = true;
               break;
            }

If you think it's better this way, I guess I could be talked into changing
it, but IMHO it's not a clear improvement.


>
> > +            }
> >           }
> >           else {
> >              /* Make sure textarget is consistent with the texture's
> type */
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-stable/attachments/20131120/658fa749/attachment-0001.html>