Creating multi font with same data without set FontDataOwnedByAtlas false, will crash #6339

New issue

Closed

opened 2023-04-16 21:23:53 +00:00 by InJoins · 1 comment

InJoins commented

2023-04-16 21:23:53 +00:00

(Migrated from github.com)

In recent days, I have been debugging a BUG.

In the process of switching the APP to the background and back again, the APP crashed randomly.
Sometimes it took dozens of minutes for the APP to crash.

After several days of digging and rewriting, I finally found the problem.
When using IMGUI to create multiple font sizes through the same TTF file data, the memory will be free twice when cleaning fontatlas!!

As a result, the memory has unexpected location crashes......
The location that causes the APP to crash is different each time.
I looked at all the possibilities... I even rewrote the entire framework from NDK-based to Java-based.
let the whole app just use IMGUI with only a small part of the code based on C++.

IMGUI has excellent performance, especially IMDRAWLIST, which I used separately as a drawing interface. I didn't use the UI part of IMGUI itself.

BELOW IS HOW I USED IMFONT----->
fontAtlas = IM_NEW(ImFontAtlas)();
long fontDataSize;
void* fontData = Platform.File.CreateDataFromResource("font.ttf", &fontDataSize);
fontDefault = fontAtlas->AddFontFromMemoryTTF(fontData, fontDataSize, 20.0f * uiScale(), &fontConfig, fontAtlas->GetGlyphRangesChineseFull());
fontPitch = fontAtlas->AddFontFromMemoryTTF(fontData, fontDataSize, 42.0f * uiScale(), &fontConfig, fontAtlas->GetGlyphRangesChineseFull());

HOW TO FIX ----->
fontAtlas = IM_NEW(ImFontAtlas)();
long fontDataSize;
void* fontData = Platform.File.CreateDataFromResource("font.ttf", &fontDataSize);
ImFontConfig fontConfig;
fontConfig.FontDataOwnedByAtlas = false;
fontDefault = fontAtlas->AddFontFromMemoryTTF(fontData, fontDataSize, 20.0f * uiScale(), &fontConfig, fontAtlas->GetGlyphRangesChineseFull());
fontPitch = fontAtlas->AddFontFromMemoryTTF(fontData, fontDataSize, 42.0f * uiScale(), &fontConfig, fontAtlas->GetGlyphRangesChineseFull());
free(fontData);

These three lines code fix the bug.

Of course, I'm trying to solve this problem from the user level. If who want to solve this problem from inside IMGUI, you need to determine whether the input TTFs data that belongs to the same memory, and then treat it differently. Or in the release process, determine whether the memory of a font has been released more than once. Of course, I won't go into that. I'm tired of debugging this random crash.

In recent days, I have been debugging a BUG. In the process of switching the APP to the background and back again, the APP crashed randomly. Sometimes it took dozens of minutes for the APP to crash. After several days of digging and rewriting, I finally found the problem. When using IMGUI to create multiple font sizes through the same TTF file data, the memory will be free twice when cleaning fontatlas!! As a result, the memory has unexpected location crashes...... The location that causes the APP to crash is different each time. I looked at all the possibilities... I even rewrote the entire framework from NDK-based to Java-based. let the whole app just use IMGUI with only a small part of the code based on C++. IMGUI has excellent performance, especially IMDRAWLIST, which I used separately as a drawing interface. I didn't use the UI part of IMGUI itself. BELOW IS HOW I USED IMFONT-----> fontAtlas = IM_NEW(ImFontAtlas)(); long fontDataSize; void* fontData = Platform.File.CreateDataFromResource("font.ttf", &fontDataSize); fontDefault = fontAtlas->AddFontFromMemoryTTF(fontData, fontDataSize, 20.0f * uiScale(), &fontConfig, fontAtlas->GetGlyphRangesChineseFull()); fontPitch = fontAtlas->AddFontFromMemoryTTF(fontData, fontDataSize, 42.0f * uiScale(), &fontConfig, fontAtlas->GetGlyphRangesChineseFull()); HOW TO FIX -----> fontAtlas = IM_NEW(ImFontAtlas)(); long fontDataSize; void* fontData = Platform.File.CreateDataFromResource("font.ttf", &fontDataSize); **ImFontConfig fontConfig; fontConfig.FontDataOwnedByAtlas = false;** fontDefault = fontAtlas->AddFontFromMemoryTTF(fontData, fontDataSize, 20.0f * uiScale(), &fontConfig, fontAtlas->GetGlyphRangesChineseFull()); fontPitch = fontAtlas->AddFontFromMemoryTTF(fontData, fontDataSize, 42.0f * uiScale(), &fontConfig, fontAtlas->GetGlyphRangesChineseFull()); **free(fontData);** These three lines code fix the bug. Of course, I'm trying to solve this problem from the user level. If who want to solve this problem from inside IMGUI, you need to determine whether the input TTFs data that belongs to the same memory, and then treat it differently. Or in the release process, determine whether the memory of a font has been released more than once. Of course, I won't go into that. I'm tired of debugging this random crash.

ocornut commented

2023-04-17 08:43:43 +00:00

(Migrated from github.com)

Hello,

By default the pointer passed to AddFontFromMemoryTTF() gets its ownership moved to ImFontAtlas, by which I mean that ImFontAtlas can free it, in this case it will attempt to free it twice.

Your fix is correct and exactly what is documented for this use case.
Therefore I am not sure what you are asking? You found the correct fix.

I agree this isn't a great default behavior that AddFontFromMemoryTTF() takes ownership, I think this will evolve in a next iteration of the font API. I don't think it is desirable either to perform a search on previous font to avoid taking ownership twice, we should just eventually remove that ownership-by-default thing.

Hello, By default the pointer passed to `AddFontFromMemoryTTF()` gets its ownership moved to ImFontAtlas, by which I mean that ImFontAtlas can free it, in this case it will attempt to free it twice. Your fix is correct and exactly what is documented for this use case. Therefore I am not sure what you are asking? You found the correct fix. I agree this isn't a great default behavior that `AddFontFromMemoryTTF()` takes ownership, I think this will evolve in a next iteration of the font API. I don't think it is desirable either to perform a search on previous font to avoid taking ownership twice, we should just eventually remove that ownership-by-default thing.

👍 1