On Fri, Jan 22, 2016 at 10:49 AM, Ian Romanick <[email protected]> wrote:
> On 01/21/2016 12:15 PM, Jason Ekstrand wrote: > > > > On Jan 21, 2016 10:30 AM, "Ian Romanick" <[email protected] > > <mailto:[email protected]>> wrote: > >> > >> On 01/20/2016 08:29 PM, Jason Ekstrand wrote: > >> > > >> > On Jan 20, 2016 6:20 PM, "Ian Romanick" <[email protected] > > <mailto:[email protected]> > >> > <mailto:[email protected] <mailto:[email protected]>>> wrote: > >> >> > >> >> From: Ian Romanick <[email protected] > > <mailto:[email protected]> > >> > <mailto:[email protected] <mailto:[email protected]>>> > >> >> > >> >> Add a big spec quotation justifying the error generated, which has > >> >> changed over the GL versions. > >> >> > >> >> Signed-off-by: Ian Romanick <[email protected] > > <mailto:[email protected]> > >> > <mailto:[email protected] <mailto:[email protected]>>> > >> >> --- > >> >> > >> >> 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 <[email protected] > > <mailto:[email protected]> > >> > <mailto:[email protected] <mailto:[email protected]>>> > >> > > >> >> + * > >> >> + * 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 > >> >> [email protected] > > <mailto:[email protected]> > > <mailto:[email protected] > > <mailto:[email protected]>> > >> >> http://lists.freedesktop.org/mailman/listinfo/mesa-dev > >
_______________________________________________ mesa-dev mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/mesa-dev
