Re: [PATCH] d3drm/tests: Add tests for IDirect3DRM*::LoadTexture (try 3).

Dec. 21, 2015

      -----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

Am 2015-12-18 um 14:43 schrieb Aaryaman Vasishta:
> +static char *create_bitmap(const char *filename, const int w, const int h, const int type)
Just fyi: The const on parameters that are passed by value (non-pointers) doesn't really do much, so we usually leave it out. Feel free to keep it though.

> +    /* Type 1 bitmaps are 8 bpp whereas type 2 bitmaps are 24bpp. */
> +    const int bpp = type == 1 ? 8 : 24;
Urk. Why not just pass the bpp? Or a "BOOL palettized"?

> +    unsigned char *buffer = NULL;
> +    DWORD written, size;
> +    int i, j;
> +
> +    /* Calculate size of the BITMAPINFO header */
> +    /* Space for 256 color palette would need to be allocated if the bitmap is 8bpp */
> +    size = sizeof(BITMAPINFOHEADER);
> +    if (type == 1)
> +        size += sizeof(RGBQUAD)* 256;
> +
> +    ZeroMemory(&file_header, sizeof(BITMAPFILEHEADER));
Please use memset. Applies to other places as well.

> +    file_header.bfType = 0x4d42;
Other places in Wine add a /* "BM" */ comment to show what the magic number is. dlls/comctl32/imagelist.c uses (('M' << 8) | 'B').

> +            buffer[i] = (i + rand()) % 255;
Generally it is a bad idea to use random numbers in tests. It encourages random test failures :-) . I guess you want some kind of irregular pattern. You could e.g. use i % 251 (the highest prime < 256).

> +static BOOL test_bitmap_data(const unsigned char *buffer1, const unsigned char *buffer2, const D3DRMIMAGE *img, const int depth, const int version)
"BOOL upside_down" would be more explanatory than "version" IMO. That the upside down property is related to the version can easily be seen by checking who calls test_bitmap_data with that parameter.

> +            /* d3drm aligns the 24bpp texture to 4 bytes in the buffer, with one bype padding from 24bpp texture. */
> +            /* The image is palettized if the total number of colors used is <= 256. */
Unless I am missing something the first comment isn't really of a concern in this codepath.

> +        for (i = 0; i < h; i++)
> +        {
> +            offset = version == 1 ? (w * i * 3) : (w * (h - i - 1) * 3);
> +            for (j = 0; j < w; j++)
> +            {
> +                val1 = buffer1[k] == buffer2[offset];
> +                val2 = buffer1[k + 1] == buffer2[offset + 1];
> +                val3 = buffer1[k + 2] == buffer2[offset + 2];
> +                if (!(val1 && val2 && val3))
> +                    return FALSE;
> +                k += 4;
> +                offset += 3;
> +            }
The 24bpp -> 32bpp comment would fit well above this code though.

> +    /* Test if image information matches that from the header by loading the bitmap seperately. */
"separately", according to my Thunderbird's dictionary.

> +    ok(check, "Cannot delete image stored in %s (error = %x).\n", file, GetLastError());
The way you're writing GetLastError values is inconsistent (%x vs %d).

> +static void test_load_texture1(void)
> ...
> +    file = create_bitmap("8bpp.bmp", 100, 100, 1);
> ...
> +static void test_load_texture2(void)
> ...
> +    file = create_bitmap("palette.bmp", 3, 39, 2);
> ...
> +static void test_load_texture3(void)
> ...
> +    file = create_bitmap("24bpp.bmp", 100, 100, 2);
- From this it isn't obvious if e.g. the upside down reading of v2 and v3 are a property of the interface version or the input format. Please test all formats with all versions.

Also consider testing what happens if a texture is created as IDirect3DRMTexture3 and QI'd to IDirect3DRMTexture1. Does it return upside down image data?

Stefan Dösinger

tags

participants (1)