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.
On 30 Jun 2017, at 22:24, Vincent Povirk madewokherd@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
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
-
Piotr Caban -
Vincent Povirk