[PATCH 0/3] MR332: d3dx10: Support creating effect from pre-built shader
Note this will probably fail on the testbot due to #53201 without Piotr's series !272 . -- https://gitlab.winehq.org/wine/wine/-/merge_requests/332
From: Andrew Eikum <aeikum(a)codeweavers.com> --- dlls/d3dx10_43/tests/Makefile.in | 2 + dlls/d3dx10_43/tests/d3dx10.c | 171 ++++++++++++++++++++++++- dlls/d3dx10_43/tests/fx_test_ecbt.fx | Bin 0 -> 325 bytes dlls/d3dx10_43/tests/fx_test_ecbt.hlsl | 10 ++ dlls/d3dx10_43/tests/resource.rc | 23 ++++ 5 files changed, 204 insertions(+), 2 deletions(-) create mode 100644 dlls/d3dx10_43/tests/fx_test_ecbt.fx create mode 100644 dlls/d3dx10_43/tests/fx_test_ecbt.hlsl create mode 100644 dlls/d3dx10_43/tests/resource.rc diff --git a/dlls/d3dx10_43/tests/Makefile.in b/dlls/d3dx10_43/tests/Makefile.in index 95078d0e12b..0759443ef3d 100644 --- a/dlls/d3dx10_43/tests/Makefile.in +++ b/dlls/d3dx10_43/tests/Makefile.in @@ -4,3 +4,5 @@ IMPORTS = d3dx10 ole32 gdi32 C_SRCS = \ d3dx10.c + +RC_SRCS = resource.rc diff --git a/dlls/d3dx10_43/tests/d3dx10.c b/dlls/d3dx10_43/tests/d3dx10.c index 1c28a62b700..252e2a40ee8 100644 --- a/dlls/d3dx10_43/tests/d3dx10.c +++ b/dlls/d3dx10_43/tests/d3dx10.c @@ -942,6 +942,23 @@ static char *get_str_a(const WCHAR *wstr) return buffer; } +BOOL load_resource(HMODULE module, const WCHAR *resource, void **data, DWORD *size) +{ + HGLOBAL hglobal; + HRSRC rsrc; + + if (!(rsrc = FindResourceW(module, resource, (const WCHAR *)RT_RCDATA)) || + !(*size = SizeofResource(module, rsrc)) || + !(hglobal = LoadResource(module, rsrc)) || + !(*data = LockResource(hglobal))) + { + ok(0, "Failed to find resource.\n"); + return FALSE; + } + + return TRUE; +} + static BOOL create_file(const WCHAR *filename, const void *data, unsigned int size, WCHAR *out_path) { WCHAR path[MAX_PATH]; @@ -3543,10 +3560,11 @@ todo_wine { ok(!refcount, "Unexpected refcount.\n"); } -static void test_create_effect_from_resource(void) +static void test_D3DX10CreateEffectFromResource(void) { ID3D10Device *device; ID3D10Effect *effect; + ID3D10Blob *errors; ULONG refcount; HRESULT hr; @@ -3556,10 +3574,157 @@ static void test_create_effect_from_resource(void) return; } + /* test resource that doesn't exist */ hr = D3DX10CreateEffectFromResourceA(GetModuleHandleA(NULL), "resource", NULL, NULL, NULL, "fx_4_0", 0, 0, device, NULL, NULL, &effect, NULL, NULL); ok(hr == D3DX10_ERR_INVALID_DATA, "Unexpected hr %#x.\n", hr); + + /* test creating effect from pre-built DXBC shader */ + errors = NULL; + effect = NULL; + hr = D3DX10CreateEffectFromResourceA(GetModuleHandleA(NULL), "fx_test_ecbt.fx", NULL, + NULL, NULL, "fx_4_0", 0x0, 0x0, device, NULL, NULL, &effect, + &errors, NULL); + todo_wine ok(hr == S_OK, "D3DX10CreateEffectFromResource failed: %#x\n", hr); + todo_wine ok(errors == NULL, "Got unexpected effect errors\n"); + todo_wine ok(effect != NULL, "No effect created\n"); + if (errors) + ID3D10Blob_Release(errors); + if (effect) + effect->lpVtbl->Release(effect); + + + /* test creating effect from source */ + errors = NULL; + effect = NULL; + hr = D3DX10CreateEffectFromResourceA(GetModuleHandleA(NULL), "fx_test_ecbt.hlsl", NULL, + NULL, NULL, "fx_4_0", 0x0, 0x0, device, NULL, NULL, &effect, + &errors, NULL); + todo_wine ok(hr == S_OK, "D3DX10CreateEffectFromResource failed: %#x\n", hr); + todo_wine ok(errors == NULL, "Got unexpected effect errors\n"); + todo_wine ok(effect != NULL, "No effect created\n"); + if (errors) + ID3D10Blob_Release(errors); + if (effect) + effect->lpVtbl->Release(effect); + + + refcount = ID3D10Device_Release(device); + ok(!refcount, "Unexpected refcount.\n"); +} + +static void test_D3DX10CreateEffectFromMemory(void) +{ + DWORD fx_test_ecbt_size, fx_test_ecbt_src_size; + void *fx_test_ecbt, *fx_test_ecbt_src; + ID3D10Device *device; + ID3D10Effect *effect; + ID3D10Blob *errors; + ULONG refcount; + HRESULT hr; + + if (!(device = create_device())) + { + skip("Failed to create device, skipping tests.\n"); + return; + } + + /* test creating effect from pre-built DXBC shader */ + load_resource(GetModuleHandleA(NULL), L"fx_test_ecbt.fx", &fx_test_ecbt, &fx_test_ecbt_size); + + errors = NULL; + effect = NULL; + hr = D3DX10CreateEffectFromMemory(fx_test_ecbt, fx_test_ecbt_size, NULL, + NULL, NULL, "fx_4_0", 0x0, 0x0, device, NULL, NULL, &effect, + &errors, NULL); + todo_wine ok(hr == S_OK, "D3DX10CreateEffectFromMemory failed: %#x\n", hr); + todo_wine ok(errors == NULL, "Got unexpected effect errors\n"); + todo_wine ok(effect != NULL, "No effect created\n"); + if (errors) + ID3D10Blob_Release(errors); + if (effect) + effect->lpVtbl->Release(effect); + + + /* test creating effect from source */ + load_resource(GetModuleHandleA(NULL), L"fx_test_ecbt.hlsl", &fx_test_ecbt_src, &fx_test_ecbt_src_size); + + errors = NULL; + effect = NULL; + hr = D3DX10CreateEffectFromMemory(fx_test_ecbt_src, fx_test_ecbt_src_size, NULL, + NULL, NULL, "fx_4_0", 0x0, 0x0, device, NULL, NULL, &effect, + &errors, NULL); + todo_wine ok(hr == S_OK, "D3DX10CreateEffectFromMemory failed: %#x\n", hr); + todo_wine ok(errors == NULL, "Got unexpected effect errors\n"); + todo_wine ok(effect != NULL, "No effect created\n"); + if (errors) + ID3D10Blob_Release(errors); + if (effect) + effect->lpVtbl->Release(effect); + + + refcount = ID3D10Device_Release(device); + ok(!refcount, "Unexpected refcount.\n"); +} + +static void test_D3DX10CreateEffectFromFile(void) +{ + DWORD fx_test_ecbt_size, fx_test_ecbt_src_size; + void *fx_test_ecbt, *fx_test_ecbt_src; + ID3D10Device *device; + ID3D10Effect *effect; + WCHAR path[MAX_PATH]; + ID3D10Blob *errors; + ULONG refcount; + HRESULT hr; + + if (!(device = create_device())) + { + skip("Failed to create device, skipping tests.\n"); + return; + } + + /* test creating effect from pre-built DXBC shader */ + load_resource(GetModuleHandleA(NULL), L"fx_test_ecbt.fx", &fx_test_ecbt, &fx_test_ecbt_size); + + create_file(L"fx_test_ecbt.fx", fx_test_ecbt, fx_test_ecbt_size, path); + + errors = NULL; + effect = NULL; + hr = D3DX10CreateEffectFromFileW(path, + NULL, NULL, "fx_4_0", 0x0, 0x0, device, NULL, NULL, &effect, + &errors, NULL); + todo_wine ok(hr == S_OK, "D3DX10CreateEffectFromFile failed: %#x\n", hr); + todo_wine ok(errors == NULL, "Got unexpected effect errors\n"); + todo_wine ok(effect != NULL, "No effect created\n"); + if (errors) + ID3D10Blob_Release(errors); + if (effect) + effect->lpVtbl->Release(effect); + delete_file(L"fx_test_ecbt.fx"); + + + /* test creating effect from source */ + load_resource(GetModuleHandleA(NULL), L"fx_test_ecbt.hlsl", &fx_test_ecbt_src, &fx_test_ecbt_src_size); + + create_file(L"fx_test_ecbt.hlsl", fx_test_ecbt_src, fx_test_ecbt_src_size, path); + + errors = NULL; + effect = NULL; + hr = D3DX10CreateEffectFromFileW(path, + NULL, NULL, "fx_4_0", 0x0, 0x0, device, NULL, NULL, &effect, + &errors, NULL); + todo_wine ok(hr == S_OK, "D3DX10CreateEffectFromFile failed: %#x\n", hr); + todo_wine ok(errors == NULL, "Got unexpected effect errors\n"); + todo_wine ok(effect != NULL, "No effect created\n"); + if (errors) + ID3D10Blob_Release(errors); + if (effect) + effect->lpVtbl->Release(effect); + delete_file(L"fx_test_ecbt.hlsl"); + + refcount = ID3D10Device_Release(device); ok(!refcount, "Unexpected refcount.\n"); } @@ -3576,5 +3741,7 @@ START_TEST(d3dx10) test_create_texture(); test_font(); test_sprite(); - test_create_effect_from_resource(); + test_D3DX10CreateEffectFromResource(); + test_D3DX10CreateEffectFromMemory(); + test_D3DX10CreateEffectFromFile(); } diff --git a/dlls/d3dx10_43/tests/fx_test_ecbt.fx b/dlls/d3dx10_43/tests/fx_test_ecbt.fx new file mode 100644 index 0000000000000000000000000000000000000000..5996abb2d3d194f97aec71862e4e18af9e192749 GIT binary patch literal 325 zcmZ>XaB{xa^SfrM?i)iVh5(Cn9^;ja3=9meKmtUo0I^$yp(a)AfbFYx~#6Ohjg#Mpo* zHbFdM$w>@pIr)ht4D3KfAj?321&9TpG$$tmLz*E&a9DhRf3RnWr(a)tRVnh}GuzmI>A z0Yh?<5ks0WTs=rH$Z;@pG=XehAl3n5bYKA#0LfVcF~~k--~{9tKm}lCfw*o!4AKJv J3SbhU7XXq|7`p%f literal 0 HcmV?d00001 diff --git a/dlls/d3dx10_43/tests/fx_test_ecbt.hlsl b/dlls/d3dx10_43/tests/fx_test_ecbt.hlsl new file mode 100644 index 00000000000..24581832dc3 --- /dev/null +++ b/dlls/d3dx10_43/tests/fx_test_ecbt.hlsl @@ -0,0 +1,10 @@ +cbuffer cb : register(b1) +{ + float f1 : SV_POSITION; + float f2 : COLOR0; +}; + +cbuffer cb2 : register(b0) +{ + float f3 : packoffset(c2); +}; diff --git a/dlls/d3dx10_43/tests/resource.rc b/dlls/d3dx10_43/tests/resource.rc new file mode 100644 index 00000000000..e8c04cc15e6 --- /dev/null +++ b/dlls/d3dx10_43/tests/resource.rc @@ -0,0 +1,23 @@ +/* + * Copyright 2022 Andrew Eikum for CodeWeavers + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA + */ + +/* @makedep: fx_test_ecbt.fx */ +fx_test_ecbt.fx RCDATA fx_test_ecbt.fx + +/* @makedep: fx_test_ecbt.hlsl */ +fx_test_ecbt.hlsl RCDATA fx_test_ecbt.hlsl -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/332
From: Andrew Eikum <aeikum(a)codeweavers.com> --- dlls/d3dx10_43/compiler.c | 41 +++++++++++++++++++++++++++++---------- 1 file changed, 31 insertions(+), 10 deletions(-) diff --git a/dlls/d3dx10_43/compiler.c b/dlls/d3dx10_43/compiler.c index c66eb679a8f..a561cc0357f 100644 --- a/dlls/d3dx10_43/compiler.c +++ b/dlls/d3dx10_43/compiler.c @@ -25,6 +25,7 @@ #include "d3dx10.h" #include "d3dcompiler.h" #include "dxhelpers.h" +#include "wine/heap.h" WINE_DEFAULT_DEBUG_CHANNEL(d3dx); @@ -67,7 +68,9 @@ HRESULT WINAPI D3DX10CreateEffectFromFileW(const WCHAR *filename, const D3D10_SH ID3D10Device *device, ID3D10EffectPool *effect_pool, ID3DX10ThreadPump *pump, ID3D10Effect **effect, ID3D10Blob **errors, HRESULT *hresult) { - ID3D10Blob *code; + char filename_a[MAX_PATH], *source = NULL; + DWORD source_size, read_size; + HANDLE file; HRESULT hr; TRACE("filename %s, defines %p, include %p, profile %s, shader_flags %#x, effect_flags %#x, " @@ -78,20 +81,38 @@ HRESULT WINAPI D3DX10CreateEffectFromFileW(const WCHAR *filename, const D3D10_SH if (pump) FIXME("Asynchronous mode is not supported.\n"); - if (!include) - include = D3D_COMPILE_STANDARD_FILE_INCLUDE; + file = CreateFileW(filename, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL); + if (file == INVALID_HANDLE_VALUE) + return HRESULT_FROM_WIN32(GetLastError()); - if (FAILED(hr = D3DCompileFromFile(filename, defines, include, "main", profile, shader_flags, - effect_flags, &code, errors))) + source_size = GetFileSize(file, NULL); + if (source_size == INVALID_FILE_SIZE) { - WARN("Effect compilation failed, hr %#lx.\n", hr); - return hr; + hr = HRESULT_FROM_WIN32(GetLastError()); + goto end; } - hr = D3D10CreateEffectFromMemory(ID3D10Blob_GetBufferPointer(code), ID3D10Blob_GetBufferSize(code), - effect_flags, device, effect_pool, effect); - ID3D10Blob_Release(code); + if (!(source = heap_alloc(source_size))) + { + hr = E_OUTOFMEMORY; + goto end; + } + + if (!ReadFile(file, source, source_size, &read_size, NULL) || read_size != source_size) + { + WARN("Failed to read file contents.\n"); + hr = E_FAIL; + goto end; + } + + WideCharToMultiByte(CP_ACP, 0, filename, -1, filename_a, sizeof(filename_a), NULL, NULL); + + hr = D3DX10CreateEffectFromMemory(source, source_size, filename_a, defines, include, profile, + shader_flags, effect_flags, device, effect_pool, pump, effect, errors, hresult); +end: + heap_free(source); + CloseHandle(file); return hr; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/332
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=117784 Your paranoid android. === debian11 (32 bit WoW report) === d3dx10_43: Unhandled exception: assertion failed in 32-bit code (0xf7f52559).
On Tue, Jun 28, 2022 at 09:29:13AM -0500, Marvin wrote:
=== debian11 (32 bit WoW report) ===
d3dx10_43: Unhandled exception: assertion failed in 32-bit code (0xf7f52559).
As stated in the MR, this should be fixed with Piotr's commit: https://gitlab.winehq.org/wine/wine/-/merge_requests/272/diffs?commit_id=0d3... Andrew
From: Andrew Eikum <aeikum(a)codeweavers.com> --- dlls/d3dx10_43/compiler.c | 34 +++++++++++++++++++++++----------- dlls/d3dx10_43/tests/d3dx10.c | 18 +++++++++--------- 2 files changed, 32 insertions(+), 20 deletions(-) diff --git a/dlls/d3dx10_43/compiler.c b/dlls/d3dx10_43/compiler.c index a561cc0357f..46b7ecaac80 100644 --- a/dlls/d3dx10_43/compiler.c +++ b/dlls/d3dx10_43/compiler.c @@ -29,6 +29,10 @@ WINE_DEFAULT_DEBUG_CHANNEL(d3dx); +#define MAKE_TAG(ch0, ch1, ch2, ch3) \ + ((DWORD)(ch0) | ((DWORD)(ch1) << 8) | \ + ((DWORD)(ch2) << 16) | ((DWORD)(ch3) << 24 )) +#define TAG_DXBC MAKE_TAG('D', 'X', 'B', 'C') HRESULT WINAPI D3DX10CreateEffectFromMemory(const void *data, SIZE_T datasize, const char *filename, const D3D10_SHADER_MACRO *defines, ID3D10Include *include, const char *profile, @@ -46,19 +50,27 @@ HRESULT WINAPI D3DX10CreateEffectFromMemory(const void *data, SIZE_T datasize, c if (pump) FIXME("Asynchronous mode is not supported.\n"); - if (!include) - include = D3D_COMPILE_STANDARD_FILE_INCLUDE; - - if (FAILED(hr = D3DCompile(data, datasize, filename, defines, include, "main", profile, - shader_flags, effect_flags, &code, errors))) + if (datasize >= sizeof(DWORD) && ((const DWORD *)data)[0] == TAG_DXBC) { - WARN("Effect compilation failed, hr %#lx.\n", hr); - return hr; + hr = D3D10CreateEffectFromMemory((char *)data, datasize, + effect_flags, device, effect_pool, effect); + } + else + { + if (!include) + include = D3D_COMPILE_STANDARD_FILE_INCLUDE; + + if (FAILED(hr = D3DCompile(data, datasize, filename, defines, include, "main", profile, + shader_flags, effect_flags, &code, errors))) + { + WARN("Effect compilation failed, hr %#lx.\n", hr); + return hr; + } + + hr = D3D10CreateEffectFromMemory(ID3D10Blob_GetBufferPointer(code), ID3D10Blob_GetBufferSize(code), + effect_flags, device, effect_pool, effect); + ID3D10Blob_Release(code); } - - hr = D3D10CreateEffectFromMemory(ID3D10Blob_GetBufferPointer(code), ID3D10Blob_GetBufferSize(code), - effect_flags, device, effect_pool, effect); - ID3D10Blob_Release(code); return hr; } diff --git a/dlls/d3dx10_43/tests/d3dx10.c b/dlls/d3dx10_43/tests/d3dx10.c index 252e2a40ee8..ec14afce1bb 100644 --- a/dlls/d3dx10_43/tests/d3dx10.c +++ b/dlls/d3dx10_43/tests/d3dx10.c @@ -3586,9 +3586,9 @@ static void test_D3DX10CreateEffectFromResource(void) hr = D3DX10CreateEffectFromResourceA(GetModuleHandleA(NULL), "fx_test_ecbt.fx", NULL, NULL, NULL, "fx_4_0", 0x0, 0x0, device, NULL, NULL, &effect, &errors, NULL); - todo_wine ok(hr == S_OK, "D3DX10CreateEffectFromResource failed: %#x\n", hr); - todo_wine ok(errors == NULL, "Got unexpected effect errors\n"); - todo_wine ok(effect != NULL, "No effect created\n"); + ok(hr == S_OK, "D3DX10CreateEffectFromResource failed: %#x\n", hr); + ok(errors == NULL, "Got unexpected effect errors\n"); + ok(effect != NULL, "No effect created\n"); if (errors) ID3D10Blob_Release(errors); if (effect) @@ -3638,9 +3638,9 @@ static void test_D3DX10CreateEffectFromMemory(void) hr = D3DX10CreateEffectFromMemory(fx_test_ecbt, fx_test_ecbt_size, NULL, NULL, NULL, "fx_4_0", 0x0, 0x0, device, NULL, NULL, &effect, &errors, NULL); - todo_wine ok(hr == S_OK, "D3DX10CreateEffectFromMemory failed: %#x\n", hr); - todo_wine ok(errors == NULL, "Got unexpected effect errors\n"); - todo_wine ok(effect != NULL, "No effect created\n"); + ok(hr == S_OK, "D3DX10CreateEffectFromMemory failed: %#x\n", hr); + ok(errors == NULL, "Got unexpected effect errors\n"); + ok(effect != NULL, "No effect created\n"); if (errors) ID3D10Blob_Release(errors); if (effect) @@ -3695,9 +3695,9 @@ static void test_D3DX10CreateEffectFromFile(void) hr = D3DX10CreateEffectFromFileW(path, NULL, NULL, "fx_4_0", 0x0, 0x0, device, NULL, NULL, &effect, &errors, NULL); - todo_wine ok(hr == S_OK, "D3DX10CreateEffectFromFile failed: %#x\n", hr); - todo_wine ok(errors == NULL, "Got unexpected effect errors\n"); - todo_wine ok(effect != NULL, "No effect created\n"); + ok(hr == S_OK, "D3DX10CreateEffectFromFile failed: %#x\n", hr); + ok(errors == NULL, "Got unexpected effect errors\n"); + ok(effect != NULL, "No effect created\n"); if (errors) ID3D10Blob_Release(errors); if (effect) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/332
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=117785 Your paranoid android. === debian11 (32 bit WoW report) === d3dx10_43: Unhandled exception: assertion failed in 32-bit code (0xf7f66559).
Matteo Bruni (@Mystral) commented about dlls/d3dx10_43/tests/d3dx10.c:
return buffer; }
+BOOL load_resource(HMODULE module, const WCHAR *resource, void **data, DWORD *size) +{ + HGLOBAL hglobal; + HRSRC rsrc; + + if (!(rsrc = FindResourceW(module, resource, (const WCHAR *)RT_RCDATA)) || + !(*size = SizeofResource(module, rsrc)) || + !(hglobal = LoadResource(module, rsrc)) || + !(*data = LockResource(hglobal)))
I've got a bunch of nitpicks... We usually put the binary operator at the start of the next line in these cases: ```suggestion:-3+0 if (!(rsrc = FindResourceW(module, resource, (const WCHAR *)RT_RCDATA)) || !(*size = SizeofResource(module, rsrc)) || !(hglobal = LoadResource(module, rsrc)) || !(*data = LockResource(hglobal))) ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/332#note_3140
Matteo Bruni (@Mystral) commented about dlls/d3dx10_43/tests/d3dx10.c:
ok(!refcount, "Unexpected refcount.\n"); }
-static void test_create_effect_from_resource(void) +static void test_D3DX10CreateEffectFromResource(void)
I'd rather keep the current function name. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/332#note_3141
Matteo Bruni (@Mystral) commented about dlls/d3dx10_43/tests/d3dx10.c:
return; }
+ /* test resource that doesn't exist */
Capital at the start, period at the end. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/332#note_3142
Matteo Bruni (@Mystral) commented about dlls/d3dx10_43/tests/d3dx10.c:
return; }
+ /* test resource that doesn't exist */ hr = D3DX10CreateEffectFromResourceA(GetModuleHandleA(NULL), "resource", NULL, NULL, NULL, "fx_4_0", 0, 0, device, NULL, NULL, &effect, NULL, NULL); ok(hr == D3DX10_ERR_INVALID_DATA, "Unexpected hr %#x.\n", hr);
+ + /* test creating effect from pre-built DXBC shader */ + errors = NULL; + effect = NULL; + hr = D3DX10CreateEffectFromResourceA(GetModuleHandleA(NULL), "fx_test_ecbt.fx", NULL, + NULL, NULL, "fx_4_0", 0x0, 0x0, device, NULL, NULL, &effect, + &errors, NULL); + todo_wine ok(hr == S_OK, "D3DX10CreateEffectFromResource failed: %#x\n", hr);
This will need to be adjusted after !272 and the removal of -DWINE_NO_LONG_TYPES. Also, period at the end of the message. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/332#note_3143
Matteo Bruni (@Mystral) commented about dlls/d3dx10_43/tests/d3dx10.c:
+ /* test resource that doesn't exist */ hr = D3DX10CreateEffectFromResourceA(GetModuleHandleA(NULL), "resource", NULL, NULL, NULL, "fx_4_0", 0, 0, device, NULL, NULL, &effect, NULL, NULL); ok(hr == D3DX10_ERR_INVALID_DATA, "Unexpected hr %#x.\n", hr);
+ + /* test creating effect from pre-built DXBC shader */ + errors = NULL; + effect = NULL; + hr = D3DX10CreateEffectFromResourceA(GetModuleHandleA(NULL), "fx_test_ecbt.fx", NULL, + NULL, NULL, "fx_4_0", 0x0, 0x0, device, NULL, NULL, &effect, + &errors, NULL); + todo_wine ok(hr == S_OK, "D3DX10CreateEffectFromResource failed: %#x\n", hr); + todo_wine ok(errors == NULL, "Got unexpected effect errors\n"); + todo_wine ok(effect != NULL, "No effect created\n");
For these we would usually do: ```suggestion:-1+0 todo_wine ok(!errors, "Got unexpected errors %p.\n", errors); todo_wine ok(!!effect, "Got unexpected effect %p.\n", effect); ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/332#note_3144
Matteo Bruni (@Mystral) commented about dlls/d3dx10_43/tests/d3dx10.c:
+ + /* test creating effect from pre-built DXBC shader */ + errors = NULL; + effect = NULL; + hr = D3DX10CreateEffectFromResourceA(GetModuleHandleA(NULL), "fx_test_ecbt.fx", NULL, + NULL, NULL, "fx_4_0", 0x0, 0x0, device, NULL, NULL, &effect, + &errors, NULL); + todo_wine ok(hr == S_OK, "D3DX10CreateEffectFromResource failed: %#x\n", hr); + todo_wine ok(errors == NULL, "Got unexpected effect errors\n"); + todo_wine ok(effect != NULL, "No effect created\n"); + if (errors) + ID3D10Blob_Release(errors); + if (effect) + effect->lpVtbl->Release(effect); + + Just one blank line is probably enough.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/332#note_3145
Matteo Bruni (@Mystral) commented about dlls/d3dx10_43/tests/d3dx10.c:
+ errors = NULL; + effect = NULL; + hr = D3DX10CreateEffectFromResourceA(GetModuleHandleA(NULL), "fx_test_ecbt.hlsl", NULL, + NULL, NULL, "fx_4_0", 0x0, 0x0, device, NULL, NULL, &effect, + &errors, NULL); + todo_wine ok(hr == S_OK, "D3DX10CreateEffectFromResource failed: %#x\n", hr); + todo_wine ok(errors == NULL, "Got unexpected effect errors\n"); + todo_wine ok(effect != NULL, "No effect created\n"); + if (errors) + ID3D10Blob_Release(errors); + if (effect) + effect->lpVtbl->Release(effect); + + + refcount = ID3D10Device_Release(device); + ok(!refcount, "Unexpected refcount.\n"); It's nice to be able to see the value of refcount if the test fails for any reason.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/332#note_3146
Matteo Bruni (@Mystral) commented about dlls/d3dx10_43/tests/d3dx10.c:
+ ULONG refcount; + HRESULT hr; + + if (!(device = create_device())) + { + skip("Failed to create device, skipping tests.\n"); + return; + } + + /* test creating effect from pre-built DXBC shader */ + load_resource(GetModuleHandleA(NULL), L"fx_test_ecbt.fx", &fx_test_ecbt, &fx_test_ecbt_size); + + errors = NULL; + effect = NULL; + hr = D3DX10CreateEffectFromMemory(fx_test_ecbt, fx_test_ecbt_size, NULL, + NULL, NULL, "fx_4_0", 0x0, 0x0, device, NULL, NULL, &effect, We usually use 0 (or NULL) in these cases, rather than 0x0, '\0' and such.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/332#note_3147
For those kind of tests with "binary" data, we usually just put the data into an array in the test .c file, prefixed by the source between #if 0 / #endif. See d3dx9_36/tests/effect.c or d3d11/tests/d3d11.c for examples. And for yet one more nitpick: I prefer the "d3dx10:" prefix in the patch subject, without the version part, when the patch doesn't affect only one specific version. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/332#note_3148
Matteo Bruni (@Mystral) commented about dlls/d3dx10_43/compiler.c:
#include "d3dx10.h" #include "d3dcompiler.h" #include "dxhelpers.h" +#include "wine/heap.h" We recently moved d3dx10 to the C runtime memory allocation functions, please use those instead.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/332#note_3149
Matteo Bruni (@Mystral) commented about dlls/d3dx10_43/compiler.c:
+ { + hr = E_OUTOFMEMORY; + goto end; + } + + if (!ReadFile(file, source, source_size, &read_size, NULL) || read_size != source_size) + { + WARN("Failed to read file contents.\n"); + hr = E_FAIL; + goto end; + } + + WideCharToMultiByte(CP_ACP, 0, filename, -1, filename_a, sizeof(filename_a), NULL, NULL); + + hr = D3DX10CreateEffectFromMemory(source, source_size, filename_a, defines, include, profile, + shader_flags, effect_flags, device, effect_pool, pump, effect, errors, hresult); Makes sense. I'm not super happy with the patch subject, it doesn't immediately suggest to me that the idea is to always go through D3DX10CreateEffectFromMemory(). I don't necessarily have a better suggestion though, other than saying exactly that like "d3dx10: Go through D3DX10CreateEffectFromMemory() in all the D3DX10CreateEffectFrom*() variants." or similar.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/332#note_3150
Matteo Bruni (@Mystral) commented about dlls/d3dx10_43/compiler.c:
if (pump) FIXME("Asynchronous mode is not supported.\n");
- if (!include) - include = D3D_COMPILE_STANDARD_FILE_INCLUDE; - - if (FAILED(hr = D3DCompile(data, datasize, filename, defines, include, "main", profile, - shader_flags, effect_flags, &code, errors))) + if (datasize >= sizeof(DWORD) && ((const DWORD *)data)[0] == TAG_DXBC) { - WARN("Effect compilation failed, hr %#lx.\n", hr); - return hr; + hr = D3D10CreateEffectFromMemory((char *)data, datasize,
So, this gets rid of the const. I'm 99.9% sure it's just the prototype of D3D10CreateEffectFromMemory() being silly but it might be a good idea to make sure and test that it doesn't actually modify the data passed into the function on Windows... -- https://gitlab.winehq.org/wine/wine/-/merge_requests/332#note_3151
participants (5)
-
Andrew Eikum -
Andrew Eikum -
Andrew Eikum (＠aeikum)
-
Marvin -
Matteo Bruni (＠Mystral)