[Mesa-dev] [PATCH 1/3] mesa: Improve validation of target, format and type of glTexSubImage[2, 3]D

[Eduardo Lima Mitev]

Thanks for the review! Please see some comments inline:

On 03/23/2015 06:53 PM, Laura Ekstrand wrote:
> I'm glad you found this target checking problem.  I have come across the
> same problem in other texture functions, but I have been too busy to
> find all instances of this.  It's kind of a natural, negative byproduct
> of adding DSA.
> 
> [..]
> 
> I recommend getting rid of the legal_texsubimage_target check in
> texsubimage_error_check and moving it up into texturesubimage, right
> after the texObj has been retrieved. Otherwise in the texsubimage case,
> the target will get checked twice, once in texsubimage and once in
> texsubimage-error-check.  In general, checking hurts driver performance,
> so we should aim to get rid of redundancies.
>  
> 

Yes, will do that.

> 
> This is not related to target checking and should be moved to a separate
> commit (with a name like gles 3 support for texsubimage_error_check).
>

Ok, makes sense.

>
> I would also move this block up to about line 2508 so it can sit next to
> the other format and type checks there.  It would be nice to have all of
> the format_and_type_checks formatted into one if, else if, else block so
> it's really obvious what's going on, too.
> 
>     +   if (_mesa_is_gles3(ctx)) {
>     +      /* Validation of format and type for ES3 has to be done here
>     +       * after the texture image is resolved, because the internal
>     +       * format is needed for the verification
>     +       */
>     +      err = _mesa_es3_error_check_format_and_type(ctx, format, type,
>     +                                                 
>     texImage->InternalFormat);
>     +      if (err != GL_NO_ERROR) {
>     +         _mesa_error(ctx, err,
>     +                     "%s(incompatible format = %s, type = %s, "
>     +                     "internalformat = %s)",
>     +                     callerName, _mesa_lookup_enum_by_nr(format),
>     +                     _mesa_lookup_enum_by_nr(type),
>     +                   
>      _mesa_lookup_enum_by_nr(texImage->InternalFormat));
>     +         return GL_TRUE;
>     +      }
>     +   }
>     +

Note that this error check has to be performed after the texture image
is resolved, because the internal format is not known before.

If we want to merge it with the above format and type error checks, the
only choice is to move all down after:

texImage = _mesa_select_tex_image(texObj, target, level);

But then we have a chicken-egg situation, since that function requires
target to be validated before.

WDYT?

cheers,
Eduardo