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 crash related to not found glyphs. #1789

Merged
merged 2 commits into from
Dec 1, 2024

Conversation

saurtron
Copy link
Collaborator

@saurtron saurtron commented Nov 29, 2024

Work done

  • Fix a crash when several not found glyphs are used.
  • Debugged but not 100% sure what's going on.

Remarks

Note this all happens with current standard engine, also with custom compiled without any of the recent fontconfig changes, also fontconfig here doesn't take part since I'm using UseFontConfigLib = 0 anyways.

  • found this while testing the engine with bar and option UseFontConfigLib = 0 and some font that doesn't have alphabet glyphs inside BAR/fonts.

    • that makes the engine load loads of unknown glyphs since it will try to preload the alphabet but its not there (also not the best).
    • not sure if it also happens on windows
    • also noticed here having games provide their fonts inside fonts/ and loading fontconfig/engine fonts from fonts/ is not very good since then we will preload all of them even if not used due to VFS.
  • also removed assigning chr to the GlyphInfo (glyph.letter = ch) since it already has a previous character so no reason to change it here. We look it up in the std::pair->first anyways, and changing the char here can make it not share some other caches with other characters sharing the same glyph (rn the kerninginfo).

  • Was trying UseFontConfigLib = 0 as a possible workaround for the Zorin Os player, and decided to investigate the crash since it might be affecting other players too. Funnily the workaround pbbly can't be used by the player since afaik when playing with launcher it will overwrite the springsettings.cfg config all the time.

Crash debug

The affected code:

