Re: [PATCH] d3dx9/test: magic isn't part of the dds structure

[Józef Kucia]

On Mon, Jan 11, 2016 at 10:46 AM, Alistair Leslie-Hughes <
leslie_alistair(a)hotmail.com> wrote:
> Signed-off-by: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com>
> ---
>  dlls/d3dx9_36/tests/surface.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/dlls/d3dx9_36/tests/surface.c b/dlls/d3dx9_36/tests/surface.c
> index 183086f..5a20658 100644
> --- a/dlls/d3dx9_36/tests/surface.c
> +++ b/dlls/d3dx9_36/tests/surface.c
> @@ -200,7 +200,6 @@ struct dds_pixel_format
>
> struct dds_header
>  {
> -    DWORD magic;
>      DWORD size;
>      DWORD flags;
>      DWORD height;
> @@ -222,7 +221,6 @@ static void fill_dds_header(struct dds_header *header)
>  {
>      memset(header, 0, sizeof(*header));
>
> -    header->magic = MAKEFOURCC('D','D','S',' ');
>      header->size = sizeof(*header);
>      header->flags = DDS_CAPS | DDS_WIDTH | DDS_HEIGHT | DDS_PIXELFORMAT;
>      header->height = 4;
> @@ -250,10 +248,12 @@ static void check_dds_pixel_format_(unsigned int
> line,
>      D3DXIMAGE_INFO info;
>      struct
>      {
> +        DWORD magic;
>          struct dds_header header;
>          BYTE data[256];
>      } dds;
>
> +    dds.magic = MAKEFOURCC('D','D','S',' ');
>      fill_dds_header(&dds.header);
>      dds.header.pixel_format.flags = flags;
>      dds.header.pixel_format.fourcc = fourcc;
> @@ -281,6 +281,7 @@ static void test_dds_header_handling(void)
>      D3DXIMAGE_INFO info;
>      struct
>      {
> +        DWORD magic;
>          struct dds_header header;
>          BYTE data[4096 * 1024];
>      } *dds;
> @@ -399,9 +400,10 @@ static void test_dds_header_handling(void)
>
> for (i = 0; i < sizeof(tests) / sizeof(tests[0]); i++)
>      {
> -        DWORD file_size = sizeof(dds->header) + tests[i].pixel_data_size;
> +        DWORD file_size = sizeof(dds->magic) + sizeof(dds->header) +
> tests[i].pixel_data_size;
>          assert(file_size <= sizeof(*dds));
>
> +        dds->magic = MAKEFOURCC('D','D','S',' ');
>          fill_dds_header(&dds->header);
>          dds->header.flags |= tests[i].flags;
>          dds->header.width = tests[i].width;
> @@ -1235,6 +1237,7 @@ static void
> test_D3DXSaveSurfaceToFileInMemory(IDirect3DDevice9 *device)
>      ID3DXBuffer *buffer;
>      IDirect3DSurface9 *surface;
>      struct dds_header *header;
> +    BYTE *data;
>
> hr = IDirect3DDevice9_CreateOffscreenPlainSurface(device, 4, 4,
> D3DFMT_A8R8G8B8, D3DPOOL_SCRATCH, &surface, NULL);
>      if (FAILED(hr)) {
> @@ -1257,9 +1260,10 @@ static void
> test_D3DXSaveSurfaceToFileInMemory(IDirect3DDevice9 *device)
>      todo_wine ok(hr == D3D_OK, "Got unexpected hr %#x.\n", hr);
>      if (SUCCEEDED(hr))
>      {
> -        header = ID3DXBuffer_GetBufferPointer(buffer);
> +        data = ID3DXBuffer_GetBufferPointer(buffer);
> +        header = (struct dds_header *)(data + sizeof(DWORD));
>
> -        ok(header->magic == MAKEFOURCC('D','D','S',' '), "Invalid DDS
> signature.\n");
> +        ok( *(DWORD*)data == MAKEFOURCC('D','D','S',' '), "Invalid DDS
> signature.\n");
>          ok(header->size == 124, "Invalid DDS size %u.\n", header->size);
>          ok(!header->height, "Got unexpected height %u.\n",
> header->height);
>          ok(!header->width, "Got unexpected width %u.\n", header->width);
> @@ -1274,9 +1278,10 @@ static void
> test_D3DXSaveSurfaceToFileInMemory(IDirect3DDevice9 *device)
>
> hr = D3DXSaveSurfaceToFileInMemory(&buffer, D3DXIFF_DDS, surface,
> NULL, NULL);
>      ok(hr == D3D_OK, "Got unexpected hr %#x.\n", hr);
> -    header = ID3DXBuffer_GetBufferPointer(buffer);
>
> -    ok(header->magic == MAKEFOURCC('D','D','S',' '), "Invalid DDS
> signature.\n");
> +    data = ID3DXBuffer_GetBufferPointer(buffer);
> +    header = (struct dds_header *)(data + sizeof(DWORD));
> +    ok( *(DWORD*)data == MAKEFOURCC('D','D','S',' '), "Invalid DDS
> signature.\n");
>      ok(header->size == 124, "Invalid DDS size %u.\n", header->size);
>      ok(header->height == 4, "Got unexpected height %u.\n",
> header->height);
>      ok(header->width == 4, "Got unexpected width %u.\n", header->width);
> --
> 2.6.4

What's the point of this change? It doesn't fix anything and doesn't make
code more readable. If the file signature isn't included in the DDS header
size it doesn't necessarily mean we have to move it out of struct
dds_header.

[PATCH] d3dx9/test: magic isn't part of the dds structure

[Matteo Bruni]

2016-01-11 16:01 GMT+01:00 Józef Kucia <joseph.kucia(a)gmail.com>:
> On Mon, Jan 11, 2016 at 10:46 AM, Alistair Leslie-Hughes
> <leslie_alistair(a)hotmail.com> wrote:
>
> What's the point of this change? It doesn't fix anything and doesn't make
> code more readable. If the file signature isn't included in the DDS header
> size it doesn't necessarily mean we have to move it out of struct
> dds_header.

I asked this kind of change in
https://www.winehq.org/pipermail/wine-patches/2016-January/145814.html,
after Henri's review spotted it in
https://www.winehq.org/pipermail/wine-devel/2015-December/110742.html,
although I was referring to the structure in the implementation more
than this one in the tests.
I'm not adamant on requiring this change but I still think that the
signature field shouldn't be in the dds_header structure (for the
little that it might count, the signature is not in the structure
described in https://msdn.microsoft.com/en-us/library/bb943982.aspx).

Specifically about this patch, moving the signature out of struct
dds_header doesn't necessarily mean that the code has to get much
uglier. I haven't tried myself but you could probably create a new
structure to contain both the signature DWORD and struct dds_header,
avoiding the new casts and manual offsets in this patch.