Re: [5/5] gdiplus: Implement GdipImageSelectActiveFrame.

[Vincent Povirk]

> - hr = IStream_Clone(image->stream, &stream);
> - if (FAILED(hr))

That's odd. When does that happen?
> - if (stat == Ok)
> - {
> - memcpy(&new_image->format, &codec->info.FormatID, sizeof(GUID));
> - free_image_data(image);
> - if (image->type == ImageTypeBitmap)
> - *(GpBitmap *)image = *(GpBitmap *)new_image;
> - else if (image->type == ImageTypeMetafile)
> - *(GpMetafile *)image = *(GpMetafile *)new_image;
> - return Ok;
> - }

Why did you re-implement move_bitmap() ?

[5/5] gdiplus: Implement GdipImageSelectActiveFrame.

[Dmitry Timoshkov]

Vincent Povirk  wrote:
> > - hr = IStream_Clone(image->stream, &stream);
> > - if (FAILED(hr))
>
> That's odd. When does that happen?

IStream returned by SHCreateStreamOnFile (wrapped by GdipCreateStreamOnFile)
intentionally doesn't implement Clone, there are even tests for that in shlwapi.
> > - if (stat == Ok)
> > - {
> > - memcpy(&new_image->format, &codec->info.FormatID, sizeof(GUID));
> > - free_image_data(image);
> > - if (image->type == ImageTypeBitmap)
> > - *(GpBitmap *)image = *(GpBitmap *)new_image;
> > - else if (image->type == ImageTypeMetafile)
> > - *(GpMetafile *)image = *(GpMetafile *)new_image;
> > - return Ok;
> > - }
>
> Why did you re-implement move_bitmap() ?

An image can be not a bitmap.

[5/5] gdiplus: Implement GdipImageSelectActiveFrame.

[Vincent Povirk]

> > > - if (stat == Ok)
> > > - {
> > > - memcpy(&new_image->format, &codec->info.FormatID, sizeof(GUID));
> > > - free_image_data(image);
> > > - if (image->type == ImageTypeBitmap)
> > > - *(GpBitmap *)image = *(GpBitmap *)new_image;
> > > - else if (image->type == ImageTypeMetafile)
> > > - *(GpMetafile *)image = *(GpMetafile *)new_image;
> > > - return Ok;
> > > - }
> >
> > Why did you re-implement move_bitmap() ?
>
> An image can be not a bitmap.

Yes, but a non-bitmap (metafile) cannot have multiple frames.

[5/5] gdiplus: Implement GdipImageSelectActiveFrame.

[Dmitry Timoshkov]

Vincent Povirk  wrote:
> > > Why did you re-implement move_bitmap() ?
> >
> > An image can be not a bitmap.
>
> Yes, but a non-bitmap (metafile) cannot have multiple frames.

It actually can have records which represent embedded EMFs.

[5/5] gdiplus: Implement GdipImageSelectActiveFrame.

[Vincent Povirk]

> > Yes, but a non-bitmap (metafile) cannot have multiple frames.
>
> It actually can have records which represent embedded EMFs.

Really? Well, OK then.
> - if (stat == Ok)
> - {
> -
>    memcpy(&new_image->format, &codec->info.FormatID, sizeof(GUID));
> -
>    free_image_data(image);
> -
>    if (image->type == ImageTypeBitmap)
> -
>        *(GpBitmap *)image = *(GpBitmap *)new_image;
> -
>    else if (image->type == ImageTypeMetafile)
> -
>        *(GpMetafile *)image = *(GpMetafile *)new_image;
> -
>    return Ok;
> - }

You appear to be leaking new_image.

[5/5] gdiplus: Implement GdipImageSelectActiveFrame.

[Dmitry Timoshkov]

Vincent Povirk  wrote:
> > - if (stat == Ok)
> > - {
> > -
> >    memcpy(&new_image->format, &codec->info.FormatID, sizeof(GUID));
> > -
> >    free_image_data(image);
> > -
> >    if (image->type == ImageTypeBitmap)
> > -
> >        *(GpBitmap *)image = *(GpBitmap *)new_image;
> > -
> >    else if (image->type == ImageTypeMetafile)
> > -
> >        *(GpMetafile *)image = *(GpMetafile *)new_image;
> > -
> >    return Ok;
> > - }
>
> You appear to be leaking new_image.

Thanks. I hope that won't prevent accepting other patches in the series.