if (iter != glyphs.end()) {
auto& glyph = glyphs[ch];
glyph = iter->second;
glyph.letter = ch;
return;

Tbh not entirely sure why it crashes XD. I know but don't know the ultimate reasons.

I investigated, and seems like sometimes (it's only after a few times entering the loop, not sure why, maybe 20) the assignment operator will get an empty GlyphInfo for some reason when calling glyph = iter->second;.

A dirtier workaround seems to be doing some glyphs[ch] access before the ``return`, in that case it all works!! Might be something with the compiler? Corruption before this?

Sorry that I don't have a more final analysis :-P

Logs

These are custom logs, interpret like:

glyph-dupe-ok.txt

glyph-dupe-crash.txt (just printing the chain that lead to the crash, there were more dupe glyphs before this)

The crash happens after [t=00:00:12.223282][f=-000001] [Font] DUPE GLYPH Noto Sans Symbols2 ['] 0 0x5b0d599a93c0 / 0 [~] 0x5b0dae3dcea0 21 0.588235 .

That means the iter->second.face has 21 count on the smart pointer.

The logs also show iter->second is ok, also everything is fine after auto& glyph = glyphs[ch];, but then when doing auto& glyph = glyphs[ch]; the assignment operator gets an uninitialized 'other' and crashes (or that's my interpretation).

They logs are printed roughly with this code:

        if (iter != glyphs.end()) {
                LOG("DUPE GLYPH %s [%lc] %d %p / %d [%lc] %p %ld %f", f->face->family_name, ch, index, f->face, iter->second.index, iter->second.letter, iter->second.face.get(), iter->second.face.use_count(), iter->second.advance);
                LOG("COPY GLYPH %lc %ld", iter->second.letter, iter->second.face.use_count());
                //glyphs[ch] = iter->second; <-- fix I'm applying
                auto& glyph = glyphs[ch];
                glyph = iter->second;
                LOG("COPIED GLYP %lc", glyph.letter);
                glyph.letter = ch;
                //glyphs[ch].letter = ch; // this makes it all work instead of crash!
                //LOG("COPIED GLYP %lc", glyphs[ch].letter); // this also makes it work instead of crash!
                return;
        }

// assignment operator (we don't have it but I added it to debug or try to find a fix inside):

GlyphInfo& GlyphInfo::operator=(const GlyphInfo& other)
{
        size = other.size;
        texCord = other.texCord;
        shadowTexCord = other.shadowTexCord;
        advance = other.advance;
        height = other.height;
        descender = other.descender;
        index = other.index;
        letter = other.letter;
        LOG("INSIDE COPY GLYPH operator %p %f %lc", other.face.get(), other.advance, other.letter);
        face = other.face;
        return *this;
}

@saurtron saurtron changed the title Fix crash with not found glyphs. Fix crash related to not found glyphs. Nov 29, 2024
@sprunk
Copy link
Collaborator

sprunk commented Nov 29, 2024

glyphs is a spring::unordered_map:

spring::unordered_map<char32_t, GlyphInfo> glyphs; // UTF32 -> GlyphInfo

This may resolve to a std::unordered_map under at least some circumstances:

namespace spring {
using std::unordered_map;

Using operator[] on a std::unorderd_map inserts a default-constructed element if one doesn't already exist, and inserting will invalidate iterators (including our iter we're trying to dereference) if this triggers a rehash. Presumably something similar concerns the other types that could be under spring::unordered_map.

So we shouldn't use iter after insertion; perhaps a proper fix would be something like

if (iter != glyphs.end()) {
	const auto [insertee, success] = glyphs.emplace(ch, iter->second);
	assert(success);
	insertee->letter = ch; // the PR removes this, but i assume `letter` is about the actual shape so could matter for kerning or whatnot so probably good to keep. idk.
        return;
}

I guess that's what the current state of the PR does under the hood already, though without the spooky unordered_map::operator[].

@sprunk
Copy link
Collaborator

sprunk commented Nov 29, 2024

@p2004a seems to be good at C++ STL so maybe he could also take a look.

@sprunk sprunk requested a review from p2004a November 29, 2024 14:33
@saurtron
Copy link
Collaborator Author

saurtron commented Nov 29, 2024

insertee->letter = ch; // the PR removes this, but i assume `letter` is about the actual shape so could matter for kerning or whatnot so probably good to keep. idk.

The shape is actually determined by the insertee->face and the insertee->index attribute.

The code here is inserting different chars that resolve to the same glyph, which was stored before with a certain letter as key and the same letter inside the glyph.

Changing the letter here just makes us store more kerning hashes later on and recalculate kerning, but has no other effect imo since at the end of the day it's going to be the same glyph and I think it should have the same kerning.

It does look a bit awkward and could be put back into the PR but imo it's more optimal this way. Maybe instead of letter could be called kerningKey so it's more evident what's going on.

glyph.letter is only used here:

const uint64_t hash = GetKerningHash(lgl.letter, rgl.letter);

Anyways since that's not really relevant to the main issue the PR is trying to fix, I can put it back if you feel this is overextending.

@saurtron
Copy link
Collaborator Author

saurtron commented Nov 29, 2024

I guess that's what the current state of the PR does under the hood already, though without the spooky unordered_map::operator[].

Thanks for shedding some light into this :D

Maybe the issue with the [] operator is that it might not be a std::unordered_map like you say, then I don't know what'll happen XD. If it's an unordered_map, it should be fine, according to docs:

"If after the operation the new number of elements is greater than old max_load_factor() * bucket_count() a rehashing takes place.
If rehashing occurs (due to the insertion), all iterators are invalidated"

So (I believe) it should be totally safe to do glyphs[ch] = iter->second. since the iter won't get invalidated until after the insertion has been done. I guess you're right the problem here is the iterators are getting invalidated if done like the code is doing, but it's a bit weird it just crashes after a few times happening and all is fine before that.

What I like about glyphs[ch] = iter.second is how clean and understandable it looks.

Don't have a problem in changing to const auto [insertee, success] = glyphs.insert({ch, iter->second}); like you propose or some other method though.

@lhog
Copy link
Collaborator

lhog commented Nov 29, 2024

Yes, that old code was bad, but the new one looks a bit suspicious too.
I'd rewrite it as following just to be on the safe side:

auto glyphInfo = iter->second;
glyphs[ch] = glyphInfo;

@lhog
Copy link
Collaborator

lhog commented Nov 29, 2024

btw it might be a good idea to rewrite

spring::unordered_map<char32_t, GlyphInfo> glyphs;

into

spring::unordered_map<char32_t, size_t> glyphIdxMap;
std::vector<GlyphInfo> glyphsVec;

or even generalize this kind of construct into

template<K, V, H, C> class VectorMap {
<goodies go here>
private:
  spring::unordered_map<K, size_t, H, C> map;
  std::vector<V> vault;
};

@saurtron
Copy link
Collaborator Author

saurtron commented Nov 29, 2024

btw it might be a good idea to rewrite (...)

Yes was thinking the same. so it's easier to share glyphs among different chars. Anyways before big rewrites I think it's good to think about it more. Tbh the glyph sharing is pretty niche since usually with fontconfig you will be finding glyphs for every char.

For now anyways I think it's good to fix this with something simple, will keep that construct in mind for later.

@saurtron
Copy link
Collaborator Author

I'd rewrite it as following just to be on the safe side:

Done, imo it's not needed at least when using std::unordered_map but ok.

@lhog lhog merged commit 3647f78 into beyond-all-reason:master Dec 1, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants