Rockbox

Tasklist

FS#11470 - Use the new skin parser in the rockbox build also BIG display code cleanup

Attached to Project: Rockbox
Opened by Jonathan Gordon (jdgordon) - Sunday, 11 July 2010, 06:46 GMT
Last edited by Jonathan Gordon (jdgordon) - Thursday, 29 July 2010, 12:38 GMT
Task Type Patches
Category Themes
Status Closed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Release 3.6
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

Lots of cleanup still to do but this should display skins correctly.

stuff I know doesnt work in this patch:
- peakmeters
- %Vf(), %Vb() only work for the whole viewport, not the line it is on
- every tag gets updated always which means scrolling no worky either
- %ax maybe..
- LCD_CHARCELL
- conditional viewports

This task depends upon

Closed by  Jonathan Gordon (jdgordon)
Thursday, 29 July 2010, 12:38 GMT
Reason for closing:  Accepted
Additional comments about closing:  in r27613
Comment by Jonathan Gordon (jdgordon) - Sunday, 11 July 2010, 07:36 GMT
this one should make conditional viewports work

sublines are busted... dont try to use them :)
Comment by Jonathan Gordon (jdgordon) - Sunday, 11 July 2010, 14:08 GMT
sublines should work in this one... the timeouts are not correct but they do change which is the main thing.
Comment by Jonathan Gordon (jdgordon) - Sunday, 11 July 2010, 14:28 GMT
subline timeouts work correctly
Comment by Jonathan Gordon (jdgordon) - Monday, 12 July 2010, 11:21 GMT
sync with svn...
Comment by Jonathan Gordon (jdgordon) - Monday, 12 July 2010, 12:52 GMT
I'm not expecting this to work fully.. Does the progressbar work with any of the above patches? its stopped working in this one...

If you see anything crazy with this patch please try newparser.2.diff and let me know if it didnt happen in that version.

edit: FYI and non conseqential... on a ipod4g build this adds about 3.6KB which should come down alot (it is already down from 7KB)
(add/remove: 39/14 grow/shrink: 13/21 up/down: 13303/-9660)
Total: 3643 bytes
Comment by Michael Chicoine (mc2739) - Monday, 12 July 2010, 13:45 GMT
Results of testing newparser.2.diff on e200v2 with r27398

1. Switching between different themes will sometimes crash rockbox (device and sim). When it doesn't crash, there is visible theme corruption (see screendump - backdrop "Rockbox" and misaligned icons).
2. Progress bar does not work
3. Using default cabbiev2 theme, text is displayed in wrong color (should be CCCCCC per .cfg file) - looks like 000000 (see screendump)
4. Using customized widecabble theme, font load in .fms file corrupts filename with data from %pb line

Line 4 = %Fl(2,33-Digital.fnt)
Line 48 = %pb(5,1,166,8,pb-176x220x16.bmp)

sim console error:
Can't open font: /.rockbox/fonts/1,166,8,pb-176x220x16.fnt
Unable to load font 2: '1,166,8,pb-176x220x16.fnt'

5. Using customized widecabble theme, viewport definition in .wps file is not calculating "-" properly

Line 77 = %Vl(f,44,162,-,19,-)

sim console error:
ERROR: set_viewport out of bounds: x: 44 y: 162 width: 176 height:19

Line 80 = %Vl(g,0,162,-,-,-)

sim console error:
ERROR: set_viewport out of bounds: x: 0 y: 162 width: 176 height:194

changing to Line 77 to %Vl(f,44,162,132,19,-) and Line 80 to %Vl(g,0,162,-,58,-) corrects the error
Comment by Jonathan Gordon (jdgordon) - Monday, 12 July 2010, 13:55 GMT
thanks!

1) havnt seen it, I'll look out for it
2) yeah, trying to figure that out
3) fixed
4) hehe, OK, I know what causes that, fixed
5) thanks, fixed

The attached patch fixes those bugs and probably adds a few more :p
Comment by Michael Chicoine (mc2739) - Monday, 12 July 2010, 16:28 GMT
3, 4, 5 above are working properly now - Thanks

New issues:

Using default cabbiev2 theme,
6. Hold icon is missing and battery icon is cut off on bottom - see screen-1.bmp
7. Progress bar works in fm radio using inbuilt theme, but it is not placed properly - see screen-2.bmp

Using customized widecabble theme,
8. Conditional viewports are not working properly (at least %?mv) - see screen-3.bmp
9. sbs icons not displaying (in sbs or wps) - see screen-3.bmp
10. In the sbs, this line is used for the clock display:

%?bs<%t(1)%s%al%bs|%al%cl:%cM>;%?bs<%t(5)%s%al%cl:%cM|%t(0)>

