On Tue, Mar 24, 2015 at 11:34 AM, Eduardo Lima Mitev <el...@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.)
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
