<div dir="ltr"><div>Hello,</div><br><div>I will fix it.<br><div>Thanks for review.</div></div><div><br></div><div>Regards,</div><div>Andrii.<br></div><br><div class="gmail_quote"><div dir="ltr">On Tue, Dec 11, 2018 at 5:04 PM Emil Velikov <<a href="mailto:emil.l.velikov@gmail.com">emil.l.velikov@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Andrii,<br>
<br>
Pardon for joining so late. The patch looks ok, although a few bits I'm unsure<br>
about - see the "Note" below.<br>
<br>
There's a bunch of cosmetic suggestions as well - I won't read too much into<br>
them though.<br>
<br>
On Mon, 3 Dec 2018 at 15:07, <<a href="mailto:asimiklit.work@gmail.com" target="_blank">asimiklit.work@gmail.com</a>> wrote:<br>
<br>
[snip]<br>
> +#define TW 64<br>
> +#define TH 64<br>
#define LEVELS 7 // aka log2(min(TW,TH)) + 1 ?<br>
Then swap nlevels with LEVELS throughout the file.<br>
<br>
static const GLubyte fancy_pixel = [255, 128, 64, 32];<br></blockquote><div><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +static void<br>
> +get_rect_bounds(int pos, int *x, int *y, int *w, int *h)<br>
> +{<br>
> +       *x = pos * (piglit_width / 3) + 5;<br>
> +       *y = 5;<br>
> +       *w = piglit_width / 3 - 10;<br>
> +       *h = piglit_height - 10;<br>
> +}<br>
Note: The variable "pos" is always 0. If that's intentional - I'd just<br>
drop and simplify.<br>
Alternatively<br>
<br>
<br>
> +       const GLfloat expected[4] = { oneby255 * 255.0,<br>
> +                                                                                       oneby255 * 128.0,<br>
> +                                                                                       oneby255 * 64.0,<br>
> +                                                                                       oneby255 * 32.0 };<br>
Use fancy_pixel for expected[]<br>
<br>
<br>
[snip]<br>
> +       for(level = 1; level < nlevels && pass; ++level)<br>
> +       {<br>
Style nits: space after "for", "{" should be trailing on previous<br>
line, post increment<br>
<br>
> +               glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_BASE_LEVEL, level);<br>
> +               glClear(GL_COLOR_BUFFER_BIT);<br>
> +               /* If we the draw_rect function doesn't cause crash/assertion<br>
> +               * it means everything is okay and test will be marked<br>
> +               * as pass<br>
> +               */<br>
> +               draw_rect(pos);<br>
> +               /** Just in case we check it<br>
> +                */<br>
> +               pass = pass && probe_pos(pos, expected);<br>
<br>
Note: In piglit we try to run all the tests, even if one fails. Some tests could<br>
be doing it wrong though :-(<br>
<br>
Personally I would drop the "&& pass" from the loop condition and use:<br>
    pass = foo() && pass;<br>
<br>
[snip]<br>
> +       for (i = 0; i < TH; i++) {<br>
> +               for (j = 0; j < TW; j++) {<br>
> +                       img[i][j][0] = 255;<br>
> +                       img[i][j][1] = 128;<br>
> +                       img[i][j][2] = 64;<br>
> +                       img[i][j][3] = 32;<br>
<br>
for (i = 0; i < TH; i++)<br>
    for (j = 0; j < TW; j++)<br>
        img[i][j] = fancy_pixel;<br>
<br>
<br>
[snip]<br>
<br>
> +       w = TW;<br>
> +       h = TH;<br>
> +       for (nlevels = 0; w > 0 && h > 0; nlevels++) {<br>
> +               w /= 2;<br>
> +               h /= 2;<br>
> +       }<br>
> +       w = TW;<br>
> +       h = TH;<br>
> +       for (i = 0; w > 0 && h > 0; i++) {<br>
> +               glTexImage2D(GL_TEXTURE_2D, nlevels - 1 - i, GL_RGBA, w, h, 0,<br>
> +                            GL_RGBA, GL_UNSIGNED_BYTE, img);<br>
> +               w /= 2;<br>
> +               h /= 2;<br>
> +       }<br>
> +<br>
With the LEVELS, all the above becomes:<br>
<br>
       for (i = 0; i < LEVELS; i++) {<br>
               glTexImage2D(GL_TEXTURE_2D, nlevels - 1 - i, GL_RGBA, w, h, 0,<br>
                            GL_RGBA, GL_UNSIGNED_BYTE, img);<br>
               w /= 2;<br>
               h /= 2;<br>
       }<br>
<br>
<br>
HTH<br>
Emil<br>
_______________________________________________<br>
Piglit mailing list<br>
<a href="mailto:Piglit@lists.freedesktop.org" target="_blank">Piglit@lists.freedesktop.org</a><br>
<a href="https://lists.freedesktop.org/mailman/listinfo/piglit" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/piglit</a><br>
</blockquote></div></div>