Re: [PATCH 1/3] d3d9/tests/visual: Add Vertexbuffer locking tests, try 2

[Henri Verbeet]

On 11 July 2016 at 18:13, Patrick Rudolph  wrote:
> +static void test_vb_lock_flags(void)
> +{
> - IDirect3DVertexBuffer9 *buffer;
> - IDirect3DDevice9 *device;
> - IDirect3D9 *d3d9;
> - DWORD i, j, tri, quads;
> - HWND window;
> - HRESULT hr;
> - ULONG size;
> - unsigned refcount;

"ULONG refcount;", but that's minor.
> -
>    {
> -
>        "0",
> -
>        0,
> -
>        0,
> -
>        FALSE,
> -
>        FALSE
> -
>    },

This may have been inspired by other tests, but as long as it fits I'd
prefer the more compact option of keeping this all on a single line.
You may have to shorten the "flags_s" values a bit to do that.
> - static struct
> - {
> -
>    struct
> -
>    {
> -
>        struct vec3 position;
> -
>        DWORD diffuse;
> -
>    } strip[4];
> - }
> - *quad_strip_array, quad_strip1 =
> - {
> -
>    {
> -
>        {{-1.0f, -1.0f, 0.0f}, 0xffff0000},
> -
>        {{-1.0f,  1.0f, 0.0f}, 0xff00ff00},
> -
>        {{ 1.0f, -1.0f, 0.0f}, 0xff0000ff},
> -
>        {{ 1.0f,  1.0f, 0.0f}, 0xffffffff},
> -
>    }
> - },
> - quad_strip2 =
> - {
> -
>    {
> -
>        {{-1.0f, -1.0f, 0.0f}, 0xffffff00},
> -
>        {{-1.0f,  1.0f, 0.0f}, 0xffffff00},
> -
>        {{ 1.0f, -1.0f, 0.0f}, 0xffffff00},
> -
>        {{ 1.0f,  1.0f, 0.0f}, 0xffffff00},
> -
>    }
> - };

These are triangle strips or quads, but I don't think "quad strips"
makes sense here. "quad_strip1" and "quad_strip2" can be const, and
"quad_strip_array" doesn't need to be static, right?
> -
>        hr = IDirect3DDevice9_CreateVertexBuffer(device, size, tests[i].usage, 0, D3DPOOL_DEFAULT, &buffer, NULL);
> -
>        ok(SUCCEEDED(hr), "Failed to create vertex buffer, hr %#x.\n", hr);
> -
>        if (!SUCCEEDED(hr))
> -
>            continue;

The continue is pointless, the test would have already failed at that point.
> -
>        if (tests[i].unsyncronized == unsyncronized)
> -
>            break;

This means the test will only try the smallest buffer size for cases
that you expect to be synchronised, right? That seems problematic,
since the reason you try the larger buffer sizes is that you can't
necessarily reliably detect the unsynchronised cases with the smaller
buffer sizes.
> -
>    if (tests[i].todo)
> -
>    {
> -
>        todo_wine ok(tests[i].unsyncronized == unsyncronized, "Expected buffer mapped %s. Flags = %s. Usage = %d.\n",
> -
>            tests[i].unsyncronized ? "unsyncronized" : "syncronized", tests[i].flags_s, tests[i].usage);
> -
>    }
> -
>    else
> -
>    {
> -
>        ok(tests[i].unsyncronized == unsyncronized, "Expected buffer mapped %s. Flags = %s. Usage = %d.\n",
> -
>            tests[i].unsyncronized ? "unsyncronized" : "syncronized", tests[i].flags_s, tests[i].usage);
> -
>    }

I think "todo_wine_if" would be appropriate here. Please use %#x to
print the usage.

Note that ddraw has the mostly equivalent flags DDLOCK_NOOVERWRITE and
DDLOCK_DISCARDCONTENTS as well. Direct3D 10 has
D3D10_MAP_WRITE_DISCARD and D3D10_MAP_WRITE_NO_OVERWRITE, but you
can't use them at the same time because they're not flags. Direct3D 11
is similar to Direct3D 10.

You'll need to account for the testbot failures in some way. They may
be specific to WARP, but it's suspicious that the
"D3DLOCK_NOOVERWRITE" tests do seem to pass.

Also, please follow the usual conventions for the subject line. (I.e.,
"d3d9/tests:", "d3d8/tests:", "wined3d:".)