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

Ian Romanick idr at freedesktop.org
Fri Jan 22 10:49:44 PST 2016 

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?

>> > 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

More information about the mesa-dev mailing list