Re: gdiplus: Reimplement metafile loading using gdi32 instead of IPicture.

[Nikolay Sivov]

On 17.08.2016 11:26, Sebastian Lackner wrote:
> +static GpStatus load_wmf(IStream *stream, GpMetafile **metafile)
> +{
> - WmfPlaceableFileHeader pfh;
> - GpStatus status = GenericError;
> - BOOL is_placeable = FALSE;
> - LARGE_INTEGER seek;
> - METAHEADER mh;
> - HMETAFILE hmf;
> - HRESULT hr;
> - UINT size;
> - void *buf;
> -
> - hr = IStream_Read(stream, &mh, sizeof(mh), &size);
> - if (hr != S_OK || size != sizeof(mh))
> -
>    return GenericError;
> -
> - if (mh.mtType == 0xcdd7 && mh.mtHeaderSize == 0x9ac6)

It'd be more readable to test this as UINT32 it's supposed to be,
and having this constant defined to GDIP_WMF_PLACEABLEKEY (locally in
gdiplus, it's only mentioned in comment in SDK) will help looking it up.
> - {
> -
>    is_placeable = TRUE;
> -
> -
>    seek.QuadPart = 0;
> -
>    hr = IStream_Seek(stream, seek, STREAM_SEEK_SET, NULL);
> -
>    if (FAILED(hr)) return hresult_to_status(hr);
> -
> -
>    hr = IStream_Read(stream, &pfh, sizeof(pfh), &size);
> -
>    if (hr != S_OK || size != sizeof(pfh))
> -
>        return GenericError;
> -
> -
>    hr = IStream_Read(stream, &mh, sizeof(mh), &size);
> -
>    if (hr != S_OK || size != sizeof(mh))
> -
>        return GenericError;
> - }
> -
> - seek.QuadPart = is_placeable ? sizeof(pfh) : 0;
> - hr = IStream_Seek(stream, seek, STREAM_SEEK_SET, NULL);
> - if (FAILED(hr)) return hresult_to_status(hr);
> -
> - buf = heap_alloc(mh.mtSize * 2);
> - if (!buf) return OutOfMemory;
> -
> - hr = IStream_Read(stream, buf, mh.mtSize * 2, &size);
> - if (hr == S_OK && size == mh.mtSize * 2)
> - {
> -
>    hmf = SetMetaFileBitsEx(mh.mtSize * 2, buf);
> -
>    if (hmf)
> -
>    {
> -
>        status = GdipCreateMetafileFromWmf(hmf, TRUE, is_placeable ? &pfh : NULL, metafile);
> -
>        if (status != Ok)
> -
>            DeleteMetaFile(hmf);
> -
>    }
> - }
> -
> - heap_free(buf);
> - return status;
>
> +}
> -
>
> +static GpStatus decode_image_wmf(IStream *stream, GpImage **image)
>  {
> - IPicture *pic;
> -
>
> GpMetafile *metafile;
> -
>
> GpStatus status;
>
> TRACE("%p %p\n", stream, image);
> - if(!stream || !image)
> - if (!stream || !image)
>     return InvalidParameter;
> - if(OleLoadPicture(stream, 0, FALSE, &IID_IPicture,
> -
>    (LPVOID*) &pic) != S_OK){
> -
>    TRACE("Could not load picture\n");
> - status = load_wmf(stream, &metafile);
> - if (status != Ok)
> - {
> -
>    TRACE("Could not load metafile\n");
> -
>    return status;
> - }
> -
> - *image = (GpImage *)metafile;
> - TRACE("<-- %p\n", *image);
> -
> - return Ok;
>
> +}
> -
>
> +static GpStatus load_emf(IStream *stream, GpMetafile **metafile)
> +{
> - GpStatus status = GenericError;
> - LARGE_INTEGER seek;
> - ENHMETAHEADER emh;
> - HENHMETAFILE hemf;
> - HRESULT hr;
> - UINT size;
> - void *buf;
> -
> - hr = IStream_Read(stream, &emh, sizeof(emh), &size);
> - if (hr != S_OK || size != sizeof(emh) || emh.dSignature != ENHMETA_SIGNATURE)
>     return GenericError;
> -
> - seek.QuadPart = 0;
> - hr = IStream_Seek(stream, seek, STREAM_SEEK_SET, NULL);
> - if (FAILED(hr)) return hresult_to_status(hr);
> -
> - buf = heap_alloc(emh.nBytes);
> - if (!buf) return OutOfMemory;
> -
> - hr = IStream_Read(stream, buf, emh.nBytes, &size);
> - if (hr == S_OK && size == emh.nBytes)
> - {
> -
>    hemf = SetEnhMetaFileBits(emh.nBytes, buf);
> -
>    if (hemf)
> -
>    {
> -
>        status = GdipCreateMetafileFromEmf(hemf, TRUE, metafile);
> -
>        if (status != Ok)
> -
>            DeleteEnhMetaFile(hemf);
> -
>    }
> }
> - /* FIXME: missing initialization code */
> - *image = heap_alloc_zero(sizeof(GpMetafile));
> - if(!*image) return OutOfMemory;
> - (*image)->type = ImageTypeMetafile;
> - (*image)->decoder = NULL;
> - (*image)->picture = pic;
> - (*image)->flags   = ImageFlagsNone;
> - (*image)->frame_count = 1;
> - (*image)->current_frame = 0;
> - (*image)->palette = NULL;
> - list_init(&(*(GpMetafile**)image)->containers);
> - heap_free(buf);
> - return status;
>
> +}
> -
>
> +static GpStatus decode_image_emf(IStream *stream, GpImage **image)
> +{
> -
>
> GpMetafile *metafile;
> -
>
> GpStatus status;
> -
> -
>
> TRACE("%p %p\n", stream, image);
> -
> -
>
> if (!stream || !image)
> -
>    return InvalidParameter;
> -
> -
>
> status = load_emf(stream, &metafile);
> -
>
> if (status != Ok)
> -
>
> {
> -
>    TRACE("Could not load metafile\n");
> -
>    return status;
> -
>
> }
> -
>
> *image = (GpImage *)metafile;
> TRACE("<-- %p\n", *image);
>
> return Ok;
>
> @@ -4721,7 +4750,7 @@ static const struct image_codec codecs[NUM_CODECS] = {
>              /* SigMask */            emf_sig_mask,
>          },
>          NULL,
> -
>    decode_image_olepicture_metafile,
> -
>    decode_image_emf,
>    NULL
> },
>    {
>
> @@ -4741,7 +4770,7 @@ static const struct image_codec codecs[NUM_CODECS] = {
>              /* SigMask */            wmf_sig_mask,
>          },
>          NULL,
> -
>    decode_image_olepicture_metafile,
> -
>    decode_image_wmf,
>    NULL
> },
>    {
>
> diff --git a/dlls/gdiplus/metafile.c b/dlls/gdiplus/metafile.c
> index a854e55..8b93e81 100644
> --- a/dlls/gdiplus/metafile.c
> +++ b/dlls/gdiplus/metafile.c
> @@ -317,7 +317,6 @@ GpStatus WINGDIPAPI GdipRecordMetafile(HDC hdc, EmfType type, GDIPCONST GpRectF
>      }
>  (*metafile)->image.type = ImageTypeMetafile;
> - (*metafile)->image.picture = NULL;
> (*metafile)->image.flags   = ImageFlagsNone;
> (*metafile)->image.palette = NULL;
> (*metafile)->image.xres = dpix;
>
> diff --git a/dlls/gdiplus/tests/image.c b/dlls/gdiplus/tests/image.c
> index ad84feb..cabd2fce 100644
> --- a/dlls/gdiplus/tests/image.c
> +++ b/dlls/gdiplus/tests/image.c
> @@ -1446,19 +1446,19 @@ static void test_loadwmf(void)
>  stat = GdipGetImageBounds(img, &bounds, &unit);
>  expect(Ok, stat);
> - todo_wine expect(UnitPixel, unit);
> - expect(UnitPixel, unit);
> expectf(0.0, bounds.X);
> expectf(0.0, bounds.Y);
> - todo_wine expectf(320.0, bounds.Width);
> - todo_wine expectf(320.0, bounds.Height);
> -
>
> expectf(320.0, bounds.Width);
> -
>
> expectf(320.0, bounds.Height);
>
> stat = GdipGetImageHorizontalResolution(img, &res);
> expect(Ok, stat);
> - todo_wine expectf(1440.0, res);
> -
>
> expectf(1440.0, res);
>
> stat = GdipGetImageVerticalResolution(img, &res);
> expect(Ok, stat);
> - todo_wine expectf(1440.0, res);
> -
>
> expectf(1440.0, res);
>
> memset(&header, 0, sizeof(header));
> stat = GdipGetMetafileHeaderFromMetafile((GpMetafile*)img, &header);

gdiplus: Reimplement metafile loading using gdi32 instead of IPicture.

[Sebastian Lackner]

On 17.08.2016 11:28, Nikolay Sivov wrote:
> On 17.08.2016 11:26, Sebastian Lackner wrote:
> > +static GpStatus load_wmf(IStream *stream, GpMetafile **metafile)
> > +{
> > - WmfPlaceableFileHeader pfh;
> > - GpStatus status = GenericError;
> > - BOOL is_placeable = FALSE;
> > - LARGE_INTEGER seek;
> > - METAHEADER mh;
> > - HMETAFILE hmf;
> > - HRESULT hr;
> > - UINT size;
> > - void *buf;
> > -
> > - hr = IStream_Read(stream, &mh, sizeof(mh), &size);
> > - if (hr != S_OK || size != sizeof(mh))
> > -
> >    return GenericError;
> > -
> > - if (mh.mtType == 0xcdd7 && mh.mtHeaderSize == 0x9ac6)
>
> It'd be more readable to test this as UINT32 it's supposed to be,
> and having this constant defined to GDIP_WMF_PLACEABLEKEY (locally in
> gdiplus, it's only mentioned in comment in SDK) will help looking it up.

Probably yes, thanks. I'll wait if Vincent has any other feedback
(for example if he's fine with the simplified error handling for
IStream_Read) before sending a new version.

Regards,
Sebastian