[PATCH v2 2/2] d3dx10: Implement D3DX10GetImageInfoFromMemory().

[Ziqing Hui]

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=48856
Signed-off-by: Ziqing Hui 
---
v2:
* Add Wine-Bug link in commit message.
* Add parameter name for WICCreateImagingFactory_Proxy() declaration.
* Change variable declarations order to a reverse Christmas tree.
* Move FIXME() for WMP format to the beginning of the function.

dlls/d3dx10_43/Makefile.in      |   1 +
 dlls/d3dx10_43/d3dx10_43_main.c | 143 +++++++++++++++++++++++++++++++-
 dlls/d3dx10_43/tests/d3dx10.c   |   7 +-
 3 files changed, 144 insertions(+), 7 deletions(-)

[Matteo Bruni]

On Fri, Jul 3, 2020 at 1:06 PM Ziqing Hui  wrote:
> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=48856
> Signed-off-by: Ziqing Hui
>
> v2:
> - Add Wine-Bug link in commit message.
> - Add parameter name for WICCreateImagingFactory_Proxy() declaration.
> - Change variable declarations order to a reverse Christmas tree.
> - Move FIXME() for WMP format to the beginning of the function.
>
> dlls/d3dx10_43/Makefile.in      |   1 +
>  dlls/d3dx10_43/d3dx10_43_main.c | 143 +++++++++++++++++++++++++++++++-
>  dlls/d3dx10_43/tests/d3dx10.c   |   7 +-
>  3 files changed, 144 insertions(+), 7 deletions(-)

The patch is generally okay. I do have a number of small nits though.

+static D3DX10_IMAGE_FILE_FORMAT
wic_container_guid_to_file_format(GUID *container_format)
+{
+    if (IsEqualGUID(container_format, &GUID_ContainerFormatBmp))
+        return D3DX10_IFF_BMP;
+    else if(IsEqualGUID(container_format, &GUID_ContainerFormatJpeg))
+        return D3DX10_IFF_JPG;
+    else if (IsEqualGUID(container_format, &GUID_ContainerFormatPng))
+        return D3DX10_IFF_PNG;
+    else if(IsEqualGUID(container_format, &GUID_ContainerFormatDds))
+        return D3DX10_IFF_DDS;
+    else if(IsEqualGUID(container_format, &GUID_ContainerFormatTiff))
+        return D3DX10_IFF_TIFF;
+    else if(IsEqualGUID(container_format, &GUID_ContainerFormatGif))
+        return D3DX10_IFF_GIF;
+    else if(IsEqualGUID(container_format, &GUID_ContainerFormatWmp))
+        return D3DX10_IFF_WMP;
+    else
+        return D3DX10_IFF_FORCE_DWORD;
+}

All the elses are unnecessary and should be dropped. In fact, it's
probably better to replicate d3dx9 here and implement this with a
table instead.

+    if (pump)
+        FIXME("Thread pump is not supported yet.");

Missing '\n' at the end of the message. More of the same below.

+    if ((src_data_size >= 4) && (!strncmp(src_data, "II\xbc\x00", 4)
|| !strncmp(src_data, "II\xbc\x01", 4)))
+    {
+        FIXME("File type WMP is not supported yet.\n");
+        return E_FAIL;
+    }

This is not ideal. In particular, doing this with string comparisons
looks awkward.
I'd get rid of the check entirely and turn the "Invalid or unsupported
image file" TRACE at the end of the file into a WARN instead.

+    hr = IWICStream_InitializeFromMemory(stream, (BYTE*)src_data,
(DWORD)src_data_size);

The src_data_size cast shouldn't be needed.

+end:
+    if (factory)
+        IWICImagingFactory_Release(factory);
+    if (stream)
+        IWICStream_Release(stream);
+    if (decoder)
+        IWICBitmapDecoder_Release(decoder);
+    if (frame)
+        IWICBitmapFrameDecode_Release(frame);
+    if (dds_decoder)
+        IWICDdsDecoder_Release(dds_decoder);

Nitpick: I'd prefer to have the release happen in reverse order of
initialization.

