Rockbox

This is the bug/patch tracker for Rockbox. Click here for more information.

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

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

Attached to Project: Rockbox
Opened by Fred Bauer (freddyb) - Thursday, 29 September 2011, 04:47 GMT+2
Last edited by Fred Bauer (freddyb) - Monday, 17 October 2011, 03:51 GMT+2
Task Type Bugs
Category Font/charset
Status Closed
Assigned To No-one
Player Type All players
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Private No

Details

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.
   glyphcachefix.diff (5.7 KiB) 
 firmware/export/font.h |    2 -
 firmware/font.c        |   90 ++++++++++++++++++++++++++++++++++---------------
 2 files changed, 65 insertions(+), 27 deletions(-)

This task depends upon

Closed by  Fred Bauer (freddyb)
Monday, 17 October 2011, 03:51 GMT+2
Reason for closing:  Fixed
Additional comments about closing:  in r30760. Each font now gets its own glyphcache file.
Comment by Thomas Martitz (kugel.) - Thursday, 29 September 2011, 10:11 GMT+2
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.
Comment by Fred Bauer (freddyb) - Thursday, 29 September 2011, 15:27 GMT+2
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.
Comment by Jonathan Gordon (jdgordon) - Tuesday, 04 October 2011, 07:28 GMT+2
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.
Comment by Fred Bauer (freddyb) - Tuesday, 04 October 2011, 15:03 GMT+2
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.
Comment by Jonathan Gordon (jdgordon) - Tuesday, 04 October 2011, 15:17 GMT+2
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.
Comment by Andree Buschmann (Buschel) - Tuesday, 04 October 2011, 19:41 GMT+2
The patch does not compile for iPod Video. After making it compile it crashes during startup.
Comment by Jonathan Gordon (jdgordon) - Wednesday, 05 October 2011, 12:03 GMT+2
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.
   fix-glyphcache.diff (8.3 KiB) 
 b/apps/misc.c            |    1 
 b/apps/settings.c        |    4 +-
 b/firmware/export/font.h |    4 +-
 b/firmware/font.c        |   71 +++++++++++++++++++++++++++++++----------------
 4 files changed, 53 insertions(+), 27 deletions(-)

Comment by Andree Buschmann (Buschel) - Wednesday, 05 October 2011, 19:07 GMT+2
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().
   Image2.jpg (55.6 KiB)
Comment by Jonathan Gordon (jdgordon) - Thursday, 06 October 2011, 00:31 GMT+2
hmm, ok wierd
Comment by Andree Buschmann (Buschel) - Thursday, 06 October 2011, 08:18 GMT+2
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?
   fix-glyphcache_wo_cachesave.patch (7.9 KiB) 
 apps/settings.c        |    4 +-
 apps/misc.c            |    1 
 firmware/export/font.h |    4 +-
 firmware/font.c        |   80 ++++++++++++++++++++++++++++++++++---------------
 4 files changed, 62 insertions(+), 27 deletions(-)

Comment by Jonathan Gordon (jdgordon) - Thursday, 06 October 2011, 08:22 GMT+2
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.
   fix-glyphcache.1.diff (10.6 KiB) 
 b/apps/misc.c                          |    1 
 b/apps/settings.c                      |    4 -
 b/firmware/drivers/lcd-bitmap-common.c |    2 
 b/firmware/export/font.h               |    6 +-
 b/firmware/font.c                      |   91 ++++++++++++++++++++-------------
 5 files changed, 67 insertions(+), 37 deletions(-)

