From: Matteo Bruni Subject: Re: [PATCH 3/4] d3dx10/tests: Test frame data for created texture. Message-Id: Date: Thu, 10 Jun 2021 11:22:35 +0200 In-Reply-To: References: On Mon, Jun 7, 2021 at 8:58 AM Ziqing Hui wrote: > > > Signed-off-by: Ziqing Hui > --- > dlls/d3dx10_43/tests/d3dx10.c | 208 ++++++++++++++++++++++++++++++++++ > 1 file changed, 208 insertions(+) > +static unsigned int get_bpp_from_format(DXGI_FORMAT format) > +{ > + switch (format) > + { > + case DXGI_FORMAT_R32G32B32A32_TYPELESS: If you wanted to make the patch smaller you could get rid of all the cases you don't need for the current tests. I don't mind though. > +static ID3D10Texture2D *copy_texture(ID3D10Device *device, ID3D10Texture2D *texture) This could borrow from the other d3d tests (e.g. d3d10core, d3d11) and become get_texture_readback() or something along those lines. > +{ > + ID3D10Texture2D *texture_copy; > + D3D10_TEXTURE2D_DESC desc; > + HRESULT hr; > + > + ID3D10Texture2D_GetDesc(texture, &desc); > + desc.Usage = D3D10_USAGE_STAGING; > + desc.BindFlags = 0; > + desc.CPUAccessFlags = D3D10_CPU_ACCESS_READ | D3D10_CPU_ACCESS_WRITE; You only want D3D10_CPU_ACCESS_READ here. > +static void check_frame_data(D3D10_MAPPED_TEXTURE2D *map, unsigned int stride, unsigned int height, > + unsigned int subresource, unsigned int i, unsigned int line) I don't think this needs to be a separate function, unless you plan to add a second caller later on. > + ok_(__FILE__, line)(line_match, "Test %u: Subresource %u: Data mismatch for line %u.\n", i, subresource, j); This seems to be a great occasion to make use of the new winetest_push_context / winetest_pop_context functions. It probably makes sense to add a patch earlier in the sequence to replace all the "Test %u: " prefixes in check_resource_info() with a test context. > +static void check_resource_data(ID3D10Device *device, ID3D10Resource *resource, unsigned int i, unsigned int line) If you use the test context functions you don't need to pass the test index anymore, provided that you instead pass the struct test_image pointer to the function. Which to me looks nicer anyway. > + stride = (width * get_bpp_from_format(desc.Format) + 7) >> 3; Nitpick, I'd rather have a "/ 8" here in place of the shift, it's the same thing of course but it makes it a tiny bit clearer that this is a bit -> byte count conversion. > + if (is_block_compressed(desc.Format)) > + { > + stride <<= 2; > + height = (height + 3) >> 2; > + } Similarly here, except this also assumes that the block side is 4 (which is a fair assumption in the tests, but no reason to make it more obscure than necessary).