[Piglit] [PATCH v2] miptree: test ability to use upside down miptree

[Emil Velikov]

Hi Andrii,

Pardon for joining so late. The patch looks ok, although a few bits I'm unsure
about - see the "Note" below.

There's a bunch of cosmetic suggestions as well - I won't read too much into
them though.

On Mon, 3 Dec 2018 at 15:07, <asimiklit.work at gmail.com> wrote:

[snip]
> +#define TW 64
> +#define TH 64
#define LEVELS 7 // aka log2(min(TW,TH)) + 1 ?
Then swap nlevels with LEVELS throughout the file.

static const GLubyte fancy_pixel = [255, 128, 64, 32];

> +static void
> +get_rect_bounds(int pos, int *x, int *y, int *w, int *h)
> +{
> +       *x = pos * (piglit_width / 3) + 5;
> +       *y = 5;
> +       *w = piglit_width / 3 - 10;
> +       *h = piglit_height - 10;
> +}
Note: The variable "pos" is always 0. If that's intentional - I'd just
drop and simplify.
Alternatively


> +       const GLfloat expected[4] = { oneby255 * 255.0,
> +                                                                                       oneby255 * 128.0,
> +                                                                                       oneby255 * 64.0,
> +                                                                                       oneby255 * 32.0 };
Use fancy_pixel for expected[]


[snip]
> +       for(level = 1; level < nlevels && pass; ++level)
> +       {
Style nits: space after "for", "{" should be trailing on previous
line, post increment

> +               glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_BASE_LEVEL, level);
> +               glClear(GL_COLOR_BUFFER_BIT);
> +               /* If we the draw_rect function doesn't cause crash/assertion
> +               * it means everything is okay and test will be marked
> +               * as pass
> +               */
> +               draw_rect(pos);
> +               /** Just in case we check it
> +                */
> +               pass = pass && probe_pos(pos, expected);

Note: In piglit we try to run all the tests, even if one fails. Some tests could
be doing it wrong though :-(

Personally I would drop the "&& pass" from the loop condition and use:
    pass = foo() && pass;

[snip]
> +       for (i = 0; i < TH; i++) {
> +               for (j = 0; j < TW; j++) {
> +                       img[i][j][0] = 255;
> +                       img[i][j][1] = 128;
> +                       img[i][j][2] = 64;
> +                       img[i][j][3] = 32;

for (i = 0; i < TH; i++)
    for (j = 0; j < TW; j++)
        img[i][j] = fancy_pixel;


[snip]

> +       w = TW;
> +       h = TH;
> +       for (nlevels = 0; w > 0 && h > 0; nlevels++) {
> +               w /= 2;
> +               h /= 2;
> +       }
> +       w = TW;
> +       h = TH;
> +       for (i = 0; w > 0 && h > 0; i++) {
> +               glTexImage2D(GL_TEXTURE_2D, nlevels - 1 - i, GL_RGBA, w, h, 0,
> +                            GL_RGBA, GL_UNSIGNED_BYTE, img);
> +               w /= 2;
> +               h /= 2;
> +       }
> +
With the LEVELS, all the above becomes:

       for (i = 0; i < LEVELS; i++) {
               glTexImage2D(GL_TEXTURE_2D, nlevels - 1 - i, GL_RGBA, w, h, 0,
                            GL_RGBA, GL_UNSIGNED_BYTE, img);
               w /= 2;
               h /= 2;
       }


HTH
Emil