Comment by Thomas Martitz (kugel.) - Thursday, 06 October 2011, 08:45 GMT+2
FWIW, using the test allocation from r30719, I can get a reproducible crash in an 320x480 sdl-app build.
Comment by Jonathan Gordon (jdgordon) - Thursday, 06 October 2011, 09:00 GMT+2
Just tried with master+the above patch, no crash... gonna need a better repro
Comment by Thomas Martitz (kugel.) - Thursday, 06 October 2011, 09:03 GMT+2
I didn't apply the patch to repro the crash, then was with plain svn (if that reply was meant for me).
Comment by Jonathan Gordon (jdgordon) - Thursday, 06 October 2011, 09:04 GMT+2
ok, so apply the patch and try again
Comment by Andree Buschmann (Buschel) - Thursday, 06 October 2011, 09:09 GMT+2
> 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.
Comment by Jonathan Gordon (jdgordon) - Thursday, 06 October 2011, 11:51 GMT+2
Just tried that patch with svn on my clipv1 and it boots fully so someone commit it! :)
edit: data abort on shutdown though...
Comment by Thomas Martitz (kugel.) - Thursday, 06 October 2011, 12:02 GMT+2
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.
Comment by Jonathan Gordon (jdgordon) - Thursday, 06 October 2011, 12:38 GMT+2
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.
Comment by Thomas Martitz (kugel.) - Thursday, 06 October 2011, 13:14 GMT+2
Could be that it's in apps. Could implement it in firmware instead (filefuncs.c or something).
Comment by Jonathan Gordon (jdgordon) - Thursday, 06 October 2011, 14:16 GMT+2
this fixes all the various font bugs
   fix-glyphcache.2.diff (11.1 KiB) 
 b/apps/settings.c                      |    4 -
 b/firmware/drivers/lcd-bitmap-common.c |    2 
 b/firmware/export/font.h               |    5 +
 b/firmware/font.c                      |   98 ++++++++++++++++++++-------------
 b/firmware/powermgmt.c                 |    4 -
 5 files changed, 71 insertions(+), 42 deletions(-)

Comment by Fred Bauer (freddyb) - Thursday, 06 October 2011, 14:26 GMT+2
Does font_unload_all() unload fonts with multiple refcounts?
Comment by Jonathan Gordon (jdgordon) - Thursday, 06 October 2011, 14:33 GMT+2
does now.
   fix-glyphcache.3.diff (11.3 KiB) 
 b/apps/settings.c                      |    4 -
 b/firmware/drivers/lcd-bitmap-common.c |    2 
 b/firmware/export/font.h               |    5 +
 b/firmware/font.c                      |  102 +++++++++++++++++++++------------
 b/firmware/powermgmt.c                 |    4 -
 5 files changed, 75 insertions(+), 42 deletions(-)

