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 <madewokherd(a)gmail.com> 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.

-- 
Dmitry.

[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 <madewokherd(a)gmail.com> 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.

-- 
Dmitry.

[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 <madewokherd(a)gmail.com> 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.

-- 
Dmitry.