[Mesa-dev] [PATCH 5/4] mesa: Refactor error checking for GL_TEXTURE_BASE_LEVEL vs texture targets

Jason Ekstrand jason at jlekstrand.net
Fri Jan 22 14:00:11 PST 2016 

On Fri, Jan 22, 2016 at 10:49 AM, Ian Romanick <idr at freedesktop.org> wrote:

> On 01/21/2016 12:15 PM, Jason Ekstrand wrote:
> >
> > On Jan 21, 2016 10:30 AM, "Ian Romanick" <idr at freedesktop.org
> > <mailto:idr at freedesktop.org>> wrote:
> >>
> >> On 01/20/2016 08:29 PM, Jason Ekstrand wrote:
> >> >
> >> > On Jan 20, 2016 6:20 PM, "Ian Romanick" <idr at freedesktop.org
> > <mailto:idr at freedesktop.org>
> >> > <mailto:idr at freedesktop.org <mailto:idr at freedesktop.org>>> wrote:
> >> >>
> >> >> From: Ian Romanick <ian.d.romanick at intel.com
> > <mailto:ian.d.romanick at intel.com>
> >> > <mailto:ian.d.romanick at intel.com <mailto:ian.d.romanick at intel.com>>>
> >> >>
> >> >> Add a big spec quotation justifying the error generated, which has
> >> >> changed over the GL versions.
> >> >>
> >> >> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com
> > <mailto:ian.d.romanick at intel.com>
> >> > <mailto:ian.d.romanick at intel.com <mailto:ian.d.romanick at intel.com>>>
> >> >> ---
> >> >>
> >> >> I intended to send this out with the other four, but I selected the
> >> >> wrong SHA from the list.
> >> >>
> >> >>  src/mesa/main/texparam.c | 31 ++++++++++++++++++++++++-------
> >> >>  1 file changed, 24 insertions(+), 7 deletions(-)
> >> >>
> >> >> diff --git a/src/mesa/main/texparam.c b/src/mesa/main/texparam.c
> >> >> index 89f286c..ee50b5a 100644
> >> >> --- a/src/mesa/main/texparam.c
> >> >> +++ b/src/mesa/main/texparam.c
> >> >> @@ -369,8 +369,31 @@ set_tex_parameteri(struct gl_context *ctx,
> >> >>        if (texObj->BaseLevel == params[0])
> >> >>           return GL_FALSE;
> >> >>
> >> >> +      /* Section 8.10 (Texture Parameters) of the OpenGL 4.5 Core
> >> > Profile spec
> >> >> +       * says:
> >> >> +       *
> >> >> +       *     "An INVALID_OPERATION error is generated if the
> >> > effective target
> >> >> +       *     is either TEXTURE_2D_MULTISAMPLE or
> >> > TEXTURE_2D_MULTISAMPLE_ARRAY,
> >> >> +       *     and pname TEXTURE_BASE_LEVEL is set to a value other
> >> > than zero.
> >> >> +       *
> >> >> +       *     ...
> >> >> +       *
> >> >> +       *     An INVALID_OPERATION error is generated if the
> effective
> >> > target
> >> >> +       *     is TEXTURE_RECTANGLE and pname TEXTURE_BASE_LEVEL is
> set
> >> > to any
> >> >> +       *     value other than zero."
> >> >
> >> > Do we really need the "pname"s?  IMHO, they make it harder to read.
> >>
> >> I'm not sure what you mean.  The pnames in the spec quote or ... ?
> >
> > Yes. Some versions of the spec have them and some don't.  The quote
> > below, for instance, doesn't.
>
> From 4.1 to 4.2 the format of the spec was completely changed.  In 4.1
> an earlier, the errors were mixed with the prose.  There was a lot of
> inconsistency and missing errors as a result.  In 4.2 and later, the
> errors are listed in a break-out box at the end of the section.  Having
> had to scrape through the spec countless times to find all the possible
> error cases for a new command over the years, this is a very welcome
> change.
>
> In truth, I should have quoted a bit more text below because it doesn't
> qualify the conditions in which the old error code were generated.  It's
> just some text buried in the paragraph about TEXTURE_RECTANGLE.  For
> clarity, I probably should have quoted more context.
>
>     "When target is TEXTURE_RECTANGLE, certain texture parameter
>     values may not be specified.... The error INVALID_VALUE is
>     generated if TEXTURE_BASE_LEVEL is set to any value other than
>     zero."
>
> TEXTURE_2D_MULTISAMPLE and TEXTURE_2D_MULTISAMPLE_ARRAY did not exist in
> 3.3, so they are not mentioned.  I didn't quote additional context only
> because I was only trying to highlight that the error had changed from
> INVALID_VALUE to INVALID_OPERATION since 3.3.
>
> What I think makes the 4.5 quotation unfortunate is that the two nearly
> identical errors are listed in separate bullets.  This is compounded by
> the slight differences in language ("a value" vs "any value").  I have
> submitted a Khronos bug with a patch for this issue.  My compacted
> wording is:
>
>     An INVALID_OPERATION error is generated if the effective target
>     is TEXTURE_2D_MULTISAMPLE, TEXTURE_2D_MULTISAMPLE_ARRAY, or
>     TEXTURE_RECTANGLE, and pname TEXTURE_BASE_LEVEL is set to a value
>     other than zero.
>
> The existing TEXTURE_RECTANGLE bullet is removed.
>
> The whole point of quoting the spec is to justify the code. For that to
> be effective, the quotation has to have enough context.  I'm not sure
> what we'd trim without weakening that justification.  If we remove one
> of the bullets, why is the error generated for the other cases?  If we
> remove the pnames from the bullets, why aren't we generating the error
> for other cases?
>
> We're trying to reduce the amount of time our future selves have to look
> back through the specs when a bug is submitted for a buggy application
> or conformance test. :)
>
> I'm not in a big hurry on this patch, so I'm willing to wait until
> there's some response on the spec bug.  Assuming the response is
> positive, is using the amended wording in the quotation a good compromise?
>

seems fine


>
> >> > Other than that, the series looks good.
> >> >
> >> > Reviewed-by: Jason Ekstrand <jason.ekstrand at intel.com
> > <mailto:jason.ekstrand at intel.com>
> >> > <mailto:jason.ekstrand at intel.com <mailto:jason.ekstrand at intel.com>>>
> >> >
> >> >> +       *
> >> >> +       * Note that section 3.8.8 (Texture Parameters) of the OpenGL
> >> > 3.3 Core
> >> >> +       * Profile spec said:
> >> >> +       *
> >> >> +       *     "The error INVALID_VALUE is generated if
> >> > TEXTURE_BASE_LEVEL is
> >> >> +       *     set to any value other than zero."
> >> >> +       *
> >> >> +       * We take the 4.5 language as a correction to the 3.3, and we
> >> > implement
> >> >> +       * that on all GL versions.
> >> >> +       */
> >> >>        if ((texObj->Target == GL_TEXTURE_2D_MULTISAMPLE ||
> >> >> -           texObj->Target == GL_TEXTURE_2D_MULTISAMPLE_ARRAY) &&
> >> > params[0] != 0)
> >> >> +           texObj->Target == GL_TEXTURE_2D_MULTISAMPLE_ARRAY ||
> >> >> +           texObj->Target == GL_TEXTURE_RECTANGLE) && params[0] !=
> 0)
> >> >>           goto invalid_operation;
> >> >>
> >> >>        if (params[0] < 0) {
> >> >> @@ -378,12 +401,6 @@ set_tex_parameteri(struct gl_context *ctx,
> >> >>                       "glTex%sParameter(param=%d)", suffix,
> params[0]);
> >> >>           return GL_FALSE;
> >> >>        }
> >> >> -      if (texObj->Target == GL_TEXTURE_RECTANGLE_ARB && params[0]
> > != 0) {
> >> >> -         _mesa_error(ctx, GL_INVALID_OPERATION,
> >> >> -                     "glTex%sParameter(target=%s, param=%d)",
> suffix,
> >> >> -                     _mesa_enum_to_string(texObj->Target),
> params[0]);
> >> >> -         return GL_FALSE;
> >> >> -      }
> >> >>        incomplete(ctx, texObj);
> >> >>
> >> >>        /** See note about ARB_texture_storage below */
> >> >> --
> >> >> 2.5.0
> >> >>
> >> >> _______________________________________________
> >> >> mesa-dev mailing list
> >> >> mesa-dev at lists.freedesktop.org
> > <mailto:mesa-dev at lists.freedesktop.org>
> > <mailto:mesa-dev at lists.freedesktop.org
> > <mailto:mesa-dev at lists.freedesktop.org>>
> >> >> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20160122/e985b8bc/attachment-0001.html>

More information about the mesa-dev mailing list