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

[Laura Ekstrand]

On Tue, Mar 24, 2015 at 11:34 AM, Eduardo Lima Mitev <elima at igalia.com>
wrote:

> 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
>
> What does the (es)_error_check_format_and_type have to do with validating
the target?  I thought legal_texsubimage_target did all of the target
checking?  So if we move up _mesa_select_tex_image above the format and
type checks, it shouldn't be a problem because the target is already
checked.  (That's my understanding of the problem.)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150324/973b56cd/attachment-0001.html>