[Mesa-dev] [PATCH] mesa: refactor target error checking in glGetTexLevelParameter

[Timothy Arceri]

On Thu, 2015-08-13 at 15:01 +0300, Tapani Pälli wrote:
> 
> On 08/13/2015 02:51 PM, Timothy Arceri wrote:
> > On Thu, 2015-08-13 at 14:05 +0300, Tapani Pälli wrote:
> > > With non-dsa functions we need to do target error checking before
> > > _mesa_get_current_tex_object which would just call _mesa_problem without
> > > raising GL_INVALID_ENUM error. In other places of Mesa, target gets 
> > > checked
> > > before this call. For dsa functions, we do not need this check. Texture
> > > has been created already with a proper target value.
> > > 
> > > Fixes failures in:
> > >     ES31-CTS.texture_storage_multisample.APIGLGetTexLevelParameterifv.*
> > > 
> > > Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
> > > ---
> > >   src/mesa/main/texparam.c | 26 +++++++++++++++++++-------
> > >   1 file changed, 19 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/src/mesa/main/texparam.c b/src/mesa/main/texparam.c
> > > index c0611c3..81c209f 100644
> > > --- a/src/mesa/main/texparam.c
> > > +++ b/src/mesa/main/texparam.c
> > > @@ -1562,6 +1562,19 @@ invalid_pname:
> > >                  _mesa_enum_to_string(pname));
> > >   }
> > > 
> > > +static bool
> > > +valid_tex_level_parameteriv_target(struct gl_context *ctx, GLenum 
> > > target,
> > > +                                   bool dsa)
> > > +{
> > > +   const char *suffix = dsa ? "ture" : "";
> > > +   if (!legal_get_tex_level_parameter_target(ctx, target, dsa)) {
> > > +      _mesa_error(ctx, GL_INVALID_ENUM,
> > > +                  "glGetTex%sLevelParameter[if]v(target=%s)", suffix,
> > > +                  _mesa_enum_to_string(target));
> > > +      return false;
> > > +   }
> > > +   return true;
> > > +}
> > > 
> > >   /**
> > >    * This isn't exposed to the rest of the driver because it is a part 
> > > of the
> > > @@ -1585,13 +1598,6 @@ get_tex_level_parameteriv(struct gl_context *ctx,
> > >         return;
> > >      }
> > > 
> > > -   if (!legal_get_tex_level_parameter_target(ctx, target, dsa)) {
> > > -      _mesa_error(ctx, GL_INVALID_ENUM,
> > > -                  "glGetTex%sLevelParameter[if]v(target=%s)", suffix,
> > > -                  _mesa_enum_to_string(target));
> > > -      return;
> > > -   }
> > > -_mesa_GetTexLevelParameterfv( GLenum target, GLint
> > > level,
> > >      GLint iparam;
> > >      GET_CURRENT_CONTEXT(ctx);
> > > 
> > > +   if (!valid_tex_level_parameteriv_target(ctx, target, false))
> > > +      return;
> > > +
> > >      texObj = _mesa_get_current_tex_object(ctx, target);
> > >      if (!texObj)
> > >         return;
> > > @@ -1636,6 +1645,9 @@ _mesa_GetTexLevelParameteriv( GLenum target, GLint
> > > level,
> > >      struct gl_texture_object *texObj;
> > >      GET_CURRENT_CONTEXT(ctx);
> > > 
> > > +   if (!valid_tex_level_parameteriv_target(ctx, target, false))
> > > +      return;
> > > +
> > >      texObj = _mesa_get_current_tex_object(ctx, target);
> > >      if (!texObj)
> > >         return;
> > 
> > 
> > I think you forgot to add calls to the new validation for the dsa 
> > functions.
> > 
> 
> Nope, the motivation for leaving out the checks for dsa functions is 
> that the texture target enum has been validated already for existing 
> textures, it cannot be invalid at this point. At least this is how I 
> understand it.

I don't think thats right. The dsa functions are just passed a texture id, the
id may find you a texture that exists but you need to make sure the texture is
a valid type for use with the LevelParameter*


> 
> // Tapani