Rockbox

This patch fixes  FS#12268  for me.

Why don't you do "handle = core_free(X)" in a single line?

because you insist on buflib_free() returning 0

todays work:
remove FONT_REMOTEUI completly and change FONT_UI to mean "any loaded ui font" (catchall for plugins mostly, needs to be fixed). FONT_SYSFIXED is now == -1 which causes other issues which also need to be dealt with but it is now used more like an error value.

the ui font id is now stored in global_status.font_id[screen] instead of the magic #define.

Still to be done: don't load the same font more than once, make sure all allocs are being freed correctly, test, fix plugins.

   fonts.1.diff (24.8 KiB)

* fix plugins
* add a plugin wrapper for setfont so FONT_UI and FONT_SYSFIXED still work there with a view that this will be removed *one day*
* remove all instances of lcd_setfont(0)

still need to:
* refcount fonts so we dont unload/load more than needed
* configurable cache size
* TEST!

   fonts.2.diff (37.3 KiB)

Testing fonts.2.diff on e200 device and sim (r30557)

e200 w32 sim
1. go to Settings → Theme Settings → Browse Theme Files
2. select cabbiev2
3. sim crashes

e200 Linux sim
1. go to Settings → Theme Settings → Browse Theme Files
2. select cabbiev2 multiple times
3. first select appears normal, second select outputs console message "font 2 not specified", third select outputs "font 3 not specified", etc.
4. buflib allocs shows font loaded multiple times - each time cabbiev2 is loaded, there is an additional font entry

e200 device
same as above - except no "font X not specified" messages

I am also seeing font corruption* when switching from cabbiev2 to a theme with sbs, or from that theme back to cabbiev2 - this can be reproduced on both the device and sim
1. reset settings
2. switch to to a theme with an sbs, such as SpartanBlack - http://themes.rockbox.org/download.php?themeid=507 3. font may be garbled, if not switch back to cabbiev2

*see attached screendump

I also saw backdrop corruption after copying files to the device and disconnecting USB, but have not been able to reproduce this.

   dump 110915-181622.bmp (75.7 KiB)

yeah thanks, my test target is e200-sim and I havnt seen what you're seeing (apart from the font corruption - which is awesome : ) - )
still plenty of work to be done, viewports need a reliable way of having their font id's changed :/

All of the above patches result in a forced shutdown when selecting a new theme (tested with sim only). After the next startup the newly selected theme is active. Is this wanted?

no :)

Buschel: can you try with reverting r3080?

3080?

grr, r30480

No, the same happens (what did I do in detail: Updating to r30567, rolling back r30480 only, applying font.0.diff).

arg, don't bother with the older version of the patch please.
Try this one which hopefully fixes Michaels issues. Hopefully the only crashes anyone sees is with playback running (which is a problem with r30480)

   fonts.3.diff (49 KiB)

Not that much better… The very first change of the theme is working w/o reboot. The next change will result in a reboot. Furthermore the theme list (under Settings/Theme Settings/Browse Theme Files) gets corrupted sometime after the first theme change – I cannot really find a rule how in details this happens. The results is that one theme name is changed (only the last characters or none remains).

this fixes Buschel's bug

edit: known issue, switching themes could cause lines to dissapear in th elist, I suspect fontcache is broken

   fonts.4.diff (49.2 KiB)

fonts.4.diff does fix most of the problems I reported above.

I still see font corruption, but it is now very intermittent. It seems to happen most often when switching themes after stopping playback.

Testing on the e200 sim, I am getting a segfault with the SpartanBlack theme when going into the fm screen. This theme does not use an fms but does use a custom ui. The gdb output is:

Program received signal SIGSEGV, Segmentation fault.
0x0808147a in skin_render (gwps=0x81ba990, refresh_mode=4294901760)

  at /home/mc2739/rockbox/apps/gui/skin_engine/skin_render.c:740

740 skin_viewport = (struct skin_viewport *)viewport→data;

I went back and checked with fonts.2.diff and also saw the same segfault. Current svn does not have a problem with this theme.

I've only seen intermittent font corruption as well. If it appears in viewport fonts of a new theme, re-selecting the theme seems to make it go away. However, if the system font is corrupted, only a reboot will fix it.

I've also seen a strange thing testing on the e200 sim (with the –checkwps flag turned on): When switching between cabbiev2 and skins with FM screens (i.e., any of the ipodosha or iPhosha skins), I get a "font x not specified" message (where x is 2, 3 or 4–no pattern, no relation to the fonts actually loaded) on cabbiev2.wps, and sometimes the FMS as well; any screen with that message doesn't work. If I switch back to a non-cabbiev2 WPS screen, whether as a WPS or a theme, everything's OK; but when I try cabbiev2 again, the problems return. (I may need to try another FMS-free theme such as Spartan Black; I have it on my real e200, but not the sim.)

