<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Fri, Jan 22, 2016 at 10:49 AM, Ian Romanick <<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 01/21/2016 12:15 PM, Jason Ekstrand wrote: > > On Jan 21, 2016 10:30 AM, "Ian Romanick" <<a href="mailto:idr@freedesktop.org">idr@freedesktop.org</a> > <mailto:<a href="mailto:idr@freedesktop.org">idr@freedesktop.org</a>>> wrote: >> >> On 01/20/2016 08:29 PM, Jason Ekstrand wrote: >> > >> > On Jan 20, 2016 6:20 PM, "Ian Romanick" <<a href="mailto:idr@freedesktop.org">idr@freedesktop.org</a> > <mailto:<a href="mailto:idr@freedesktop.org">idr@freedesktop.org</a>> >> > <mailto:<a href="mailto:idr@freedesktop.org">idr@freedesktop.org</a> <mailto:<a href="mailto:idr@freedesktop.org">idr@freedesktop.org</a>>>> wrote: >> >> >> >> From: Ian Romanick <<a href="mailto:ian.d.romanick@intel.com">ian.d.romanick@intel.com</a> > <mailto:<a href="mailto:ian.d.romanick@intel.com">ian.d.romanick@intel.com</a>> >> > <mailto:<a href="mailto:ian.d.romanick@intel.com">ian.d.romanick@intel.com</a> <mailto:<a href="mailto:ian.d.romanick@intel.com">ian.d.romanick@intel.com</a>>>> >> >> >> >> Add a big spec quotation justifying the error generated, which has >> >> changed over the GL versions. >> >> >> >> Signed-off-by: Ian Romanick <<a href="mailto:ian.d.romanick@intel.com">ian.d.romanick@intel.com</a> > <mailto:<a href="mailto:ian.d.romanick@intel.com">ian.d.romanick@intel.com</a>> >> > <mailto:<a href="mailto:ian.d.romanick@intel.com">ian.d.romanick@intel.com</a> <mailto:<a href="mailto:ian.d.romanick@intel.com">ian.d.romanick@intel.com</a>>>> <div><div class="h5">>> >> --- >> >> >> >> 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. </div></div>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? </blockquote><div> </div><div>seems fine </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> >> > Other than that, the series looks good. >> > >> > Reviewed-by: Jason Ekstrand <<a href="mailto:jason.ekstrand@intel.com">jason.ekstrand@intel.com</a> > <mailto:<a href="mailto:jason.ekstrand@intel.com">jason.ekstrand@intel.com</a>> >> > <mailto:<a href="mailto:jason.ekstrand@intel.com">jason.ekstrand@intel.com</a> <mailto:<a href="mailto:jason.ekstrand@intel.com">jason.ekstrand@intel.com</a>>>> <div><div class="h5">>> > >> >> + * >> >> + * 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 >> >> <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > <mailto:<a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a>> </div></div>> <mailto:<a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > <mailto:<a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a>>> >> >> <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div> </div></div>