Re: [PATCH] d3dx10: Parially implement async data loaders interfaces
2016-11-06 19:45 GMT+01:00 Nikolay Sivov <nsivov(a)codeweavers.com>:
Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> --- dlls/d3dx10_43/async.c | 296 ++++++++++++++++++++++++++++++++++++++++-- dlls/d3dx10_43/tests/d3dx10.c | 36 ++--- 2 files changed, 306 insertions(+), 26 deletions(-)
diff --git a/dlls/d3dx10_43/async.c b/dlls/d3dx10_43/async.c index f49eb92..a9cbde5 100644 --- a/dlls/d3dx10_43/async.c +++ b/dlls/d3dx10_43/async.c
+/* Memory data loader. */ +static HRESULT WINAPI memorydataloader_Load(ID3DX10DataLoader *loader) +{ + TRACE("loader %p\n", loader);
We usually put a period at the end of trace messages. By the way, these comments don't add much IMO, it's pretty clear where each interface implementation starts and ends.
+static HRESULT WINAPI memorydataloader_Decompress(ID3DX10DataLoader *iface, void **data, SIZE_T *size) +{ + struct asyncdataloader *loader = impl_from_ID3DX10DataLoader(iface); + + FIXME("loader %p, data %p, size %p semi-stub\n", loader, data, size); + + *data = loader->data; + *size = loader->size; + + return S_OK; +}
Why the fixme? What's still missing here?
+static HRESULT WINAPI filedataloader_Load(ID3DX10DataLoader *iface) +{ + struct asyncdataloader *loader = impl_from_ID3DX10DataLoader(iface); + HANDLE mapping; + + TRACE("loader %p\n", loader); + + if (loader->u.file == INVALID_HANDLE_VALUE) + return D3D10_ERROR_FILE_NOT_FOUND; + + if (loader->data) + return S_OK; + + GetFileSize(loader->u.file, (DWORD*)&loader->size); + mapping = CreateFileMappingW(loader->u.file, NULL, PAGE_READONLY, 0, 0, NULL); + if (!mapping) + return E_FAIL; + + loader->data = MapViewOfFile(mapping, FILE_MAP_READ, 0, 0, 0); + CloseHandle(mapping); + if (!loader->data) + return E_FAIL; + + return S_OK; +}
Creating and keeping a file mapping open might not be correct. What happens on Windows if e.g. you try to delete the file after Load() but before Decompress()? The issue might also be between Decompress() and Destroy(). I guess I'd like some more tests for the file and the resource loaders...
+ if (!(rsrc = FindResourceA(module, resource, (LPSTR)RT_RCDATA)))
Please avoid LPtypes and casting away const, here and below.
On 07.11.2016 0:23, Matteo Bruni wrote:
2016-11-06 19:45 GMT+01:00 Nikolay Sivov <nsivov(a)codeweavers.com>:
Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> --- dlls/d3dx10_43/async.c | 296 ++++++++++++++++++++++++++++++++++++++++-- dlls/d3dx10_43/tests/d3dx10.c | 36 ++--- 2 files changed, 306 insertions(+), 26 deletions(-)
diff --git a/dlls/d3dx10_43/async.c b/dlls/d3dx10_43/async.c index f49eb92..a9cbde5 100644 --- a/dlls/d3dx10_43/async.c +++ b/dlls/d3dx10_43/async.c
+/* Memory data loader. */ +static HRESULT WINAPI memorydataloader_Load(ID3DX10DataLoader *loader) +{ + TRACE("loader %p\n", loader);
We usually put a period at the end of trace messages. By the way, these comments don't add much IMO, it's pretty clear where each interface implementation starts and ends.
Ok.
+static HRESULT WINAPI memorydataloader_Decompress(ID3DX10DataLoader *iface, void **data, SIZE_T *size) +{ + struct asyncdataloader *loader = impl_from_ID3DX10DataLoader(iface); + + FIXME("loader %p, data %p, size %p semi-stub\n", loader, data, size); + + *data = loader->data; + *size = loader->size; + + return S_OK; +}
Why the fixme? What's still missing here?
My understanding is that it should detect certain types of compression and actually decompress if needed.
+static HRESULT WINAPI filedataloader_Load(ID3DX10DataLoader *iface) +{ + struct asyncdataloader *loader = impl_from_ID3DX10DataLoader(iface); + HANDLE mapping; + + TRACE("loader %p\n", loader); + + if (loader->u.file == INVALID_HANDLE_VALUE) + return D3D10_ERROR_FILE_NOT_FOUND; + + if (loader->data) + return S_OK; + + GetFileSize(loader->u.file, (DWORD*)&loader->size); + mapping = CreateFileMappingW(loader->u.file, NULL, PAGE_READONLY, 0, 0, NULL); + if (!mapping) + return E_FAIL; + + loader->data = MapViewOfFile(mapping, FILE_MAP_READ, 0, 0, 0); + CloseHandle(mapping); + if (!loader->data) + return E_FAIL; + + return S_OK; +}
Creating and keeping a file mapping open might not be correct. What happens on Windows if e.g. you try to delete the file after Load() but before Decompress()? The issue might also be between Decompress() and Destroy().
Sure, that should be easy enough to test.
I guess I'd like some more tests for the file and the resource loaders...
+ if (!(rsrc = FindResourceA(module, resource, (LPSTR)RT_RCDATA)))
Please avoid LPtypes and casting away const, here and below.
Ok.
2016-11-06 22:30 GMT+01:00 Nikolay Sivov <bunglehead(a)gmail.com>:
On 07.11.2016 0:23, Matteo Bruni wrote:
2016-11-06 19:45 GMT+01:00 Nikolay Sivov <nsivov(a)codeweavers.com>:
+static HRESULT WINAPI memorydataloader_Decompress(ID3DX10DataLoader *iface, void **data, SIZE_T *size) +{ + struct asyncdataloader *loader = impl_from_ID3DX10DataLoader(iface); + + FIXME("loader %p, data %p, size %p semi-stub\n", loader, data, size); + + *data = loader->data; + *size = loader->size; + + return S_OK; +}
Why the fixme? What's still missing here?
My understanding is that it should detect certain types of compression and actually decompress if needed.
Ah, I guess that's possible. I assumed the method to be there only for custom ID3DX10DataLoader implementations but that's not necessarily the case. I'd just manually check if loading a .zip file (and maybe some other compressed format) with the file loader does return the compressed or the uncompressed data and either change the FIXME with a plain TRACE or add a small comment about the supported compressed formats you found.
participants (2)
-
Matteo Bruni -
Nikolay Sivov