My understanding is that it should display the clock continuously if no sleep timer is set, and alternate the clock for 5 secs. and sleep timer for 1 sec. if a sleep timer is set (it does work like this with svn).

With newparser3a.diff, with a sleep timer set, it alternates at approximately a 2 sec. interval between the clock and the sleep timer. With no sleep timer set, it alternates at approximately a 2 sec. interval between the clock and nothing (clock on 2 secs. and then off 2 secs.)
Comment by Jonathan Gordon (jdgordon) - Monday, 12 July 2010, 22:33 GMT
thanks.
6 - I saw that and wasnt sure if I was crazy or not... they are 2 pixels down or something?
7 - yeah, fixme in the code for that.. tonight maybe
8 - not sure if they work at all yet actually :) tonight maybe
9 - could be a theme issue,
10 - il investigate :)
Comment by Jonathan Gordon (jdgordon) - Tuesday, 13 July 2010, 14:08 GMT
6,7 fixed, 9 is probably fixed also (if it is the same problem as the pb not drawing)

hopefully no more crashes when loading themes.
Reworked the progressbar parser function so it uses the new stuff so look out for mistakes there also.

Ignore the massive amounts of warnings...
Comment by Michael Chicoine (mc2739) - Tuesday, 13 July 2010, 15:15 GMT
With latest patch, 6, 7, and 9 are fixed.

10 is mostly fixed - it is now correct with no sleep timer, but still alternates at a 2 sec. interval when a sleep timer is set.

Crash on theme load is still happening. On target, I get either a frozen device which needs a long power press to turn off, or I get a data abort at address 30027378 (different than I reported in #rockbox, but still find_viewport in the map file). The crash seems to happen more when loading a large theme (like my modified widecabbie which I am attaching this time). Switching between smaller themes does not seem to crash, but causes visual problems - see screen-4.bmp (very similar to screendumo.bmp above, but more obvious that everything but backdrop is shifted down). This was with cabbiev2, which does not use viewports, but I don't know if that is relevent.
Comment by Jonathan Gordon (jdgordon) - Wednesday, 14 July 2010, 13:34 GMT
Update time.
Conditional viewports now work, I havnt looked into the timeout thing yet.

I forgot to mention earlier that the draw order is now completly dependant on the skin code, so images are actually drawn when a %Xd is hit instead of at the end of the viewport like in svn. This means skins might need a tiny bit of work to make them work properly... not a problem if you use viewports though.

I'll try to figure out that crash tomorrow...
Comment by Frank Gevaerts (fg) - Wednesday, 14 July 2010, 13:59 GMT
I'll test as soon as there's a version that actually compiles
Comment by Michael Chicoine (mc2739) - Wednesday, 14 July 2010, 17:22 GMT
newparser.5.diff does not build due to printf at lines 1553 and 1580 of the patch. Commenting those two lines allows building (with make bin).

With my widecabbie-mod theme, sbs is not working. The default (inbuilt) status bar is displayed instead. Changing the status bar to off or from off to top/bottom hard locks the e200v2 and closes the sim with no message.

Conditional viewports are better, although there may still be problems with the fms (or it may be display order related).

Progressbar is not working in cabbiev2, but is working in themes with viewports. It was working in newparser.4.diff
Comment by Jonathan Gordon (jdgordon) - Wednesday, 14 July 2010, 22:33 GMT
Frank, do a make bin, it doesnt change codec/plugin apis so is safe.
the progressbar is working, but it is being drawn over later, you need to use viewports or move the %pb() line to the end
Comment by Jonathan Gordon (jdgordon) - Thursday, 15 July 2010, 09:26 GMT
minor update... empty lines dont get cleared anymore but emtpy sublines do.. this fixes cabbie's progressbar in the e200 and hopefully your progressbar issue also.
also sync to svn, nothing else changed
Comment by Jonathan Gordon (jdgordon) - Thursday, 15 July 2010, 12:15 GMT
this hopefully doesnt crash anymore. I'm pretty sure sublines inside conditionals was never supported in svn and isnt implemented here yet either.

This should compile plugins file also

edit: 6a is resynced to svn
Comment by Jonathan Gordon (jdgordon) - Thursday, 15 July 2010, 13:41 GMT
almost the whole point of the excersize! down to only 4 more seperate sub parseing functions left. This patch also makes the playlist viewer work
Comment by Jonathan Gordon (jdgordon) - Saturday, 17 July 2010, 11:15 GMT
Getting very close to being finished.. the only thing I know for sure to not work is the list title and icon tags. everything else should work.
Comment by Jonathan Gordon (jdgordon) - Saturday, 17 July 2010, 13:13 GMT
use the single skin buffer correctly., make checkwps compile
Comment by Frank Gevaerts (fg) - Saturday, 17 July 2010, 13:33 GMT
I have two issues with this, using http://themes.rockbox.org/index.php?themeid=687&target=gigabeatfx

- scrolling doesn't work generally. I only see things scroll in the brief time between pressing stop and the system going back to the WPS (I have bookmarks-on-stop enabled, so this takes a second or so on a disk target)
- when starting playback, the WPS first shows the non-AA variant, and then when the AA gets loaded it switches to the AA variant, as expected. However, the conditional viewports for non-AA do not get cleared when switching, so the old text remains visible.

Use a track with relatively long album or title tags to see this
Comment by Jonathan Gordon (jdgordon) - Sunday, 18 July 2010, 10:33 GMT
This one hopefully fixes those issues... I dont have any disk targets here so cant really test the AA fix (seems to work in the sim with a noticable draw issue though :/ )

the hack to make that work is to force line updates twice after a conditional changes. it aint pretty but it works...

edit: more info to maybe get another brain thinking about the problem...

when a conditional changes values the false branch is checked for things which need to be cleared (like AA and images), when that happens the image will clear the whole area including any new text which draws over it, so to fix that the next update forces the line to update again (if i didnt it would only work if that line needed to scroll)
Comment by Frank Gevaerts (fg) - Sunday, 18 July 2010, 13:26 GMT
This one does indeed seem to fix my issues.
I still see the non-AA variant pop up for a moment on track change. I don't remember seeing that with svn, but I'm not sure.
Comment by Jonathan Gordon (jdgordon) - Sunday, 18 July 2010, 14:07 GMT
I'm doing a build for my mini2g now (I've been holding off because title stuff isnt working yet which my theme needs..) I know that cabbie on it does show the no aa varient with svn (which doesnt surprise me at all), so if this shows it also it doesnt worry me.