I have a somewhat related question. Does d3dx10 support loading
uncompressed DDS files? I know that WIC doesn't, but d3dx10 might have
internal support for that...

[Ziqing Hui]

Hi Bruni,

Thanks for reviewing. I'll sent a v3 version later.

According to my tests, at least GetImageInfoFromMemory() supports uncompressed DDS. Native WIC doesn't support uncompressed DDS, but I would like to support it in wine's implementation for WIC.

On 7/8/20 5:15 AM, Matteo Bruni wrote:
> On Fri, Jul 3, 2020 at 1:06 PM Ziqing Hui  wrote:
> > Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=48856
> > Signed-off-by: Ziqing Hui
> >
> > v2:
> > - Add Wine-Bug link in commit message.
> > - Add parameter name for WICCreateImagingFactory_Proxy() declaration.
> > - Change variable declarations order to a reverse Christmas tree.
> > - Move FIXME() for WMP format to the beginning of the function.
> >
> > dlls/d3dx10_43/Makefile.in      |   1 +
> >  dlls/d3dx10_43/d3dx10_43_main.c | 143 +++++++++++++++++++++++++++++++-
> >  dlls/d3dx10_43/tests/d3dx10.c   |   7 +-
> >  3 files changed, 144 insertions(+), 7 deletions(-)
>
> The patch is generally okay. I do have a number of small nits though.
>
> +static D3DX10_IMAGE_FILE_FORMAT
> wic_container_guid_to_file_format(GUID *container_format)
> +{
> - if (IsEqualGUID(container_format, &GUID_ContainerFormatBmp))
> -
>    return D3DX10_IFF_BMP;
> - else if(IsEqualGUID(container_format, &GUID_ContainerFormatJpeg))
> -
>    return D3DX10_IFF_JPG;
> - else if (IsEqualGUID(container_format, &GUID_ContainerFormatPng))
> -
>    return D3DX10_IFF_PNG;
> - else if(IsEqualGUID(container_format, &GUID_ContainerFormatDds))
> -
>    return D3DX10_IFF_DDS;
> - else if(IsEqualGUID(container_format, &GUID_ContainerFormatTiff))
> -
>    return D3DX10_IFF_TIFF;
> - else if(IsEqualGUID(container_format, &GUID_ContainerFormatGif))
> -
>    return D3DX10_IFF_GIF;
> - else if(IsEqualGUID(container_format, &GUID_ContainerFormatWmp))
> -
>    return D3DX10_IFF_WMP;
> - else
> -
>    return D3DX10_IFF_FORCE_DWORD;
>
> +}
>
> All the elses are unnecessary and should be dropped. In fact, it's
> probably better to replicate d3dx9 here and implement this with a
> table instead.
> - if (pump)
> -
>    FIXME("Thread pump is not supported yet.");
>
> Missing '\n' at the end of the message. More of the same below.
> - if ((src_data_size >= 4) && (!strncmp(src_data, "II\xbc\x00", 4)
>
> || !strncmp(src_data, "II\xbc\x01", 4)))
> - {
> -
>    FIXME("File type WMP is not supported yet.\n");
> -
>    return E_FAIL;
> - }
>
> This is not ideal. In particular, doing this with string comparisons
> looks awkward.
> I'd get rid of the check entirely and turn the "Invalid or unsupported
> image file" TRACE at the end of the file into a WARN instead.
> - hr = IWICStream_InitializeFromMemory(stream, (BYTE*)src_data,
>
> (DWORD)src_data_size);
>
> The src_data_size cast shouldn't be needed.
>
> +end:
> - if (factory)
> -
>    IWICImagingFactory_Release(factory);
> - if (stream)
> -
>    IWICStream_Release(stream);
> - if (decoder)
> -
>    IWICBitmapDecoder_Release(decoder);
> - if (frame)
> -
>    IWICBitmapFrameDecode_Release(frame);
> - if (dds_decoder)
> -
>    IWICDdsDecoder_Release(dds_decoder);
>
> Nitpick: I'd prefer to have the release happen in reverse order of
> initialization.
>
> I have a somewhat related question. Does d3dx10 support loading
> uncompressed DDS files? I know that WIC doesn't, but d3dx10 might have
> internal support for that...