Soft reminder. -----Original Message----- From: [email protected] [mailto:[email protected]] On Behalf Of Huang, JunX A Sent: Friday, February 28, 2014 5:07 PM To: Ian Romanick; Brian Paul; [email protected] Subject: Re: [Piglit] [PATCH] for gles v1 extension oes_query_matrix Importance: High
Hi Paul, The reason for using snprintf(), strcat() is that I wanted to output the whole matrix as 4*4 table when any position went wrong, but you are right, it was too complicate, so I change it as your suggestion, only print the fail position. And I also add some comment in tests function, as your suggestion too. Hi Ian, I cannot set a #define for glQueryMatrixxOES because there is already a define in my local driver file "glext.h", so I use piglit_QueryMatrixxOES directly. Thanks for your suggestion! And I make some other small changes, as new file attached. Thanks again~ Jun -----Original Message----- From: Ian Romanick [mailto:[email protected]] Sent: Tuesday, December 24, 2013 4:54 AM To: Brian Paul; Huang, JunX A; [email protected] Subject: Re: [Piglit] [PATCH] for gles v1 extension oes_query_matrix Importance: High On 12/18/2013 07:54 AM, Brian Paul wrote: > On 12/17/2013 07:06 PM, Huang, JunX A wrote: >> Hi Paul, >> >> Thanks for your correcting! >> But I still can not follow your following saying, could you be more >> specific? >> "Also, maybe it should be structured so that the floating point >> matrix is first constructed from the mantisa & exponent arrays first. >> Then, compare that matrix to the expected result." >> And here is the update file: > > See below. > > >> >> /* >> * Copyright © 2013 Intel Corporation >> * >> * Permission is hereby granted, free of charge, to any person >> obtaining a >> * copy of this software and associated documentation files (the >> "Software"), >> * to deal in the Software without restriction, including without >> limitation >> * the rights to use, copy, modify, merge, publish, distribute, >> sublicense, >> * and/or sell copies of the Software, and to permit persons to whom the >> * Software is furnished to do so, subject to the following conditions: >> * >> * The above copyright notice and this permission notice (including >> the next >> * paragraph) shall be included in all copies or substantial >> portions of the >> * Software. >> * >> * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, >> EXPRESS OR >> * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF >> MERCHANTABILITY, >> * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO >> EVENT SHALL >> * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES >> OR OTHER >> * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, >> ARISING >> * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR >> OTHER DEALINGS >> * IN THE SOFTWARE. >> */ >> >> /** >> * @file >> * @brief Test GL_OES_query_matrix with types of operation in OpenGL >> ES 1.1. >> * >> * It uses the GL_FIXED data type. >> * >> */ >> >> #include "piglit-util-gl-common.h" >> #include "piglit-util-egl.h" >> >> >> struct sub_test >> { >> const char *name; >> const GLfloat matrix[16]; >> }; >> >> PIGLIT_GL_TEST_CONFIG_BEGIN config.supports_gl_es_version = 11; >> PIGLIT_GL_TEST_CONFIG_END static PFNGLQUERYMATRIXXOESPROC >> glQueryMatrixxOES; static char sOut[256] = {0}, sTmp[256] = {0}; >> >> static bool >> verifyQuery(struct sub_test >> test, GLbitfield status) > > Unneeded line break. > > >> { >> const GLfloat * matrix = test.matrix; >> GLfloat (*m)[4] = (void *) matrix; > > That cast looks suspicious. I don't know why you need 'm' at all. > See below. > > >> GLint i, j, k; >> bool functionPass = true; >> GLbitfield querystatus; >> GLfixed fMan[16]; >> GLint iExp[16]; >> GLfloat fQueryresult, fLimit, fDiff; > > We usually put a blank line here between the declarations and code. > >> sOut[0] = '\0'; >> strcat(sOut, sTmp); >> querystatus = glQueryMatrixxOES(fMan, iExp); >> functionPass = piglit_check_gl_error(GL_NO_ERROR) && functionPass; >> snprintf(sTmp, sizeof(sTmp), "0x%x,0x%x, VerifyQuery:\n", >> status, querystatus); >> strcat(sOut, sTmp); >> >> for(i = 0; i < 4; i++){ >> for(j = 0; j < 4; j++){ >> k = j * 4 + i; >> strcat(sOut, "\t\t"); >> fQueryresult = 1.0 * fMan[k] * (exp2 (iExp[k]) / (1 << 16)); >> fLimit = 0.01 * (fabs(fQueryresult) + 1); >> fDiff = m[i][j] - fQueryresult; >> if((querystatus & (1 << k)) != 0 && (status & (1 << k)) != 0) >> strcat(sOut, "sameNA"); >> else if(abs(fDiff) <= fLimit){ >> strcat(sOut, "sameValue"); >> }else{ >> if(!(fDiff <= fLimit)){ >> snprintf(sTmp, sizeof(sTmp), >> "should:%f<%f", (float) fDiff, (float) >> fLimit); >> strcat(sOut, sTmp); >> }else if(!(fDiff >= (-fLimit))){ >> snprintf(sTmp, sizeof(sTmp), >> "should:%f>%f", (float) fDiff, (float) >> (-fLimit)); >> strcat(sOut, sTmp); >> }else{ >> snprintf(sTmp, sizeof(sTmp), >> "should:%f=%f,%i=%i", fQueryresult, m[i][j], >> querystatus & (1 << k), status & (1 << k)); >> strcat(sOut, sTmp); >> } >> functionPass = false; >> } >> } >> strcat(sOut, "\n"); >> } > > I'd replace the above code with two loops. First, construct the > matrix from the mantissa/exponent info: I would like some flavor of Brian's suggestion. > GLfloat mat[4]; ^ 16 > for (k = 0; k < 16; k++) { ^^ ARRAY_SIZE(mat) > mat[k] = 1.0 * fMan[k] * (exp2 (iExp[k]) / (1 << 16)); Or mat[k] = ldexp(fMan[k], iExp[k] - 16); ...assuming ldexp is available in MSVC. > } > > Then, do the loops which check the values in the matrix: > > for (k = 0; k < 16; k++) { > fLimit = .01 * (fabs(mat[k]) + 1); > fDiff = matrix[k] - mat[k]; > if (fDiff indicates wrong value) { > printf("Bad matrix value at position %d," > " expected %f, found %f\n", > k, mat[k], matrix[k]); > pass = false; > } > } > > Also, all the snprintf(), strcat() code seems complicated. What's the > purpose of that? Why not use a simple printf() when a problem is found? > > > > >> strcat(sOut, "\n"); >> sTmp[0] = '\0'; >> piglit_report_subtest_result( >> functionPass ? PIGLIT_PASS : PIGLIT_FAIL, test.name); >> return functionPass ? true : false; } >> >> >> /* new window size or exposure */ >> static bool >> tests() > > tests(void) > > >> { >> bool pass = true; >> GLint maxexp; >> GLfixed maxman; >> struct sub_test basedm = { > > const? > >> "based", >> { >> 1, 0, 0, 0 , >> 0, 1, 0, 0, >> 0, 0, 1, 0, >> 0, 0, 0, 1 >> }}, >> >> transformedm1 = { >> "transformedm1", >> { >> 6.666626, 0, 0, 0, >> 0, 5, 0, 0, >> 0, 0, -1.181793, -10.908936, >> 0, 0, -1, 0 >> }}, >> >> transformedm2 = { >> "transformedm2", >> { >> 0.444443, 0, 0, 0, >> 0, 0.333328, 0, 0, >> 0, 0, -0.666656, 0.333328, >> 0, 0, 0, 1 >> }}, >> >> transformedm3 = { >> "transformedm3", >> { >> 2, 0, 0,0, >> 0, 3, 0, 0, >> 0, 0, 0.500000, 0, >> 0, 0, 0, 1 >> }}, >> >> transformedm4 = { >> "transformedm4", >> { >> 2, 0, 0, 2, >> 0, 3, 0, 6, >> 0, 0, 0.500000, -3, >> 0, 0, 0, 1 >> }}, >> >> transformedm5 = { >> "transformedm5", >> { >> 1.999939, -0.002892, 0.005826, 0, >> 0.002926, 2.999878, -0.017427, 0, >> -0.002905, 0.002913, 0.499977, 0, >> 0, 0, 0, 1 >> }}, >> >> transformedm6 = { >> "transformedm6", >> { >> 2, 0, 0, 0 , >> 0, 0, 0, 0, >> 0, 0, 3.4E38, 0, >> 0, 0, 0, 1 >> }}, >> >> transformedm7 = { >> "transformedm7", >> { >> 1, 0, 0, 2, >> 0, 1, 0, 0, >> 0, 0, 1, 3.4E38, >> 0, 0, 0, 1 >> }}, >> >> transformedm8 = { >> "transformedm8", >> { >> 1, 0, 0, 2, >> 0, 1, 0, 0, >> 0, 0, 1, 3.4E38, >> 0, 0, 0, 1 >> }}, >> >> transformedm9 = { >> "transformedm9", >> { >> 2, 0, 0, 2, >> 0, 0, 0, 0, >> 0, 0, 3.4E38, 3.4E38, >> 0, 0, 0, 1 >> }}; >> >> GLfloat ar = 3.0f / 4.0f; >> >> glMatrixMode(GL_PROJECTION); >> glLoadIdentity(); >> glPushMatrix(); >> >> snprintf(sTmp, sizeof(sTmp), "Identity:\n"); >> pass = verifyQuery(basedm, 0) && pass; >> >> glFrustumf(-ar, ar, -1, 1, 5.0, 60.0); >> snprintf(sTmp, sizeof(sTmp), "Frustum:\n"); >> pass = verifyQuery(transformedm1, 0) && pass; >> >> >> >> glPopMatrix(); >> pass = verifyQuery(basedm, 0) && pass; >> >> glOrthof(-3.0 * ar, 3.0 * ar, -3.0, 3.0, -2.0, 1.0); >> >> pass = verifyQuery(transformedm2, 0) && pass; >> >> >> glMatrixMode(GL_MODELVIEW); >> glLoadIdentity(); >> glScalef(2.0, 3.0, 0.5); >> >> pass = verifyQuery(transformedm3, 0) && pass; >> >> snprintf(sTmp, sizeof(sTmp), "Push,Translate:\n"); > > What is all the snprintf() code for? The sTmp variable is never used > AFAICT. > > >> glPushMatrix(); >> glTranslatef(1.0, 2.0, -6.0); >> >> pass = verifyQuery(transformedm4, 0) && pass; >> >> >> snprintf(sTmp, sizeof(sTmp), "pop:%5Cn"); >> glPopMatrix(); >> pass = verifyQuery(transformedm3, 0) && pass; >> >> >> snprintf(sTmp, sizeof(sTmp), "Rotate:\n"); >> glRotatef(0.5, 1.0, 1.0, 0.5); >> >> pass = verifyQuery(transformedm5, 0) && pass; >> >> > > The following code needs comments to explain what it's doing. In > fact, the whole test has no comments at all. Please look at other > piglit tests to get an idea of how comments should be used. > > >> snprintf(sTmp, sizeof(sTmp), "overflow by max of GLfixed:\n"); >> glLoadIdentity(); >> maxexp = (1 << (8 * sizeof(GLint) - 1)) ^ (GLint) (-1); >> maxman = (1 << (8 * sizeof(GLfixed) - 1)) ^ (GLfixed) (-1); >> glTranslatef(1.0, 2.0, 1.0 * maxman * (exp2 (maxexp) / (1 << >> 16))); >> >> pass = verifyQuery(basedm, 0xf000) && pass; >> >> snprintf(sTmp, sizeof(sTmp), "invalid bits still:\n"); >> glScalef(2.0, 3.0, 0.5); >> >> pass = verifyQuery(transformedm3, 0xf000) && pass; >> >> snprintf(sTmp, sizeof(sTmp), >> "revalid bits. glScalef with max and min of >> GLfloat:\n"); > > revalid? > > >> glLoadIdentity(); >> glScalef(2.0, 3.4E-38, 3.4E38); >> pass = verifyQuery(transformedm6, 0) && pass; >> >> >> snprintf(sTmp, sizeof(sTmp), >> "glTranslatef with max and min of GLfloat:\n"); >> glLoadIdentity(); >> glTranslatef(2.0, 3.4E-38, 3.4E38); >> pass = verifyQuery(transformedm7, 0) && pass; >> >> >> snprintf(sTmp, sizeof(sTmp), "glRotatef a circle:\n"); >> glRotatef(360, 0, 0, 0); >> >> pass = verifyQuery(transformedm7, 0) && pass; >> >> >> snprintf(sTmp, sizeof(sTmp), "glRotatef with max and min of >> GLfloat:\n"); >> glRotatef(360, 2.0, 3.4E-38, 3.4E38); >> pass = verifyQuery(transformedm8, 0) && pass; >> >> >> snprintf(sTmp, sizeof(sTmp), >> "glScalef again with max and min of GLfloat:\n"); >> glScalef(2.0, 3.4E-38, 3.4E38); >> pass = verifyQuery(transformedm9, 0) && pass; >> >> >> if(pass != true) > > if (!pass) > > >> fprintf (stderr, "%s\nVerify Query Fail\n", sOut); >> return pass; >> } >> >> enum piglit_result >> piglit_display(void) >> { >> /* UNREACHED */ >> return PIGLIT_FAIL; >> } >> >> void >> piglit_init(int argc, char **argv) >> { >> piglit_require_extension("GL_OES_query_matrix"); >> glQueryMatrixxOES = (PFNGLQUERYMATRIXXOESPROC) >> eglGetProcAddress("glQueryMatrixxOES"); Do *NOT* name the function pointer the same as the function. Use piglit_QueryMatrixxOES and have a #define for glQueryMatrixxOES instead. >> if(!glQueryMatrixxOES) >> piglit_report_result(PIGLIT_FAIL); >> if(!piglit_check_gl_error(GL_NO_ERROR)){ >> /* Should be no error at this point. If there is, report >> failure */ >> piglit_report_result(PIGLIT_FAIL); >> } >> piglit_report_result(tests() ? PIGLIT_PASS : PIGLIT_FAIL); } >> >> > > _______________________________________________ > Piglit mailing list > [email protected] > http://lists.freedesktop.org/mailman/listinfo/piglit > _______________________________________________ Piglit mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/piglit
