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

[Stefan Dösinger]

> Am 23.12.2015 um 16:46 schrieb Aaryaman Vasishta :
> - file = create_bitmap("palette.bmp", 3, 39, TRUE);
> -
> - hr = Direct3DRMCreate(&d3drm1);
> - ok(hr == D3DRM_OK, "Cannot get IDirect3DRM interface (hr = %x)\n", hr);
> -
> - hr = IDirect3DRM_QueryInterface(d3drm1, &IID_IDirect3DRM2, (void **)&d3drm2);
> - ok(SUCCEEDED(hr), "Cannot get IDirect3DRM2 interface (hr = %x)\n", hr);
> -
> - file = create_bitmap("8bpp.bmp", 100, 100, FALSE);

The first line here seems to be a leftover from the last patch version. The same applies to the V3 function

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

[Aaryaman Vasishta]

On Sat, Dec 26, 2015 at 11:46 PM, Stefan Dösinger <[e-mail]
> wrote:
> > Am 23.12.2015 um 16:46 schrieb Aaryaman Vasishta <
>
> [e-mail]>:
> > - file = create_bitmap("palette.bmp", 3, 39, TRUE);
> > -
> > - hr = Direct3DRMCreate(&d3drm1);
> > - ok(hr == D3DRM_OK, "Cannot get IDirect3DRM interface (hr = %x)\n",
>
> hr);
> > -
> > - hr = IDirect3DRM_QueryInterface(d3drm1, &IID_IDirect3DRM2, (void
>
> **)&d3drm2);
> > - ok(SUCCEEDED(hr), "Cannot get IDirect3DRM2 interface (hr = %x)\n",
>
> hr);
> > -
> > - file = create_bitmap("8bpp.bmp", 100, 100, FALSE);
>
> The first line here seems to be a leftover from the last patch version.
> The same applies to the V3 function

My bad, that line is definitely redundant.
Anything else before I send try 5? Hopefully that'll be the last one :)

Cheers,
Aaryaman

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

[Stefan Dösinger]

> Am 26.12.2015 um 19:51 schrieb Aaryaman Vasishta :
>
> Anything else before I send try 5? Hopefully that'll be the last one :)

I'm sitting in a crowded train with crappy internet access right now, so reviews are a bit tricky :-) .

Actually I noticed quite a lot on a second look:
> -
>    info = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(BITMAPINFO));

I prefer sizeof(*info) instead of sizeof(BITMAPINFO) in this case, but it's mostly a matter of style.
> - WriteFile(file, (void *)info, size, &written, NULL);

A cast from / to void * is not necessary in C. (It's a C++ thingy).

You could add a few more checks for bpp and bytes_per_line in test_bitmap_data(). Most of those would be inside the format-specific paths.
> - ok(size != INVALID_FILE_SIZE, "Failed to get file size, error %d\n", GetLastError());
> - if (size == INVALID_FILE_SIZE)
> -
>    goto cleanup;

This is another case where you can skip the ifs, if the branch is taken the test already failed anyway.
> - buffer2 = ((unsigned char *)buffer + bmp_header->bfOffBits);

The outer parenthesis are kinda redundant
> - file = create_bitmap("8bpp.bmp", 100, 100, FALSE);
>
> ...
> - file = create_bitmap("24bpp.bmp", 100, 100, FALSE);
>
> ...

I don't think those two differ. I guess the first one should have palletized = TRUE

I hope that's all now, but I can't promise...

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

[Aaryaman Vasishta]

On Sun, Dec 27, 2015 at 12:40 AM, Stefan Dösinger <[e-mail]
> wrote:
> > Am 26.12.2015 um 19:51 schrieb Aaryaman Vasishta <
>
> [e-mail]>:
> > Anything else before I send try 5? Hopefully that'll be the last one :)
>
> I'm sitting in a crowded train with crappy internet access right now, so
> reviews are a bit tricky :-) .
>
> I hope that's all now, but I can't promise...

No problem. Thanks for the review! I'll make the changes. Do reply later on
if there's more. I'll send a try 5 tomorrow.

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

[Aaryaman Vasishta]

On Sun, Dec 27, 2015 at 12:40 AM, Stefan Dösinger <[e-mail]
> wrote:
> You could add a few more checks for bpp and bytes_per_line in
> test_bitmap_data(). Most of those would be inside the format-specific paths.

I've sent try 5 with some more tests and also added odd/even width tests
for palettized textures, and format-specific depth/bpp tests along with the
rest of the changes.

Cheers,
Aaryaman