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

[Dmitry Timoshkov]

Piotr Caban  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.

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

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