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

[Vincent Povirk]

I'd probably be OK with this as-is, but since you asked for feedback:
> @@ -2219,12 +2155,6 @@ GpStatus WINGDIPAPI GdipGetImageBounds(GpImage *image, GpRectF *srcRect,
>          srcRect->Height = (REAL) ((GpBitmap*)image)->height;
>          *srcUnit = UnitPixel;
>      }
> -    else{
> -        srcRect->X = srcRect->Y = 0.0;
> -        srcRect->Width = ipicture_pixel_width(image->picture);
> -        srcRect->Height = ipicture_pixel_height(image->picture);
> -        *srcUnit = UnitPixel;
> -    }
>
> TRACE("returning (%f, %f) (%f, %f) unit type %d\n", srcRect->X, srcRect->Y,
>            srcRect->Width, srcRect->Height, *srcUnit);

Even for ipicture metafiles, this case wasn't really used before,
except when given an invalid image object (type isn't bitmap or
metafile) where it would likely crash. We should probably return
InvalidParameter here.

Same with the other GdipGetImage* functions where we're removing else clauses.

I am OK with the simplified error handling from IStream_Read.
> +    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);
> +        }
>      }
>
> +    heap_free(buf);
> +    return status;
> +}

I guess this works, but it's kind of confusing how status gets set
here when IStream_Read or SetEnhMetaFileBits fails. I'd rather it be
set closer to the failure.

gdiplus: Reimplement metafile loading using gdi32 instead of IPicture.

[Sebastian Lackner]

On 17.08.2016 18:42, Vincent Povirk wrote:
> I'd probably be OK with this as-is, but since you asked for feedback:
> > @@ -2219,12 +2155,6 @@ GpStatus WINGDIPAPI GdipGetImageBounds(GpImage *image, GpRectF *srcRect,
> >          srcRect->Height = (REAL) ((GpBitmap*)image)->height;
> >          *srcUnit = UnitPixel;
> >      }
> > -    else{
> > -        srcRect->X = srcRect->Y = 0.0;
> > -        srcRect->Width = ipicture_pixel_width(image->picture);
> > -        srcRect->Height = ipicture_pixel_height(image->picture);
> > -        *srcUnit = UnitPixel;
> > -    }
> >
> > TRACE("returning (%f, %f) (%f, %f) unit type %d\n", srcRect->X, srcRect->Y,
> >            srcRect->Width, srcRect->Height, *srcUnit);
>
> Even for ipicture metafiles, this case wasn't really used before,
> except when given an invalid image object (type isn't bitmap or
> metafile) where it would likely crash. We should probably return
> InvalidParameter here.
>
> Same with the other GdipGetImage* functions where we're removing else clauses.
>
> I am OK with the simplified error handling from IStream_Read.
> > +    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);
> > +        }
> >      }
> >
> > +    heap_free(buf);
> > +    return status;
> > +}
>
> I guess this works, but it's kind of confusing how status gets set
> here when IStream_Read or SetEnhMetaFileBits fails. I'd rather it be
> set closer to the failure.

Thanks for the feedback, I've just sent a new version. Please let me know if
it addresses all your concerns. In case you are planning to implement other
metafile gdiplus functions, you might also want to take a look here to check
on what else Dmitry has been working on in the past months:
https://github.com/wine-compholio/wine-staging/tree/master/patches/gdiplus-G...
I can try to clean them up when I have some time, however I also don't mind
if you take a look yourself and refactor / rewrite them (if necessary) for
upstream inclusion.

Best regards,
Sebastian