Just loaded Spartan Black onto the e200 sim. The first time I switched from cabbiev2 to Spartan Black and tried the FM screen, I got this error:
X Error of failed request: BadWindow (invalid Window parameter)

Major opcode of failed request:  4 (X_DestroyWindow)
Resource id in failed request:  0x480000d
Serial number of failed request:  129
Current serial number in output stream:  129

The second time, I got a different error:
rockboxui: ../../src/xcb_io.c:221: poll_for_event: Assertion `1) ⇐ 0)' failed.
Aborted

The third time, I got a simplified form of Michael's message:
SDL_WaitEvent() error
Segmentation fault

I tried using Spartan Black with the FM screen set separately (i.e., the FMS from both ipodosha-v1.4 and iPhosha-v1.1); I got the second & third error messages again.

Finally, it seems only switching from ipodosha-v1.4 (or possibly other skins with FM screens) directly to cabbiev2 causes the "font x not specified" error. Switching between Spartan Black and the other skins is OK, even if you switch from ipodosha-v1.4 to Spartan Black and then immediately to cabbiev2.

To be more specific, my e200 sim was running in my build environment (the pre-built Ubuntu 11.04 image for VirtualBox found at http://www.rockbox.org/wiki/DevelopmentGuide ). My patch command also showed errors when it tried to delete skin_fonts.c & skin_fonts.h, but I didn't see them used at compile time.

JD, for some reason line 1849 in skin_parser.c causes a crash for me. The previous line's token allocation fails in case>TAG when specifying glyphs for a font in fms. Let me know what you want if you need more to go on.

OK, new rule, if you mention a theme and don't link to it I have to ignore it. I just grabbed http://themes.rockbox.org/index.php?themeid=507&target=sansae200 which is what you all seem t mention but it works fine.

Michael: are you seeing font corruption without changing themes/fonts?

I have only seen the font corruption when changing themes.

The fm screen problem fails only on the sim, but on both win & linux sims. I thought it might have been due to using my preset file, but after removing that, it still crashes. When it asks to scan for presets, after answering yes or no the sim will crash.

And, I did link the theme earlier in this task when I first mentioned it. :) http://www.rockbox.org/tracker/task/12273#comment40657

Michael, it was the skins I referenced that JD apparently couldn't find. The most recent versions of those, other than cabbiev2 (the default) and Spartan Black (already linked), are at http://themes.rockbox.org/index.php?themeid=1416&target=sansae200 (ipodosha-v1.4) and http://themes.rockbox.org/index.php?themeid=1333&target=sansae200 (iPhosha-v1.1). The e200 Linux sim actually seems stable with those skins; it's when you switch from them back to cabbiev2 that the font errors begin. Once that happens, the cabbiev2 WPS won't load until you restart the sim, but other WPS skins will still run. (The FMS of iPhosha-v1.1 won't work unless you load it under a different theme, but that's also true with current unpatched builds.)

I should note that the FMS skins of previous ipodosha versions 1.2 ( http://themes.rockbox.org/index.php?themeid=1369&target=sansae200 ) and 1.3 ( http://themes.rockbox.org/index.php?themeid=1407&target=sansae200 ), neither of which work in current builds, *do* work with this patch. Someone else wrote 1.2; I updated it to 1.3 merely by adding a 12-hour clock option. After discovering that the 1.3 FMS wouldn't run in current builds, I found in sim testing that the 35-point frequency display font wasn't loading; after trying the patch at  FS#12268  (which worked) and some discussion in IRC (which also pointed me to this FS), I updated it to 1.4 by reducing glyphs & fonts in all three skins, which un-broke the FMS in current unpatched builds. (The iPhosha skin was written by the same guy who wrote ipodosha-v1.2 & earlier. Earlier versions of both designs are also on the themes site; all of them are among the 12 newest themes listed for the e200.)

Though I've only tested this patch on the e200 (v1) Linux sim, I can confirm Michael's issues with Spartan Black there.

And I also haven't seen any font corruption other than AFTER changing skins. Once when font corruption appeared only in a viewport using a skin-defined, glyph-limited font (i.e., the clock in ipodosha-v1.4), reloading the theme made it go away. However, once font #1 is corrupted, the only solution is to restart the sim.

Actually, what I have seen regarding the corrupted font, if you load a theme that does not use the existing corrupted font, it is removed from memory. Reloading the theme which uses the font that was corrupted loads a fresh version of the font into memory which then looks normal.

\o/ I figure out the font corruption issue (I *hope*). The dissapearing line bug is still there but I don't care about it (well, cant figure it out). I'm getting "font 3 not specified" eventually but not crashes. please test

   fonts.5.diff (49.6 KiB)

and this one hopefully fixes the skin fonts issue

   fonts.6.diff (49.9 KiB)

Will this fix the "disappearing line"-issue? I will be able to test in a few hours – still at work now.

Not yet, I have no idea where to start with that one! I'm hoping I've got the other bugs though. If that is the last bug (and it only shows up when changing fonts) I'm going to commit it anyway.

I can do a build for you if you want? I'd kill for feedback in the next 2 hours while I'm awake and working on this :)

OK, if you get the dissapearing line, turn off dircache and try again. I suspect a dircache bug here :)

The fm screen crash is still there.

There is a build problem due to a printf in apps/filetree.c line 431 (it was there in fonts.4.diff also)

Regarding the missing line, it is not just missing text, when it occurs on the browse themes list, if you press select on the blank line it does not load the theme that should be listed there, it reloads the current theme.

Blank line still occurs with dircache off.

blank lines was a bug in svn, fixed in r20577, im going to commit this tomorrow hopefully

There is a problem if buflib_compact is called while loading a skin. It causes the font pointers to miss. How does the font engine know when fonts are moved in memory?

font handles cant move while that font is accessing the disk (being loaded or getting glyph from disk), The only danger is if someone calls font_get() then does disk access, then uses the font * directly, but that shouldnt happen ever (I tihnk I made sure of that in svn)

One little point - sim build fails for me unless I include settings.h in skin_render.c (for global_settings). Will do some testing shortly…

I mean after loading and the font is in use. Say you load a font at 0x10000 and the glyph_cache table entries are 0x10100,0x10110, 0x10120… The font then gets relocated to 0x20000 and the glyph_cache table entries are now at 0x20100, 0x20210,… Where do all the font pointers get updated to reflect the move? Sorry if I'm being obtuse.

Compiles for sim (nano 2g, win32 cc under Linux) and works as expected for my usecase.

OK, changing themes twice during playback segfaults the sim (Windows, Cygwin) reliably for me. I'll dig a bit deeper but here's a backtrace in the meantime:
Program received signal SIGSEGV, Segmentation fault.
0x00485cac in buflib_get_name (ctx=0x6bdc04, handle=17) at /home/Steve/rockbox/firmware/buflib.c:701
701 size_t len = data[-1].val;
(gdb) bt
#0 0x00485cac in buflib_get_name (ctx=0x6bdc04, handle=17) at /home/Steve/rockbox/firmware/buflib.c:701
#1 0x00484fc2 in handle_to_block (ctx=0x6bdc04, handle=17) at /home/Steve/rockbox/firmware/buflib.c:164
#2 0x0048595a in buflib_free (ctx=0x6bdc04, handle_num=17) at /home/Steve/rockbox/firmware/buflib.c:554
#3 0x0047ab08 in core_free (handle=17) at /home/Steve/rockbox/firmware/core_alloc.c:35
#4 0x00435eeb in skin_data_free_buflib_allocs (wps_data=0x585670)

  at /home/Steve/rockbox/apps/gui/skin_engine/skin_parser.c:1452

#5 0x004340eb in settings_apply_skins () at /home/Steve/rockbox/apps/gui/skin_engine/skin_engine.c:113
#6 0x0041c62e in settings_load_config (file=0x22f8c0 "/.rockbox/themes/Pen&Paper.cfg", apply=true)

  at /home/Steve/rockbox/apps/settings.c:366

#7 0x00426f96 in ft_enter (c=0x543148) at /home/Steve/rockbox/apps/filetree.c:573
#8 0x0042172e in dirbrowse () at /home/Steve/rockbox/apps/tree.c:690
#9 0x00421ee2 in rockbox_browse (browse=0x22fbe0) at /home/Steve/rockbox/apps/tree.c:995
#10 0x004072b5 in browse_folder (param=0x48d1d4) at /home/Steve/rockbox/apps/menus/theme_menu.c:327
#11 0x00406752 in do_menu (start_menu=0x4915e8, start_selected=0x0, parent=0x0, hide_theme=false)

  at /home/Steve/rockbox/apps/menu.c:579

#12 0x0041a244 in miscscrn (param=0x4915e8) at /home/Steve/rockbox/apps/root_menu.c:329
#13 0x0041a80f in load_screen (screen=3) at /home/Steve/rockbox/apps/root_menu.c:561
#14 0x0041a75a in root_menu () at /home/Steve/rockbox/apps/root_menu.c:732
#15 0x004053ec in main (argc=3, argv=0x3e4ea0) at /home/Steve/rockbox/apps/main.c:196
(gdb) print data
$1 = (union buflib_data *) 0x0

Minor note: The glyph cache file is no longer saved. pf→fd is always -1 when called. Maybe the call to glyph_cache_save() should be moved to font_unload() because font unloading was implicit in font_load() before and now it's not.

Fred: all the relevant pointers are moved together in font.c buflibmove_callback().
Steve: can you give a full repro recipe? which sim? whic themes? etc?

Nice work and it may solute my problem, but failed to compile for OndaVX747.
The source is from r30401, and apply the patch sucessfully.
[…]
/home/yzflcyq/PMP-Firmware-s-Rockbox-X/apps/filetree.c: In function ‘ft_load_font’:
/home/yzflcyq/PMP-Firmware-s-Rockbox-X/apps/filetree.c:432: warning: implicit declaration of function ‘printf’
/home/yzflcyq/PMP-Firmware-s-Rockbox-X/apps/recorder/albumart.c: In function ‘search_albumart_files’:
/home/yzflcyq/PMP-Firmware-s-Rockbox-X/apps/recorder/albumart.c:193: warning: implicit declaration of function ‘snprintf’
/home/yzflcyq/PMP-Firmware-s-Rockbox-X/test/apps/filetree.o: In function `ft_enter':
(.text.ft_enter+0x484): undefined reference to `printf'
/home/yzflcyq/PMP-Firmware-s-Rockbox-X/test/apps/filetree.o: In function `ft_enter':
(.text.ft_enter+0x484): relocation truncated to fit: R_MIPS_26 against `printf'
collect2: ld returned 1 exit status
make: *** [/home/yzflcyq/PMP-Firmware-s-Rockbox-X/test/rockbox.elf] 错误 1

Edit (ABuschmann): I just removed unecessary compile output.

You will just need to comment the "printf()" and recompile. This has been pointed out in of the former comments here.

Hi JD,

I'm running an H300 sim (r30579 + fonts.6.diff) under Windows, compiled under Cygwin. Recipe is simple:
1) Start playback
2) Change theme (e.g. from Cabbiev2 to Pen&Paper). Sometimes Kaboom!
3) Change theme back (e.g. from Pen&Paper to Cabbiev2). Always Kaboom!

It happens with all themes I've tried so far - I suspect they all change font though. I do have voice menus and dircache enabled - my config.cfg is attached.

   config.cfg (0.5 KiB)

can you link me the themes? just to make sure i get the right one.

Here's the files I'm using (apologies if I'm breaking licences here…)

   themes.7z (178.3 KiB)

Woo - I don't even need to start playback… just switching themes is enough.
I added some printfs to log all changes to alloc… looks like we're trying to free something (handle 17) twice…

Entering settings_apply_skins
freeing skin 0, screen 0
Entering skin_data_free_buflib_allocs
Leaving skin_data_free_buflib_allocs
freeing skin 0, screen 1
Entering skin_data_free_buflib_allocs
Entering buflib_free handle_num=17
Entering buflib_free handle_num=18
Entering buflib_free handle_num=19
Entering buflib_free handle_num=20
Entering buflib_free handle_num=21
Entering buflib_free handle_num=22
Entering buflib_free handle_num=23
Entering buflib_free handle_num=24
Entering buflib_free handle_num=25
Entering buflib_free handle_num=26
Entering buflib_free handle_num=27
Entering buflib_free handle_num=28
Entering buflib_free handle_num=29
Entering buflib_free handle_num=30
Entering buflib_free handle_num=31
Entering buflib_free handle_num=32
Entering buflib_free handle_num=33
Entering buflib_free handle_num=34
Entering buflib_free handle_num=35
Entering buflib_free handle_num=36
Entering buflib_free handle_num=37
Entering buflib_free handle_num=38
Entering buflib_free handle_num=39
Entering buflib_free handle_num=40
Entering buflib_free handle_num=41
Leaving skin_data_free_buflib_allocs
freeing skin 1, screen 0
Entering skin_data_free_buflib_allocs
Leaving skin_data_free_buflib_allocs
freeing skin 1, screen 1
Entering skin_data_free_buflib_allocs
Entering buflib_free handle_num=42
Entering buflib_free handle_num=43
Entering buflib_free handle_num=44
Entering buflib_free handle_num=45
Entering buflib_free handle_num=46
Entering buflib_free handle_num=47
Entering buflib_free handle_num=48
Entering buflib_free handle_num=49
Entering buflib_free handle_num=50
Leaving skin_data_free_buflib_allocs
freeing skin 2, screen 0
Entering skin_data_free_buflib_allocs
Entering buflib_free handle_num=17

Program received signal SIGSEGV, Segmentation fault.
0x00485d9c in buflib_get_name (ctx=0x6bdc04, handle=17) at /home/Steve/rockbox/firmware/buflib.c:706
706 size_t len = data[-1].val;

Hope this is useful.

(Edit - updated trace for clarity.)

FWIW, I can confirm that the crash goes away if I make buflib_free do nothing if the handle was already freed - this involves buflib_get_name and buflib_get_data being able to return NULL though, so a new can of worms.

fix Steves crashes…. Still no idea what Michael's bug is about :(

   fonts.7.diff (50.2 KiB)

Pondlife & JD: The Pen&Paper theme for H300 is also on the themes site at http://themes.rockbox.org/index.php?themeid=900&target=iriverh300 .

I've applied the newest patch to r30579 and loaded it to both my e200 Linux sim and my live e280; the latter required a commented-out printf() in filetree.c.to compile (as Buschel suggested). So far, the biggest issue seems to be multiple theme changes during a session, especially during playback. Several times when the final theme change on the live target was to ipodosha-v1.4 (previously linked), the scroll wheel became unusuably slow until after a reboot. The live target also crashed once when the final switch was to SpartanBlack (also previously linked), but I was in bed and don't recall the exact steps or the error message; I wasn't able to reproduce it.

Since ipodosha-v1.4 uses a larger default font (15 points vs. 12 for cabbiev2 and 11 for SpartanBlack), it's very much possible that larger default fonts contribute to this issue. It may also be related to use of an SBS skin with viewports; cabbiev2 doesn't have an SBS, but both ipodosha-v1.4 and SpartanBlack do (though the latter seems to have less problems).

I'm also still getting the crash when opening the FM radio (with or without a skin) on SpartanBlack, but *only* in sim–*not* on the live e200. I'll have to recompile the sim without the patch to verify; but since it's in sim only with one theme only, I suspect it may be unrelated to this patch. (The sim also crashed once when entering FM with the cabbiev2 theme and ipodosha-v1.4's FMS–neither cabbiev2 nor SpartanBlack explicitly clears the FMS when changing; but I couldn't reproduce it.) The other problems I noted with patch 4 in the sim (font corruption, inappropriate "font x not specified" messages) are gone.

One other oddity: Sometimes when changing the theme (especially to ipodosha-v1.4) during playback, after interruption the sim restarted the track at the beginning. (I haven't done enough testing during playback with the live target to confirm if it happens there also.) However, like the multiple-change issue, that most likely involves default and/or SBS fonts; playback was only briefly interrupted as expected, not restarted, when changing FMS skins (even to ipodosha-v1.4's FMS with 35-point frequency-display font, albeit glyph-limited).

Clarify: My previous comment was with patch 6; I posted it before discovering JD had uploaded patch 7.

Confirm that v7 fixes my problem.

One tiny thing I noticed (although not in your patch…) - I don't think buflib.c needs to do handle→alloc = NULL after calling handle_free().

After several missteps (note to self: Delete make.dep *every* time a file is deleted), have updated to r30581 and applied patch 7. These are the only two issues I still see in the e200 Linux sim:

(a) Michael's crash when launching the FM radio with SpartanBlack still exists. (I confirmed that it does *not* exist in unpatched SVN.) However, it didn't happen on my live e200 target with r30579 + patch 6 (I haven't updated it yet), so IMO it's insignificant.

(b) The *first* time I load the ipodosha-v1.4 theme during playback, the track restarts at the beginning. It does *not* happen with cabbiev2 or SpartanBlack, nor on second or subsequent loads of ipodosha-v1.4 while playing the same track. From reviewing –debugwps messages, it appears the first simultaneous change of all three skins causes the sim to forget the track's resume point; of the three themes I'm testing, only ipodosha-v1.4 has an FMS skin. (It doesn't happen when manually changing any skin.) I haven't compiled the update for the live target yet; I'll check that later.

The live target compiled (no more printf() crash). However, I still get this compiler warning, which AFAIK has appeared on every patch I've tested (sim or live), but *not* when I compiled unpatched r30581 for sim:

/home/ubuntu/rockbox/apps/recorder/albumart.c: In function ‘search_albumart_files’:
/home/ubuntu/rockbox/apps/recorder/albumart.c:193: warning: implicit declaration of function ‘snprintf’

Have loaded the patch to my live e280. I noted these issues:

(a) As with the sim, switching to ipodosha-v1.4 during playback results in the track restarting. However, the behavior is slightly different on live target: It first pauses and continues playing, then pauses again and restarts; I believe it paused slightly again before continuing. That seems to confirm the playback reset is caused by too-quick loading of either the FMS or (most likely) WPS skin.

(b) A multiple theme switch in the sequence ipodosha-v1.4 → cabbiev2 → SpartanBlack normally (but not always) causes a crash: "Data abort at 000FDEC (0)". After hard power-off and reboot, the theme is still cabbiev2.

© There are several circumstances in which initially loading ipodosha-v1.4 may cause the scroll wheel to become unresponsive. Without playback, it has been seen several times (but not always) if SpartanBlack has been previously loaded. During playback, however, I've seen it (along with font corruption, and sometimes power-off being unresponsive except for hard power-off) in other circumstances, such as when switching from it to cabbiev2 and back again. However, everything is OK after a reboot.

As before, Michael's FM issue with SpartanBlack on the sim does *not* occur on the live target.

Since the theme links were posted much, much earlier in the thread, I'll post them again:
ipodosha-v1.4: http://themes.rockbox.org/index.php?themeid=1416&target=sansae200 SpartanBlack: http://themes.rockbox.org/index.php?themeid=507&target=sansae200

fonts.7diff works for me as well.

I think you might need something like this added to buflibmove_callback. I think the font_cache._index pointers are absolute. If you can figure how to reliably get a cached font moved after loading you can see the effect.
for( i = 0; i < alloc→font.cache._capacity ; i++ ) alloc→font.cache._index[i] += diff;

Richard: the stopping on theme change issue is irrleveant to this patch (it is caused by buflib, there is a open bug for this)

Fred: You 100% sure? that _index is an integer not a pointer. though it does look like font_cache_get() needs to lock the font handle as it does disk access

JD: What's the FS number of that bug?

AFAIK the short interruption when changing themes is unavoidable as buflib may (if not "will") have to reallocate the audio buffer. The bug I was referring to was restarting *after* interruption at the *beginning* at the track, instead of where it was interrupted.

Correction: *of* the track.

 FS#12279 

That's basically my problem, though for me it does *NOT* occur with cabbiev2. However, that FS is specifically for the Clip+ (AMSv2); no other player is mentioned there, not even other AMSv2 Sansas. My Sansa is an e280v1 (PP); its only connections to the Clip+ are same manufacturer and same basic processor type (ARM). The FS would be more relevant if it included other PP-based players (Sansa or non-Sansa), not just a single AMS Sansa. But that doesn't necessarily rule it out as the same issue; it may just be more obvious on the Clip+ due to its smaller screen. Is it really the same bug for e200v1 as for Clip+?

it's the same bug

Crap, I read it wrong. It's a pointer to an array of shorts. font.cache._index might need adjusted, tho.

you are indeed correct.
cool! github lets you edit+commit changes from the website :) I wont be doing an update till sat night (sunday more likely) but pull from github or just add "pf→font.cache._index += diff;" to buflibmove_callback().
https://github.com/jdgordon/rockbox

edit: also pushed an untested change to lock the handle when doing a cache lookup from disk

Your untested change (I downloaded its font.c from GitHub, then diffed it against r30582 and applied it as a patch) crashed my e200v1 Linux sim compile:

CC firmware/font.c
/home/ubuntu/rockbox/firmware/font.c: In function ‘buflibmove_callback’:
/home/ubuntu/rockbox/firmware/font.c:105:5: error: ‘pf’ undeclared (first use in this function)
/home/ubuntu/rockbox/firmware/font.c:105:5: note: each undeclared identifier is reported only once for each function it appears in
/home/ubuntu/rockbox/firmware/font.c: In function ‘load_cache_entry’:
/home/ubuntu/rockbox/firmware/font.c:565:18: warning: initialization makes integer from pointer without a cast
/home/ubuntu/rockbox/firmware/font.c:567:29: error: ‘struct buflib_alloc_data’ has no member named ‘pf’
/home/ubuntu/rockbox/firmware/font.c: In function ‘pf_to_handle’:
/home/ubuntu/rockbox/firmware/font.c:639:5: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘attribute’ before ‘for’
/home/ubuntu/rockbox/firmware/font.c:639:10: error: ‘i’ undeclared (first use in this function)
/home/ubuntu/rockbox/firmware/font.c:645:29: error: ‘struct buflib_alloc_data’ has no member named ‘pf’
/home/ubuntu/rockbox/firmware/font.c: In function ‘font_get_bits’:
/home/ubuntu/rockbox/firmware/font.c:667:13: warning: passing argument 4 of ‘font_cache_get’ makes pointer from integer without a cast
/home/ubuntu/rockbox/firmware/include/font_cache.h:47:26: note: expected ‘void *’ but argument is of type ‘int’
make: *** [/home/ubuntu/rockbox/build-sim/firmware/font.o] Error 1

Will attempt to apply the simple update and see what happens there.

I can't get any more rendering weirdness now. Don't forget about saving the .glyphcache file. This might be for a another day, but what would you think about saving the .glyphcache file for each font? ( i.e. ./rockbox/fonts/glyphcache/<fontname>.cache )

Richard, change your line to use alloc→ instead of pf→ like this:

  alloc->font.cache._index += diff;

Same error even on simple one-line patch:

CC firmware/font.c
/home/ubuntu/rockbox/firmware/font.c: In function ‘buflibmove_callback’:
/home/ubuntu/rockbox/firmware/font.c:105:5: error: ‘pf’ undeclared (first use in this function)
/home/ubuntu/rockbox/firmware/font.c:105:5: note: each undeclared identifier is reported only once for each function it appears in
/home/ubuntu/rockbox/firmware/font.c: In function ‘load_cache_entry’:
/home/ubuntu/rockbox/firmware/font.c:565:18: warning: initialization makes integer from pointer without a cast
/home/ubuntu/rockbox/firmware/font.c:567:29: error: ‘struct buflib_alloc_data’ has no member named ‘pf’
/home/ubuntu/rockbox/firmware/font.c: In function ‘pf_to_handle’:
/home/ubuntu/rockbox/firmware/font.c:639:5: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘attribute’ before ‘for’
/home/ubuntu/rockbox/firmware/font.c:639:10: error: ‘i’ undeclared (first use in this function)
/home/ubuntu/rockbox/firmware/font.c:645:29: error: ‘struct buflib_alloc_data’ has no member named ‘pf’
/home/ubuntu/rockbox/firmware/font.c: In function ‘font_get_bits’:
/home/ubuntu/rockbox/firmware/font.c:667:13: warning: passing argument 4 of ‘font_cache_get’ makes pointer from integer without a cast
/home/ubuntu/rockbox/firmware/include/font_cache.h:47:26: note: expected ‘void *’ but argument is of type ‘int’
make: *** [/home/ubuntu/rockbox/build-sim/firmware/font.o] Error 1

Now I'll have to see if the unpatched font.c actually works…

You probably didn't save it. It's still trying to compile pf→…

Richard: fixed all but the last warning which is ignorable for now.
Fred: yep, I dont really tihnk a cache for each font is a good idea, the glyphs indexes are not font specific (they are the char codes), Though sure depending on the theme different fonts would use different chars more often, I tinhk for now just using the one .glyphcache like svn does is the way to go, then later we can revist this.

Freddy: I was posting my second fail just as you posted the alloc→ solution. Getting ready to download JD's latest revision…

My compile warnings on e200v1 Linux sim using JD's latest patch (no crashes, unless you count the one when I tried using Ctrl-C to copy–old Windows habits die hard :D ; thankfully, make simply started from where it left off):

(a) Been getting this one (and ignoring it) all along:
CC apps/recorder/albumart.c
/home/ubuntu/rockbox/apps/recorder/albumart.c: In function ‘search_albumart_files’:
/home/ubuntu/rockbox/apps/recorder/albumart.c:193:13: warning: implicit declaration of function ‘snprintf’

(b) Font.c warnings (JD, I assume these are your "ignorable" ones):
CC firmware/font.c
/home/ubuntu/rockbox/firmware/font.c: In function ‘load_cache_entry’:
/home/ubuntu/rockbox/firmware/font.c:565:18: warning: initialization makes integer from pointer without a cast
/home/ubuntu/rockbox/firmware/font.c: In function ‘font_get_bits’:
/home/ubuntu/rockbox/firmware/font.c:667:13: warning: passing argument 4 of ‘font_cache_get’ makes pointer from integer without a cast
/home/ubuntu/rockbox/firmware/include/font_cache.h:47:26: note: expected ‘void *’ but argument is of type ‘int’

However, rockboxui crashes on launch (with or without –debugwps):
SDL_WaitEvent() error
Segmentation fault

This time I made my own "simple" patch: I manually added the alloc→ line to existing font.c, saved the result to another file, then formally applied it via diff & patch. Compiled & ran in e200 sim just fine; apparently works fine except for existing issues (FM-radio crash in SpartanBlack, sim-only issue; restarting tracks when loading ipodosha-v1.4, not a patch issue).

Think I'll retry the other (just in case my crash was due to the make interruption) before testing on live target.

Tried downloading JD's last patch again; same file, same compiler warnings, same crash. Definitely wasn't caused by a Ctrl-C on make this time. :(

Will revert to my "simple" patch before testing on live target.

After compiling the "simple" patch (i.e., simply adding the alloc→ line where needed) to my live e280v1 target, it seems more stable than before. The biggest issue is an occasional crash ("Data abort at 0005FDF8 (0)") in some cases when switching from ipodosha-v1.4 to cabbiev2; I also see a slow scroll wheel and/or font corruption after making several (i.e., 3 or more) theme switches in a row. However, if it's just a slow scroll wheel, it goes away if you can get to the top and select cabbiev2. (Of course, rebooting makes both scroll-wheel and font-corruption issues go away.)

Most (but not all) of the time–contrary to JD's previous statement (though fixing his bug might help)–playback no longer reverts to the track's beginning when loading ipodosha-v1.4 while playing, though the theme-change process is still slow while playing MP3s. (FM radio playback doesn't appear to affect theme changing at all.) The player doesn't even pause when switching to SpartanBlack, or when switching skins within themes.

All in all, looks like we're almost there. If JD can fix the remaining issues with his full patch, it might be ready to go.

If you didnt apply the locking part of the work I pushed to github then there is no point testing further. that data abort is probably due to it not locking. check the rockbox.map file, search for 0005fd (maybe drop the d) and figure out which function that address is in

I compiled your latest github patch for the e200 Linux sim; it crashed on launch every time. (It also produced compiler warnings on font.c, as posted earlier–*not* the same ones as when pf→ was used.) Maybe I'll have better luck trying that patch with the live target tomorrow, but for now I need to go to bed.

The line you were looking for in rockbox.map is as follows:
.text 0x00005fdc 0x2c4 /home/ubuntu/rockbox/build/apps/menus/theme_menu.o

Just to be sure there wasn't some other problem, I used the sim of my "simple" patch to change the theme back to cabbiev2, then recompiled JD's latest patch; same compiler warnings, same segfault crash when opening the sim. Since JD apparently missed it, here's the compiler warnings on his patch again:

CC firmware/font.c
/home/ubuntu/rockbox/firmware/font.c: In function ‘load_cache_entry’:
/home/ubuntu/rockbox/firmware/font.c:565:18: warning: initialization makes integer from pointer without a cast
/home/ubuntu/rockbox/firmware/font.c: In function ‘font_get_bits’:
/home/ubuntu/rockbox/firmware/font.c:667:13: warning: passing argument 4 of ‘font_cache_get’ makes pointer from integer without a cast
/home/ubuntu/rockbox/firmware/include/font_cache.h:47:26: note: expected ‘void *’ but argument is of type ‘int’

The only difference in warnings between sim and live compiles was it omitted the second number in each file reference (i.e., font.c:565 instead of font.c:565:18); that's probably just different gcc versions. I'm still working on USB-related issues before I can load it to the live target.

JD: Finally got the full patch loaded to live target. It boots, but with cabbiev2 the clock font is corrupted; with ipodosha-v1.4 the menu font is corrupted. Your patch still needs some work; it crashes the e200v1 Linux sim and causes font corruption on a live e200v1.

JD: From analyzing the compiler warnings and the code (I haven't written in C but I did write in Cobol & Fortran back in college), it appears the big issue is in line 565:

  int handle = &buflib_allocations[(int)callback_data];

It seems you're declaring handle as an integer here, but other parts of the code are expecting something else.

The other two warnings are apparently due to handle being passed to font_cache_get() as an integer; that could very well have caused the segfault crash (sim) or font corruption (live target).

JD, in font.c:font_get_bits() the fuction bugs out if handle < 0 but I think it's valid for SYSFONT. NULL is then passed into lcd_whatever() as a pointer to the glyph's bits causing a seg fault.

fonts.7.diff can be compiled sucessfully without error but albumart.c has a warning:
CC apps/recorder/albumart.c
/home/yzflcyq/rockbox/apps/recorder/albumart.c: In function ‘search_albumart_files’:
/home/yzflcyq/rockbox/apps/recorder/albumart.c:193:13: warning: implicit declaration of function ‘snprintf’

I added #include <stdio.h> to the start of albumart.c and the warning goes.
I forgot to bring my DAP back to home :( I'll test this great job next week.

Purling: Your patch removed the compile warning on e200v1 (both live and Linux sim) and ran fine on the sim. Still waiting for the live compile to finish, but I doubt there'll be any issues with it.

JD: After reviewing the compiler warnings on your latest github revision to font.c (and Wikipedia on C variable types & declarations), I made a few changes to deal with the warnings, as well as comment out the NULL return Freddy complained about; it's attached here as a patch to fonts.7.diff's font.c. Along with Purling's patch to albumart.c (above), it compiled with no warnings for e200v1 Linux sim, and if the theme is cabbiev2 or SpartanBlack it apparently runs fine (though playback restarts when changing, which didn't happen before with those two themes). However, if you switch to ipodosha-v1.4, it segfaults till you manually revert /.rockbox/config.cfg to something resembling cabbiev2. Perhaps you could take a look and fix it? (Be easy on me; I'm new to C, though not to programming.)

   font.c.patch6 (2.4 KiB)