Rockbox

  • Status Closed
  • Percent Complete
    100%
  • Task Type Bugs
  • Category User Interface → Font/charset
  • Assigned To No-one
  • Operating System All players
  • Severity Low
  • Priority Very Low
  • Reported Version Daily build (which?)
  • Due in Version Undecided
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by freddyb - 2011-09-29
Last edited by freddyb - 2011-10-17

FS#12299 - Font Glyph cache is no longer getting saved

edit: the later patches save a glyph file for each font instead of the original solution immediately below. i.e. /.rockbox/fonts/15-Times.fnt would have a glyphcache file /.rockbox/fonts/15-Times.glyphcache. -FB

The font system is no longer saving the .glyphcache file. This file data is used to preload the most recently used glyphs in disk order when loading a cached font to reduce the number of disk seeks. I’m posting this fix for review because it changes the old behavior a bit. Doing this right should improve font loading, especially for non-English users.

1) Saves the LRU data of the cached font with the most glyphs in memory. It did use FONT_UI for a long time but that might not be the best choice anymore as ID3 tags might be displayed by a skin font.
2) Only overwrites the existing file if it has more LRU data.
3) Doesn’t write the file for less than 20 entries.
4) Removes a kludge in the glyph cache reading code for an old bug that no longer exists.

Closed by  freddyb
2011-10-17 01:51
Reason for closing:  Fixed
Additional comments about closing:  

in r30760. Each font now gets its own glyphcache file.

I’m not sure about 1) and even less about 2).

1) might be ok, but why save only if it has “more LRU data”? The point isn’t to save the biggest, but the mos recent data.

Because LRU glyphs were still used glyphs and shrinking the .glyphcache file potentially means another random disk seek after loading for every entry removed. If you’re willing to put up with the extra writes you can change the test so that writes out if the new .glyphcache is the same size.

there really isnt a good way to do this, using the cache with the most items could just as likely cause 100% miss on a different font.
Two possible solutions:
1) use a compltly seperate LRU for glyhcache which just stores the glyph code and no data for the most recent X glyphs (lru is cheap so probbaly 1000) and load as many as the font can handle
2) store a seperate .glyphcache file for each font. This should be mostly trivial to do now, just get the font filename from the handle (core_get_name()) and rejig that to a file in /.rockbox/<filename>.glyphcache

I prefer 2 as it is more likely to cause cache hits, and the extra code to handle it shouldnt be much more than the above patch anyway.

Please do not do #1 as it might affect drawing performance. A full LRU of 1024 would add a binary search with up to ten guesses for every glyph drawn. I still think a glyph file per font is the least of bad options but I’m not going to work on this bug anymore because I just preload the US charset for my own builds.

err, what i meant was in addition to the individual font LRU’s there would be an additional one of 1024 items, that list is then saved to disk and on load a font will load as many as it can from that.

And I guess you didnt see the mailing list then? I might work on this but no promises, I’m not actively contributing anymore so a proper solution should be done by “someone” (-else). and yes, my option 2 is a very least-bad option.

The patch does not compile for iPod Video. After making it compile it crashes during startup.

OK, I lied :p
Attached is my implementation of 2. I’ve given it a quick test but without non-latin files its hard to do it properly. This saves/loads /.rockbox/<font name>.glyphcache so ti should corrreclty reflect individual font usages. I’m not completly sure that is the best place for them, possibly actually alongside the font, or differently named…

anyway, have a play and commit it if it works.

This fixes my issue with font reloading after track skip. But: Each shutdown of the iPod Video results in “Data abort at 0006248C (0)”. After each manual reset there is one additional “font.glyphcache” file (same name!) added to .rockbox, one of them is 1 KB large, the other are always 0 KB. I have attached a screenshot of this effect.

Edit: The data abort happens in glyph_cache_save().

hmm, ok wierd

If I comment the functionality within glyph_cache_save() it does – of course – not crash during the shutdown and does not create any glyphcache file. Interestingly the font-reload is *solved* then. Any idea?

font-reload works because you already have the ,glyphcache files there.. try this one which removes unnecessary (heck, outright wrong) code in internal_font_load(), also incorporates the fixed font locking fred worked on in a different task.

FWIW, using the test allocation from r30719, I can get a reproducible crash in an 320×480 sdl-app build.

Just tried with master+the above patch, no crash… gonna need a better repro

I didn’t apply the patch to repro the crash, then was with plain svn (if that reply was meant for me).

ok, so apply the patch and try again

font-reload works because you already have the ,glyphcache files there.. try this one which removes unnecessary (heck, outright wrong) code in internal_font_load(), also incorporates the fixed font locking fred worked on in a different task.

I am sure that I have deleted the glyphcache-files before my test. Nevertheless I will test your updated patch.

Just tried that patch with svn on my clipv1 and it boots fully so someone commit it! :)
edit: data abort on shutdown though…

IIRC font_shrinkfilename() is code duplication, we already have code doing this somewhere.

In glyph_cache_save(), the struct font* is used after yielding I/O (open()). It should be re-aquired afterwards. Same thing in glyph_cache_load() if I see this right.

glyph_cache_load is safe as the handle is locked before it is called, Ive just fixed _save() to have the same.
re font_shrinkfilename… pretty sure you’re tihnking about something in apps.

Could be that it’s in apps. Could implement it in firmware instead (filefuncs.c or something).

this fixes all the various font bugs

Does font_unload_all() unload fonts with multiple refcounts?

does now.

Fred, the locking in lcd-bitmap-common.c is not needed it appears.

