search

felixc / rexiv2

Rust library for read/write access to media-file metadata (Exif, XMP, and IPTC)
GNU General Public License v3.0
79 stars 17 forks source link

Fix a memory leak and change the way preview images are handled #39

Open Naarakah opened 4 years ago

Naarakah commented 4 years ago

I have fixed a memory leak in get_thumbnail and changed the way preview images are loaded to skip a (potentially costly) copy. This will slightly change the API (Metadata::get_thumbnail and PreviewImage::get_data).

felixc commented 4 years ago

Hello! Thank you for these improvements!

Are the copy optimization and the memory leak fix closely related, or would it be possible to land these changes separately — and is the API change required for both or just one of them?

I ask because I want to be able to put together an accurate changelog for the release, as well as a clear and complete commit history where each change fixes one thing at a time. Right now I can't quite tell which parts of the PR do what thing.

Could you perhaps please describe the source of the memory leak and what the fix is, and also where the copy optimization is and how it works? That would be extra helpful, even just for me to learn from.

Thanks again for the patch!

Naarakah commented 4 years ago

Oh yeah sorry, the changes can be split in two commits but are kinda related nonetheless. The gexiv2::gexiv2_metadata_get_exif_thumbnail function allocates and the memory was never freed, so the two solutions are either to make a copy of the thumbnail data then free the original or to use the data returned by the function and remember to free it later. I chose the second option, which requires a API break because the return type is no longer just a slice but is now a proxy type that also contains a pointer to the memory we need to free when the value is dropped.

The other changes are the same optimization (instead of call, copy, free, we do call, return type with information needed to free, free on drop), but do not require an API break because the function were already returning a custom type (the type definition changes but it's an opaque type thus there is no API break).