[PATCH 1/2] d3d11/tests: Added more multisample resolve tests.

[Stefan Dösinger]

Hi Jan,

Henri has asked me to spell out some of the usual style nitpicks :-) .
Don't expect any deep understanding of what the test actually does from
me...

Am 24.02.21 um 11:40 schrieb Jan Sikorski:


> +    D3D11_VIEWPORT viewport = { 0.0f, 0.0f, 1.0f, 1.0f, 0.0f, 1.0f };
> ...
> +    float clear_color[4] = {0.2f, 0.2f, 0.2f, 1.0f};

You can make data like this static const

> +    unsigned vertex_buffer_stride = 6 * sizeof(float);

This is ugly, it duplicates data wired into test_multisample_resolve2.
Afaics the IASetVertexBuffers can be done once after creating the
buffers, which would allow you to simplify run_test_multisample_resolve()

Please use unsigned int instead of unsigned.

> +    D3D11_MAPPED_SUBRESOURCE staging_mapped = {};

I am not sure if this is valid C, we usually use {0}. If you are
assigning all members later on anyway you can also skip the init here.
test_state_refcounting() uses a memset, test_create_texture2d() init all
fields explicitly. That's a matter of taste IMHO.

A possible idea to trim down the number of boilerplate code is to use
just one D3D11_TEXTURE2D_DESC and update only the changed fields between
the CreateTexture2D calls.

> +    hr = ID3D11Device_CreateTexture2D(context->device, &staging_desc, NULL, &staging_texture);
> +    if (FAILED(hr))
> +    {
> +        trace("Failed to create staging texture, hr %#x.\n", hr);
> +        return;
> +    }

Is there a legitimate reason for this to fail? In this case skip() would
be appropriate. If you always expect it to succeed just do
ok(SUCCEEDED(hr), ...) and let the test fail if CreateTexture2D
unexpectedly fails. No need to prevent follow-up failures.


> +static void test_multisample_resolve2(void)
> +{
> +    D3D_FEATURE_LEVEL feature_levels[] = { D3D_FEATURE_LEVEL_11_0 };
> +    UINT device_creation_flags = D3D11_CREATE_DEVICE_BGRA_SUPPORT;
> +    ID3D11PixelShader *pixel_shader, *pixel_shader_uint;
> +    struct resolve_test_context context;
> +    unsigned int i, quality_levels;
> +    unsigned char result[4];
> +    HRESULT hr;
> +
> +    const BYTE vs_code[] =

static const

> +    {
> +#if 0
> ...
> +#endif
> +        68,88,66,67,30,184,39,220,81,141,142,55,98,251,192,170,226,59,

This is different from the way we put the other shader bytecode. Afaics
you should be able to tell the MS shader compiler to give you hex dumps.

Personally I can read some stuff out of at least SM <= 3 hex dumps, but
with decimal numbers I am lost.

> +    float vertex_data [] =
> +    { /* x, y, r, g, b, a */
> +        1.0f,   1.0f,  .8f, 0.f, 0.f, 1.f,
> +        1.0f,  -1.0f,  .8f, 0.f, 0.f, 1.f,
> +        -1.0f, -1.0f,  .8f, 0.f, 0.f, 1.f,
> +    };

We have struct vec2/vec3/vec4 to make the vectorized data types more
obvious in the C program.

> +    hr = ID3D11Device_CreatePixelShader(context.device, ps_code, sizeof(ps_code),
> +            NULL, &pixel_shader);
> +    if (FAILED(hr))
> +    {
> +        trace("Failed to create pixel shader, hr %#x.\n", hr);
> +        goto out_2;
> +    }
> +    hr = ID3D11Device_CreatePixelShader(context.device, ps_uint_code, sizeof(ps_uint_code),
> +            NULL, &pixel_shader_uint);
> +    if (FAILED(hr))
> +    {
> +        trace("Failed to create uint pixel shader, hr %#x.\n", hr);
> +        goto out_3;
> +    }

Likewise, it's ok to let the test crash if one of those unexpectedly
fails. If you do expect failure in some case a skip() is more
appropriate, maybe with skipping those tests that need the float/uint
pshaders.

As a general rule of thumb, when you init a pile of things and want to
gracefully clean them up with a goto, you can init them all to NULL at
declaration time, use one goto label and then do an if
(pixel_shader_uint) ID3D11PixelShader_Release(pixel_shader_uint). If
it's just HeapAlloc'ed then HeapFree will gracefully ignore NULL
pointers. This matters more for error handling in the implementation
than the tests though.

>      queue_test(test_multisample_resolve);
> +    queue_test(test_multisample_resolve2);

This looks unsatisfying :-) . Is it possible to merge them into the
existing test_multisample_resolve? Is whatever test_multisample_resolve
does already covered by the "new" test? Or, if they are orthogonal, I am
sure a better name can be found.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20210224/fb42e5ee/attachment.sig>