Ok, I’m going to be away for a few days.

The latest patch again reloads fonts on each track change. It also does not save the glyphcache-file.

Edit: Sorry, I should have applied the patch before testing…

Here are the results after applying the patch.
1. Font-reloading olny happens with the first track skip after buffering has been finished. I can live with that.
2. With each shutdown of the device a new (empty) glyphcache file is created. See my posted image from above.

I think the glyphcache files should be saved to and read from FONT_DIR instead of ROCKBOX_DIR.

I again applied fix-glyphcache.3.diff, additionally commented the whole function glyph_cache_save() and started the iPod after removing all glypchcache files.

Result:
1) Even the first skip does not result in a disk spinup (maybe this is/was caused by the disk write access?). There is no font reloading or diskspin up occuring except when buffering is required.
2) Of course there is no glyphcache file created at all (as expected).

hopefuly we can be online on sunday at the same time to figure this out. cache_save() should be correct now :(

Resync to SVN.
Removed all locking code.
Fixed and inlined glyphcache file pathname generation.

Andree: glyph_cache_load() will preload the glyph codes 32 to 255 (up to capacity) if opening the glyph cache file fails, so you shouldn’t see a disk spin up for fonts unless you’re using glyphs beyond that range.

If you really want to inline the pathname code please put it in a static inline function. That sort of code duplication is prone to bugs later. Also, not using the full filename for the buflib alloc saves a *tiny* bit of ram and makes the buflib debug screen slightly nicer to use on small displays

glyphcachefix.4.diff shows no effect at all. I see font reloading with each skip and no glyphcache-files is created at all. (Yes, I have applied the patch this time ;)

Edit: Patched against r30740.

Created a separate function to create the glyphcache file path.
Did not shorten the buflib font handle name since that is a separate issue and might cause problems if a font is loaded from outside of the the font directory.
Changed powermgmt.c to call glyph_cache_save_all().
The UI simulator has no equivalent of shutdown_hw() where glyph_cache_save_all() would be called so the simulator will not save glyphcache files unless you change themes or fonts.

Andree: If you test this, would you do a “make clean; make reconf” first?

Same behaviour as with glyphcachefix.4.fix (reloading, no glyphcache files). Btw, I did “make clean” and “make reconf” (as with my test before).

Andree: I’m stuck. I’ve tried this on a Fuze v1, Fuze v2, fuze simulator, ipod, and ipod simulator and I can’t get anything close to the behavior you’re describing. Can you give me more details about what fonts and themes you’re using?

Perhaps upload config.cfg?

One thing I have just seen is that the glyphcache-files *were* existing in the fonts-path. I did not see any information about this path-change and were always searching in the .rockbox-path. After I have deleted them and restarted the iPod the font re-loading is solved.

But: Each on/off-cycle still adds a new glyphcache-file of the same name than the existing ones. I have attached two of the those files (the second is the type of file that is created with each on/off-cycle).

Another issue is that the first buffering of audio data (pressing play for the very first time after startup) will show HDD actitivity 2 times. First long HDD access is ~10-14s – this should be the audio data buffering. The the disk spins down and up again for a short time – font loading? This happens for each first buffering after restart.

I have also attached my current WPS incl. the fonts and the config.cfg.

Edit: Just in case this helps → I am using embedded coverart in each track.

Andree: Does changing glyph_cache_save_all() to font_unload_all() in powermgmt.c stop the extra files?

Fred, I’ve made this change and tested it → still multiple files with the same name are generated.

What solves the problem is when I add a small check to glyph_cache_save(). The code checks if the cache-file already exists (just via trying to open it), and does not re-create it in this case.

So, the question is why does “open(filename, O_WRONLY|O_CREAT|O_TRUNC, 0666);” create a new file of the same name if there is already one existing? I cannot even copy those mutliple files to my local HDD because they overwrite themselves…

Possibly file system corruption?

I saw it too, but I thought i fixed it on mine

Filesystem is and was fine. Could this effect depend on the build environment? I could compile versions for you which you could check for this effect. Which target builds would you need?

Andree: I was able to reproduce your problem with a good file system and hopefully, a good build environment. I haven’t figured out why yet.

Can this happen if the file is opened for read access when trying to open/create it for write access? Anyway, good to hear you can reproduce it.

Kugel: Are you sure that disk i/o cannot yield? It just doesn’t sound right.

Where did I say that? disk i/o of course yields.

Then I’d like to put some font locks in SVN to hopefully get enough stability to try to run rockbox on target long enough to figure out Andree’s multiple glyphcache file problem.

Andree: I hate to ask you to do this again … but could you svn up and try these together? If it works could you try again without the yield()’s? Thanks!

I updated svn to r30755, applied both patches and made a clean build. Font reloading does not happen, but there are still multiple files of the same name generated. This is happening with and without the yield()’s.

I’m pretty much grasping at straws at this point. Does turning off the directory cache have any effect?

Same :/

Ok, I put a sleep(HZ/10) in beginning of glyph cache save and it seems fix the problem for me.

Yes, this works around the issue for me as well.

Here it is. I’ll commit it tomorrow if no one objects.

Works for me. There is no font reloading happening, and there is only a single “*.gc” file created after several on/off cycles.

It would be nice if the real issue could be found. Is it perhaps concurrent access to the files?

I tested for concurrent access and it doesn’t seem to happen. Maybe parts of our disk i/o are not thread safe?

Loading...

Available keyboard shortcuts

Tasklist

Task Details

Task Editing