Re: [PATCH 4/5] d3d9/tests: Add a test for ATI1N and ATI2N texture formats.

[Henri Verbeet]

On 29 October 2014 18:01, Matteo Bruni  wrote:
> @@ -6145,6 +6145,7 @@ static void test_lockrect_offset(void)
>          {D3DFMT_DXT3,                 "D3DFMT_DXT3", 4, 4, 16},
>          {D3DFMT_DXT4,                 "D3DFMT_DXT4", 4, 4, 16},
>          {D3DFMT_DXT5,                 "D3DFMT_DXT5", 4, 4, 16},
> -
>    {MAKEFOURCC('A','T','I','1'), "ATI1N",       1, 1,  1},
>    {MAKEFOURCC('A','T','I','2'), "ATI2N",       1, 1,  1},

Mostly just curiosity, but there are other tests in device.c for
ATI2N, any reason for not adding ATI1N coverage to those as well?
> - static const DWORD expected_colors_ati[] = {0x003f3f3f, 0x003f3f3f, 0x00007fff, 0x007f00ff};
> - static const DWORD expected_colors_nvidia[] = {0x007f0000, 0x007f0000, 0x00007fff, 0x007f00ff};

Conceptually I might have preferred this as

static const struct
    {
       struct vec2 position;
       D3DCOLOR amd;
       D3DCOLOR nvidia;
    }
    expected_colors[]
    {
       ...
    }

but it's not a big deal.
> - for (i = 0; i < 4; ++i)
> - {
> -
>    color = getPixelColor(device, 80 + 160 * i, 240);
> -
>    trace("Got 0x%08x.\n", color);

Is this leftover debug code?

[PATCH 4/5] d3d9/tests: Add a test for ATI1N and ATI2N texture formats.

[Matteo Bruni]

2014-10-30 11:45 GMT+01:00 Henri Verbeet :
> On 29 October 2014 18:01, Matteo Bruni  wrote:
> > @@ -6145,6 +6145,7 @@ static void test_lockrect_offset(void)
> >          {D3DFMT_DXT3,                 "D3DFMT_DXT3", 4, 4, 16},
> >          {D3DFMT_DXT4,                 "D3DFMT_DXT4", 4, 4, 16},
> >          {D3DFMT_DXT5,                 "D3DFMT_DXT5", 4, 4, 16},
> > -
> >    {MAKEFOURCC('A','T','I','1'), "ATI1N",       1, 1,  1},
> >    {MAKEFOURCC('A','T','I','2'), "ATI2N",       1, 1,  1},
>
> Mostly just curiosity, but there are other tests in device.c for
> ATI2N, any reason for not adding ATI1N coverage to those as well?

Heh, I was undecided about adding the ATI1N case to the other tests in
the same patch or not and I guess by mistake I ended up with some kind
of "middle ground" choice...
I think I'm going to add it to the missing tests too.
> > - static const DWORD expected_colors_ati[] = {0x003f3f3f, 0x003f3f3f, 0x00007fff, 0x007f00ff};
> > - static const DWORD expected_colors_nvidia[] = {0x007f0000, 0x007f0000, 0x00007fff, 0x007f00ff};
>
> Conceptually I might have preferred this as
> static const struct
> {
>    struct vec2 position;
>    D3DCOLOR amd;
>    D3DCOLOR nvidia;
> }
> expected_colors[]
> {
>    ...
> }
>
> but it's not a big deal.
> > - for (i = 0; i < 4; ++i)
> > - {
> > -
> >    color = getPixelColor(device, 80 + 160 * i, 240);
> > -
> >    trace("Got 0x%08x.\n", color);
>
> Is this leftover debug code?

Initially it was but then left it there on purpose, to be able to
check whether the tested driver takes the AMD or the Nvidia approach.
Not sure it is worth it though.

[PATCH 4/5] d3d9/tests: Add a test for ATI1N and ATI2N texture formats.

[Henri Verbeet]

On 30 October 2014 14:13, Matteo Bruni  wrote:
> Initially it was but then left it there on purpose, to be able to
> check whether the tested driver takes the AMD or the Nvidia approach.
> Not sure it is worth it though.

I think it's mostly just adding noise when running the tests manually
on Windows.