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

[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...