From: "Stefan Dösinger" Subject: Re: [PATCH 1/2] d3d11/tests: Added more multisample resolve tests. Message-Id: Date: Wed, 24 Feb 2021 13:32:09 +0100 In-Reply-To: <20210224104035.41465-1-jsikorski@codeweavers.com> References: <20210224104035.41465-1-jsikorski@codeweavers.com> 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. -----BEGIN PGP SIGNATURE----- wsF5BAABCAAjFiEEQxb0tqoFWyeVMl1sPRO8yFRPGiIFAmA2R0kFAwAAAAAACgkQPRO8yFRPGiKY qw//bM/D0uEgkxVfZejIZeYiDM2nreAatChEqQQpS/rdYYwnPRbqtuFCgoU40lEdxgNqypxue5/S yDlqbLG6NI7NLtJ1DewL9MLPnh8l9c95QJ/QDO1wciMt7MEZ/fOtfFHcdwNeqKSB4C/2VO6HvEzQ +TYkifZWPvTNJXPySQJNQr1eZATrJuHjRW8dd+RKQpJgKHVJmIaV7gScnuXQliPriDXHHkg5eeKw vNj6LgZUaimxKxHKGxqOYE0N3geMxzTbakaZiQ5evKPHCjZSq2ePWXpDfz9+mPu6/brQZ9f+89b+ LXPklZQMFfXJEkfFXdhUB9xK05mIbXGiNxpkRcTCzyuDKJjolO5CPSKVyEEEG8nv/SBuFN2Vs24L 66735I8K8EmojuZfoGYkEGXn1Sk0hkAqFPQtYKw9LILFUvYCt1MdWtq3V6I1nWd/laDa0sj1vkqs 4dASgE5F+HLRS0nHTckAELMaUc8LpzAqS7lMc3RgDOioxLxqQRY4+9liASGg0w5uQBJu9frRb4+2 K9c0ADtCL8hXJtj2rhSdhyoCmCDrE07lypsO+p4NQAK3nfcj3hnL+gnMZMsbZ26M9psJuJGAvyCN +cl6Xn/kJdeOev2eG9jCSAgiCLU6yKl3UB7f+uEaxl8sxmv1jsBasym83TvwoCSyOJjJkZe7FIif JFs= =Vdku -----END PGP SIGNATURE-----