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

Fixed memory leak of font data #5763

Conversation

yoshinorisano
Copy link

@yoshinorisano yoshinorisano commented Oct 9, 2022

There is a memory leak where font data is handled.
I found it by using _CrtDumpMemoryLeaks() , which is available in Visual Studio.
After applying this change, no memory leak detected.

The memory leak detector reported like this:

Detected memory leaks!
Dumping objects ->
{491} normal block at 0x00000254A12E2070, 9527428 bytes long.
 Data: <ttcf           (> 74 74 63 66 00 02 00 00 00 00 00 04 00 00 00 28 
Object dump complete

@yoshinorisano
Copy link
Author

Could you review this?

@ocornut
Copy link
Owner

ocornut commented Oct 11, 2022

Hello,

Thanks for the PR.
This looks incorrect. If font_cfg->FontDataOwnedByAtlas is false we don't claim overship over font_cfg->FontData as it is owned by application and therefore should be freed by the allocation.

(One purpose of this is to allow application to have FontData point to e.g. read-only memory).

@thedemons
Copy link
Contributor

You must set FontDataOwnedByAtlas to false if you were loading the fonts from resources or somewhere in memory that you allocated on your own and not by ImGui, so in this case, it's not a bug

	static const ImWchar rangesAwesome[] = { ICON_MIN_FA, ICON_MAX_FA, 0 };
	ImFontConfig configAwesome;
	configAwesome.MergeMode = true;
	configAwesome.FontDataOwnedByAtlas = false; // don't free memory
	configAwesome.OversampleH = configAwesome.OversampleV = 1;
	fontAtlas->AddFontFromMemoryTTF((void*)fa_solid_900, sizeof(fa_solid_900), 12.f, &configAwesome, rangesAwesome);

@yoshinorisano
Copy link
Author

@ocornut @thedemons
Sorry, I didn't understand the usage of the ImFontConfig's FontDataOwnedByAtlas member...
In fact, I did copy & paste some code from somewhere, in that code, FontDataOwnedByAtlas = false was coded.
Now I understand Dear ImGui much more.
Thank you so much!!

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.

None yet

3 participants