Comment by Thomas Martitz (kugel.) - Thursday, 06 October 2011, 14:40 GMT+2
Fred, the locking in lcd-bitmap-common.c is not needed it appears.
Comment by Fred Bauer (freddyb) - Thursday, 06 October 2011, 15:38 GMT+2
Ok, I'm going to be away for a few days.
Comment by Andree Buschmann (Buschel) - Thursday, 06 October 2011, 19:40 GMT+2
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.
Comment by Fred Bauer (freddyb) - Thursday, 06 October 2011, 23:09 GMT+2
I think the glyphcache files should be saved to and read from FONT_DIR instead of ROCKBOX_DIR.
Comment by Andree Buschmann (Buschel) - Friday, 07 October 2011, 07:40 GMT+2
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).
Comment by Jonathan Gordon (jdgordon) - Friday, 07 October 2011, 07:51 GMT+2
hopefuly we can be online on sunday at the same time to figure this out. cache_save() should be correct now :(
Comment by Fred Bauer (freddyb) - Monday, 10 October 2011, 05:51 GMT+2
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.
   glyphcachefix.4.diff (7.9 KiB) 
 firmware/export/font.h |    3 -
 firmware/font.c        |  113 +++++++++++++++++++++++++++++--------------------
 firmware/powermgmt.c   |    2 
 3 files changed, 70 insertions(+), 48 deletions(-)

Comment by Jonathan Gordon (jdgordon) - Monday, 10 October 2011, 06:03 GMT+2
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
Comment by Andree Buschmann (Buschel) - Monday, 10 October 2011, 06:46 GMT+2
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.
Comment by Fred Bauer (freddyb) - Tuesday, 11 October 2011, 06:10 GMT+2
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?
   glyphcachefix.7.diff (8.2 KiB) 
 firmware/export/font.h |    4 +
 firmware/font.c        |  125 ++++++++++++++++++++++++++++++-------------------
 firmware/powermgmt.c   |    2 
 3 files changed, 81 insertions(+), 50 deletions(-)

Comment by Andree Buschmann (Buschel) - Tuesday, 11 October 2011, 07:50 GMT+2
Same behaviour as with glyphcachefix.4.fix (reloading, no glyphcache files). Btw, I did "make clean" and "make reconf" (as with my test before).
Comment by Fred Bauer (freddyb) - Tuesday, 11 October 2011, 14:46 GMT+2
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?
Comment by Thomas Martitz (kugel.) - Tuesday, 11 October 2011, 14:48 GMT+2
Perhaps upload config.cfg?
Comment by Andree Buschmann (Buschel) - Tuesday, 11 October 2011, 21:35 GMT+2
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.
   theme_video.7z (39 KiB)
   config.cfg (0.7 KiB)
   15-arialbd.glyphcache (0.4 KiB)
   15-arialbd.glyphcache (0.4 KiB)
Comment by Fred Bauer (freddyb) - Wednesday, 12 October 2011, 01:20 GMT+2
Andree: Does changing glyph_cache_save_all() to font_unload_all() in powermgmt.c stop the extra files?
Comment by Andree Buschmann (Buschel) - Wednesday, 12 October 2011, 08:02 GMT+2
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...
   glyphcachefix.8.patch (8.4 KiB) 
 firmware/export/font.h |    4 +
 firmware/font.c        |  134 +++++++++++++++++++++++++++++++------------------
 firmware/powermgmt.c   |    2 
 3 files changed, 90 insertions(+), 50 deletions(-)

Comment by Thomas Martitz (kugel.) - Wednesday, 12 October 2011, 08:35 GMT+2
Possibly file system corruption?
Comment by Jonathan Gordon (jdgordon) - Wednesday, 12 October 2011, 08:49 GMT+2
I saw it too, but I thought i fixed it on mine
Comment by Andree Buschmann (Buschel) - Thursday, 13 October 2011, 06:25 GMT+2
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?
Comment by Fred Bauer (freddyb) - Friday, 14 October 2011, 05:21 GMT+2
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.
Comment by Andree Buschmann (Buschel) - Friday, 14 October 2011, 07:46 GMT+2
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.
Comment by Fred Bauer (freddyb) - Friday, 14 October 2011, 17:11 GMT+2
Kugel: Are you sure that disk i/o cannot yield? It just doesn't sound right.
Comment by Thomas Martitz (kugel.) - Friday, 14 October 2011, 17:12 GMT+2
Where did I say that? disk i/o of course yields.
Comment by Fred Bauer (freddyb) - Friday, 14 October 2011, 17:20 GMT+2
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.
Comment by Fred Bauer (freddyb) - Saturday, 15 October 2011, 03:39 GMT+2
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!
   glyphcachefix.9.diff (9 KiB) 
 firmware/export/font.h |    4 +
 firmware/font.c        |  134 +++++++++++++++++++++++++++++++++----------------
 firmware/powermgmt.c   |    3 -
 3 files changed, 97 insertions(+), 44 deletions(-)

   lock_fonts.3.patch (2.6 KiB) 
 firmware/export/font.h               |    1 +
 firmware/font.c                      |   12 +++++++++++-
 firmware/drivers/lcd-bitmap-common.c |    4 ++++
 3 files changed, 16 insertions(+), 1 deletion(-)

Comment by Andree Buschmann (Buschel) - Saturday, 15 October 2011, 10:12 GMT+2
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.
Comment by Fred Bauer (freddyb) - Saturday, 15 October 2011, 15:30 GMT+2
I'm pretty much grasping at straws at this point. Does turning off the directory cache have any effect?
Comment by Andree Buschmann (Buschel) - Saturday, 15 October 2011, 17:35 GMT+2
Same :/
Comment by Fred Bauer (freddyb) - Saturday, 15 October 2011, 22:44 GMT+2
Ok, I put a sleep(HZ/10) in beginning of glyph cache save and it seems fix the problem for me.
Comment by Andree Buschmann (Buschel) - Saturday, 15 October 2011, 23:01 GMT+2
Yes, this works around the issue for me as well.
Comment by Fred Bauer (freddyb) - Sunday, 16 October 2011, 03:17 GMT+2
Here it is. I'll commit it tomorrow if no one objects.
   glyphcache.patch (8.8 KiB) 
 firmware/export/font.h |    2 
 firmware/font.c        |  155 +++++++++++++++++++++++++++++++++----------------
 firmware/powermgmt.c   |    2 
 3 files changed, 108 insertions(+), 51 deletions(-)

Comment by Andree Buschmann (Buschel) - Sunday, 16 October 2011, 14:48 GMT+2
Works for me. There is no font reloading happening, and there is only a single "*.gc" file created after several on/off cycles.
Comment by Thomas Martitz (kugel.) - Sunday, 16 October 2011, 17:46 GMT+2
It would be nice if the real issue could be found. Is it perhaps concurrent access to the files?
Comment by Fred Bauer (freddyb) - Sunday, 16 October 2011, 21:46 GMT+2
I tested for concurrent access and it doesn't seem to happen. Maybe parts of our disk i/o are not thread safe?

Loading...