Re: [PATCH 1/9 v3] gdiplus: Add partial support for GdipDrawImagePointsRect on metafile

[Dmitry Timoshkov]

Piotr Caban <piotr(a)codeweavers.com> wrote:
> +static GpStatus METAFILE_CreateCompressedImageStream(GpImage *image, IStream **stream, DWORD *size)
> +{
> +    static const CLSID png_clsid = { 0x557cf406, 0x1a04, 0x11d3,
> +        { 0x9a, 0x73, 0x0, 0x0, 0xf8, 0x1e, 0xf3, 0x2e } };
> +    LARGE_INTEGER zero;
> +    STATSTG statstg;
> +    GpStatus stat;
> +    HRESULT hr;
> +
> +    *size = 0;
> +
> +    hr = CreateStreamOnHGlobal(NULL, TRUE, stream);
> +    if (FAILED(hr)) return hresult_to_status(hr);
> +
> +    stat = GdipSaveImageToStream(image, *stream, &png_clsid, NULL);

Even internally gdiplus shouldn't do this kind of hacks IMO, if there is
a need for a shortcut for a particular encoder class id it's better to
add a helper. Have a look at the encoder/decoder tests how it's supposed
to look like.

-- 
Dmitry.

[PATCH 1/9 v3] gdiplus: Add partial support for GdipDrawImagePointsRect on metafile

[Piotr Caban]

On 06/30/17 16:14, Dmitry Timoshkov wrote:
> Piotr Caban <piotr(a)codeweavers.com> wrote:
> > +static GpStatus METAFILE_CreateCompressedImageStream(GpImage *image, IStream **stream, DWORD *size)
> > +{
> > +    static const CLSID png_clsid = { 0x557cf406, 0x1a04, 0x11d3,
> > +        { 0x9a, 0x73, 0x0, 0x0, 0xf8, 0x1e, 0xf3, 0x2e } };
> > +    LARGE_INTEGER zero;
> > +    STATSTG statstg;
> > +    GpStatus stat;
> > +    HRESULT hr;
> > +
> > +    *size = 0;
> > +
> > +    hr = CreateStreamOnHGlobal(NULL, TRUE, stream);
> > +    if (FAILED(hr)) return hresult_to_status(hr);
> > +
> > +    stat = GdipSaveImageToStream(image, *stream, &png_clsid, NULL);
>
> Even internally gdiplus shouldn't do this kind of hacks IMO, if there is
> a need for a shortcut for a particular encoder class id it's better to
> add a helper. Have a look at the encoder/decoder tests how it's supposed
> to look like.
> I've done it this way because I don't think we should go through 
> enumerating all supported formats internally. How about using 
> encode_image_png function directly?

Thanks,
Piotr

[PATCH 1/9 v3] gdiplus: Add partial support for GdipDrawImagePointsRect on metafile

[Dmitry Timoshkov]

Piotr Caban <piotr.caban(a)gmail.com> wrote:
> > Even internally gdiplus shouldn't do this kind of hacks IMO, if there is
> > a need for a shortcut for a particular encoder class id it's better to
> > add a helper. Have a look at the encoder/decoder tests how it's supposed
> > to look like.
> > I've done it this way because I don't think we should go through 
> > enumerating all supported formats internally.

Why not?
> How about using 
> encode_image_png function directly?

I personally don't like it, this is not a performance critical code.

While looking at this I noticed that 'clsid' parameter in encode_image_func()
is unused, that's probably a leftover from previous implementation.

-- 
Dmitry.

[PATCH 1/9 v3] gdiplus: Add partial support for GdipDrawImagePointsRect on metafile

[Piotr Caban]

On 06/30/17 16:46, Dmitry Timoshkov wrote:
> Piotr Caban <piotr.caban(a)gmail.com> wrote:
> > > Even internally gdiplus shouldn't do this kind of hacks IMO, if there is
> > > a need for a shortcut for a particular encoder class id it's better to
> > > add a helper. Have a look at the encoder/decoder tests how it's supposed
> > > to look like.
> > > I've done it this way because I don't think we should go through
> > > enumerating all supported formats internally.
>
> Why not?
> Mainly because it unnecessarily complicates the code. Also I don't see 
> any advantages from doing it this way.