Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Naarakah
Copy link

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
Copy link
Owner

felixc commented May 18, 2020

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
Copy link
Author

Naarakah commented May 19, 2020

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants