Re: [PATCH 03/10 v4] gdiplus: Add functions for managing metafile objects id

[Vincent Povirk]

This seems like overkill to me?

I don't see the need for per-object refcounting. Every drawing
operation that uses an object is just going to end with a series of
releases, and after it returns all refcounts should be 0.

+    /* TODO: object id should be released on object dispose */
I don't think this approach is possible. It would require every object
to be aware of which metafiles are using it, and every operation that
modifies the object would have to release them. Bitmap objects can
refer to an application buffer that could be modified without any call
into gdiplus. (Or, Bitmaps could be modified by an ImageAttributes, in
which case we can't reuse them except with an identical
ImageAttributes.)

If we wanted to reuse Bitmaps, we'd have to compare the data. For
other large types like Paths, maybe keeping an id to detect whether it
changed would make sense. For types of bounded size, it probably would
be easier to store a complete copy.

[PATCH 03/10 v4] gdiplus: Add functions for managing metafile objects id

[Piotr Caban]

> On 30 Jun 2017, at 22:24, Vincent Povirk <madewokherd(a)gmail.com> wrote:
>
> This seems like overkill to me?
>
> I don't see the need for per-object refcounting. Every drawing
> operation that uses an object is just going to end with a series of
> releases, and after it returns all refcounts should be 0.
>
> +    /* TODO: object id should be released on object dispose */
> I don't think this approach is possible. It would require every object
> to be aware of which metafiles are using it, and every operation that
> modifies the object would have to release them. Bitmap objects can
> refer to an application buffer that could be modified without any call
> into gdiplus. (Or, Bitmaps could be modified by an ImageAttributes, in
> which case we can't reuse them except with an identical
> ImageAttributes.)
>
> If we wanted to reuse Bitmaps, we'd have to compare the data. For
> other large types like Paths, maybe keeping an id to detect whether it
> changed would make sense. For types of bounded size, it probably would
> be easier to store a complete copy.
> Currently the implementation only overwrites ids in the same way as native does. It’s there to make sure we never run out of IDs. I think it’s nice that the functions are failsafe. It’s also closer to native behaviour.

I think that everything else can be figured out later. The comment is based on observation of native dll behaviour. I’ve checked what happens when new bitmaps are created and drawn to metafile in a loop. All of the bitmaps contained exactly the same data. In this case the IDs were not freed unless we run out of free IDs. After adding GdiDisposeImage call to the loop the IDs got released. I think it makes the comment valid even if we never implement this behaviour.

Thanks,
Piotr

[PATCH 03/10 v4] gdiplus: Add functions for managing metafile objects id

[Piotr Caban]

On 06/30/17 22:24, Vincent Povirk wrote:
> This seems like overkill to me?
> I just realized that maybe you were only referring to the comment. If 
> you prefer I can change it to something like:
> TODO: object id should not be released here
> or I can even remove the function for releasing the IDs for now.
> I don't see the need for per-object refcounting. Every drawing
> operation that uses an object is just going to end with a series of
> releases, and after it returns all refcounts should be 0.
> Currently there's no per-object refcounting. There's only kind of a 
> timestamp that stores when object id was last used. Thanks to it the 
> function frees object that was not used for the longest time first. This 
> will be needed for reusing of object IDs. Also thanks to it we don't 
> need to free the IDs on all error paths.

Thanks,
Piotr