5 min pause...

OK crap... "undefined instruction" and "prefetch abort"on the mini2g (no eabi build) (actually I'm hoping they are from codec mismatch) and the area around the AA image is fubar... crap...
Comment by Frank Gevaerts (fg) - Sunday, 18 July 2010, 14:44 GMT
The non-AA variant also appears for a moment with svn. That means I see no regressions.
Comment by Michael Chicoine (mc2739) - Sunday, 18 July 2010, 18:02 GMT
With newparser.10.diff and r27477 I am now getting the following on boot (on device only):

Data abort
at 30028AAC
FSR 0x8
(domain 0, fault 8)
address 0xEA000067

Per the map file, the is in skin_render.o
0x30028a4c skin_render


Also, with this version, on the sim using my modded widecabbie,

1. I do not see any scrolling in the WPS
2. Clock is again alternating on and off (item 10 above, which was working)
3. SBS icons (battery, shuffle, repeat, volume, play status) all blink off then back on when updating
Comment by Jonathan Gordon (jdgordon) - Monday, 19 July 2010, 07:59 GMT
I'm slightly disappointed by this change but it is necessary (for the time being anyway) :/ go back to the same draw order as in svn. no more AA weirdness and images flickering, and no more nasty redraw hacks... but back to shitty unknown ordering...
Comment by Jonathan Gordon (jdgordon) - Tuesday, 20 July 2010, 13:13 GMT
everything should be fixed again... the clock alternating is because you need to change to sublines inside conditionals instead of conditionals inside sublines (this is a new feature)
Comment by Michael Chicoine (mc2739) - Tuesday, 20 July 2010, 15:13 GMT
The data abort I noted above was on the e200v2 and may be fixed by r27495. The data abort is not happening on e200v1.

Album art in wps looks good now.

I'm still having problems with viewports and album art in the fms (widecabbie-mod) when switching from a preset with album art to a preset without album art. I have attached screen shots of what I am seeing. If I leave the fm screen with the menu button and then return it will correct the display.

Scrolling still is not working at all using widecabbie-mod. With some further testing, I found if I have a wps with this single line:

%sThis is just a very long line to test scrolling in the new wps parser.

it will scroll properly. If I change to this:

%V(0,0,-,-,0)
%sThis is just a very long line to test scrolling in the new wps parser.

the sim (e200v2 r27477) crashes and target (e200v1 r27477) freezes. Change to this:

%Vd(a)
%Vl(a,0,0,-,-,0)
%sThis is just a very long line to test scrolling in the new wps parser.

and the sim does not crash, but the line does not scroll. If I add a comment line above the %V like this:

#
%V(0,0,-,-,0)
%sThis is just a very long line to test scrolling in the new wps parser.

I get no crash and the scrolling does work. This does not help with the %Vd/%Vl though.
Comment by Tomasz Kowalczyk (mitk) - Wednesday, 21 July 2010, 08:46 GMT
Fuze v1 with build you made from svn27509:

Data abort
at 30028D7C
FSR 0x8
(domain 0, fault 8)
address 0xEA000067

No data abort with clean, non patched svn27509.
Comment by Johannes Linke (Jaykay) - Wednesday, 21 July 2010, 09:08 GMT
the scrolling issue i have sounds like the one posted above, but here are my results:

in BlacknBlue Glass, Cabbie plus, Freestate, Rockpod and SpartanBlack scrolling does not work (scrolling lines dont scroll).

in ART, AzureUltimate, Absolute Black, ART in red, arbox Widgets, scrolling does work.
Comment by Michael Chicoine (mc2739) - Wednesday, 21 July 2010, 11:30 GMT
The data abort reported by mitk sounds like the same I am seeing. Here is the info on mine:

e200v2 r27477

Data abort
at 30028D84
FSR 0x8
(domain 0, fault 8)
address 0xEA000067

Per the map file, the is in skin_render.o
0x30028d24 skin_render


Notice the pattern, the abort address is 0x60 higher than the start of skin_render. This is the third build that I have seen this.
Comment by Clément Pit--Claudel (CFP) - Wednesday, 21 July 2010, 11:35 GMT
Hello Jonathan,

I can't manage to patch properly; here's the output:
clement@clement-laptop:~/svn/rockbox$ patch -p1 < newparser.12.diff
patching file apps/SOURCES
patching file apps/cuesheet.c
patching file apps/gui/skin_engine/skin_backdrops.c
patching file apps/gui/skin_engine/skin_buffer.c
Reversed (or previously applied) patch detected! Assume -R? [n] y
Hunk #1 FAILED at 1.
1 out of 1 hunk FAILED -- saving rejects to file apps/gui/skin_engine/skin_buffer.c.rej
patching file apps/gui/skin_engine/skin_buffer.h
Reversed (or previously applied) patch detected! Assume -R? [n] y
Hunk #1 FAILED at 1.
1 out of 1 hunk FAILED -- saving rejects to file apps/gui/skin_engine/skin_buffer.h.rej
patching file apps/gui/skin_engine/skin_display.c
patching file apps/gui/skin_engine/skin_display.h
patching file apps/gui/skin_engine/skin_engine.h
patching file apps/gui/skin_engine/skin_fonts.c
patching file apps/gui/skin_engine/skin_fonts.h
patching file apps/gui/skin_engine/skin_parser.c
Hunk #1 succeeded at 8 with fuzz 1.
patching file apps/gui/skin_engine/skin_render.c
patching file apps/gui/skin_engine/skin_tokens.c
patching file apps/gui/skin_engine/skin_tokens.h
patching file apps/gui/skin_engine/wps_debug.c
patching file apps/gui/skin_engine/wps_internals.h
patching file apps/gui/statusbar-skinned.c
patching file apps/gui/theme_settings.c
patching file apps/gui/viewport.c
patching file apps/gui/viewport.h
patching file apps/gui/wps.c
Hunk #7 succeeded at 1116 (offset -8 lines).
patching file apps/main.c
patching file apps/menus/main_menu.c
patching file apps/misc.c
patching file apps/misc.h
patching file apps/radio/radio.c
patching file apps/recorder/albumart.h
patching file apps/settings.h
patching file lib/skin_parser/SOURCES
patching file lib/skin_parser/skin_buffer.c
patching file lib/skin_parser/skin_buffer.h
patching file lib/skin_parser/skin_debug.c
patching file lib/skin_parser/skin_debug.h
patching file lib/skin_parser/skin_parser.c
patching file lib/skin_parser/skin_parser.h
patching file lib/skin_parser/skin_scan.c
Hunk #2 FAILED at 115.
1 out of 2 hunks FAILED -- saving rejects to file lib/skin_parser/skin_scan.c.rej
patching file lib/skin_parser/skin_scan.h
patching file lib/skin_parser/tag_table.c
patching file lib/skin_parser/tag_table.h
patching file tools/checkwps/SOURCES
patching file tools/checkwps/checkwps.c
patching file tools/checkwps/checkwps.make
patching file tools/root.make
patching file utils/newparser/skin_render.c

Did I do something wrong?
Comment by Clément Pit--Claudel (CFP) - Wednesday, 21 July 2010, 11:35 GMT
by the way, what are those a and b folders?
Comment by Jonathan Gordon (jdgordon) - Wednesday, 21 July 2010, 11:58 GMT
yeah, trying to track down the data abort now.

you can ignore the skin_buffer.[ch] failures, those files are deleted. skin_scan.c is odd, unless that has just been updated in svn it means you arnt up to date yet. shouldnt be too dificult to manually sync.

a/ is the origional folder, b/ is the new files in git
Comment by Jonathan Gordon (jdgordon) - Wednesday, 21 July 2010, 14:25 GMT
try this patch... hopefully fixes crahses
Comment by Michael Chicoine (mc2739) - Wednesday, 21 July 2010, 14:57 GMT
e200v2 no longer gets data abort with newparser.13.diff
Comment by Tomasz Kowalczyk (mitk) - Wednesday, 21 July 2010, 16:05 GMT
Fuze v1

svn 27509 patched with newparser.13.diff, cabbiev2:

1. No scrolling
2. After starting playback by selecting file to play or after boot and then resume playback I've got duplicated track info text overlapped by AA, as you can see on picture1.jpg. Skipping to next track doesn't change this. Going to main menu and then back to wps I've got no duplicated text and no AA, look at picture2.jpg
3. After reloading cabbiev2 theme from Theme Menu I've got normal theme wps look but without scrolling


Same firmware version and BlueBlackness theme:
1. Scrolling is working in sbs
2. Menu is looking normal
3. Theme wps doesn't load. In place I've got rockbox default simple text&peakmeter wps.

Same firmware version with Azure+ theme is looking good but still no scrolling

Comment by Michael Chicoine (mc2739) - Thursday, 22 July 2010, 00:07 GMT
Update on fms problems noted above: http://www.rockbox.org/tracker/task/11470#comment36467

This same problem exists in svn. I did a binchop to determine when it first started, and found that r27303 works and r27304 fails. I have also found that this has been previously reported in  FS#11469  and the same working/failing revisions were reported there.
Comment by Jonathan Gordon (jdgordon) - Sunday, 25 July 2010, 11:56 GMT
New one to try... I've had to "break" some things to make it work but I think those changes were sensible anyway. Viewports are now cleared when they are being disabled (so if you were relying on them not being cleared your themes will break), and now *only* images are drawn in the default viewport if other viewports are defined. progressbars and peakmeters are NOT drawn.

everything is hopeflly working now!

Comment by Michael Chicoine (mc2739) - Sunday, 25 July 2010, 18:47 GMT
Scrolling is now working.

On the FM screen, there are still update problems as noted above, although, the screen will update properly when changing from from a station with albumart to a station without albumart, or when switching from scan mode to preset mode and back.

With the cabbiev2 theme (without viewports), switching tracks looks quite strange. It will first display the "no albumart" scenario and lines of text will overwrite parts of the previous albumart. Then it will decide albumart is available and switch to the "albumart" scenario. Other themes (with viewports) also switch between "no albumart" and "albumart" scenarios, but they at least clear out the albumart viewport before overwriting.
Comment by Jonathan Gordon (jdgordon) - Sunday, 25 July 2010, 22:36 GMT
fm screen is seperate and I'll fix that once this is done.
the showing no-aa then aa is not a new thing, svn does it also, just trying to make it smoother
Comment by Tomasz Kowalczyk (mitk) - Monday, 26 July 2010, 09:46 GMT
fuze v1, r27544 patched with newparser.14a.diff:

cabbiev2, cabbie v3-blue:
1. Still doesn't display AA after going to main menu and then back to wps, same like on http://www.rockbox.org/tracker/task/11470?getfile=22336 . Track change because the end of track or by pressing "next" doesn't make AA visible. Pressing "next" 2 times or more solves this.
2. Doesn't change AA between tracks. Previous track AA is displayed. Again, pressing "next" 2 times or more solves problem


Also tested: arborWidgets, Azure+, BlueBlackness, Pen&Paper, Spartan Black - didn't saw anything bad.

FM not tested at all, following your statement I assumed it is to early now for testing it. True?
Comment by Jonathan Gordon (jdgordon) - Monday, 26 July 2010, 11:38 GMT
this should fix AA... please oh PLEASE tell me this is finished :D
Comment by Michael Chicoine (mc2739) - Monday, 26 July 2010, 11:57 GMT
Why did your patch double in size? It looks like it has a lot of unrelated stuff in it.
Comment by Jonathan Gordon (jdgordon) - Monday, 26 July 2010, 12:41 GMT
make charcell compile
Comment by Frank Gevaerts (fg) - Monday, 26 July 2010, 16:36 GMT
Player doesn't compile for me with r27576 and 15b

Loading...