FS#6862 - WPS Tokenizer

Attached to Project: Rockbox
Opened by Nicolas Pennequin (nicolas_p) - Tuesday, 20 March 2007, 01:29 GMT
Last edited by Nicolas Pennequin (nicolas_p) - Wednesday, 04 April 2007, 16:10 GMT
Task Type Patches
Category Themes
Status Closed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No


This is my WPS tokenizer patch. The basic idea is to store the WPS as an array of tokens. That way it is only parsed once and transformed into a form easier to use for displaying.
The concept is somewhat similar to what's explained here : There are also some explanations on  FS#4826  and  FS#2898 .

To make this patch, I used the code in  FS#4826  as a base and improved the parser (I posted some code on that entry). After that, I started working on integrating it into the current code. This proved to be quite hard, so I decided to rewrite some parts of it, namely gui_wps_refresh() and most of the functions it uses.

It appears to be working nicely with most of the WPS files I tested on most targets.
It still needs to go through extensive testing, so please apply it to your build and test some WPS files :)
Please report any crashes you get (although there shouldn't be any), and post feedback on performance differences, if you notice any (maybe try to activate heavy CPU features like crossfeed, EQ, replaygain, etc... and compare with an SVN build).
Also I'm not sure at all it will work well on the archos player, so it would be good if someone could try that too.
This task depends upon

Closed by  Nicolas Pennequin (nicolas_p)
Wednesday, 04 April 2007, 16:11 GMT
Reason for closing:  Accepted
Additional comments about closing:  Comitted to SVN. Please report problems on the forum : c=9727.0
Comment by Nicolas Pennequin (nicolas_p) - Wednesday, 21 March 2007, 19:17 GMT
New version, with the image preloading code adapted to the parsing code.
Also the debug code has been moved into a separate file.
Comment by Nicolas Pennequin (nicolas_p) - Thursday, 22 March 2007, 00:39 GMT
A few corrections, especially a very important check I forgot : the number of lines and sublines can't be allowed to become greater than the max values. This caused serious problems on long WPS files.
Comment by Nicolas Pennequin (nicolas_p) - Tuesday, 27 March 2007, 23:56 GMT
I added a way to hide images within a conditional when they aren't supposed to be displayed anymore. Also a few other minor improvements and some additional code commenting.

Please test and/or review !
Comment by Nicolas Pennequin (nicolas_p) - Wednesday, 28 March 2007, 23:21 GMT
A few updates. I think this patch is now close to being committable :)
Comment by Dan Everton (safetydan) - Thursday, 29 March 2007, 04:45 GMT
Just some minor comments from a quick review of the patch.

* There's an #if 0 block in gwps-common.c. Should that still be there?
* This code

+ /* Skip the rest of the line */
+ while(*(wps_token + skip) != '\\n')
+ skip++;

shows up in a few places. Would it be worth factoring that out in to a function? Might save some bytes.
Comment by Nicolas Pennequin (nicolas_p) - Friday, 30 March 2007, 12:51 GMT
The #if 0 might become useful again so I'm keeping it for the moment.
Thanks for the other suggestion, it's done :)
Another update should be arriving shortly.
Comment by Nicolas Pennequin (nicolas_p) - Friday, 30 March 2007, 13:44 GMT
So here is the updated version.
Most of the updates are discusses here :
There are a few others too, but nothing really important.
The good news is with all the duplicate code removal, binsize seems to have gone down a little. I hope I'll be able to make other improvements in that area :)
Comment by Alexandre Flament (flament) - Saturday, 31 March 2007, 17:52 GMT
I have tested the wps_tokenizer.patch (30 march 2007 version) with my Archos FM Recorder.

It seems to work with the themes I have tested except one : the rockbox_default has some pixels are set randomly (?) on the last line of the screen (just bellow the progress bar). Some pixel are set, some other are clear during the playback, but the line is never filled. The simulator has the same problem. The others themes I have tested : black-white, boxes, iCatcher, relief, Rockboxed (found here : )

If HAVE_TAGCACHE is not defined, compilation is done, but the player freezes every time the WPS should appear.

For information : the rombox.ucl is ~1ko bigger with this patch (compiled without tagcache).

Note : I compiled rockbox using the VMPlayer image and I made a dist-upgrade, but sh-elf-gcc is still the version 4.0.3 (rocbox patch #1).
Comment by Nicolas Pennequin (nicolas_p) - Saturday, 31 March 2007, 18:16 GMT
Thanks for the report :)
I think the pixels under the progressbar on the default WPS are due to the peakmeter... I'll take a closer look at it.
Also I'll check why not having tagcache causes a crash.
Comment by Nicolas Pennequin (nicolas_p) - Sunday, 01 April 2007, 13:38 GMT
Update: Moved the code around and tweaked some things to reduce memory usage.
Comment by Nicolas Pennequin (nicolas_p) - Monday, 02 April 2007, 19:48 GMT
* Fix the issue with the peakmeter described above
* Fix crashing when the WPS had no newline at the end of the file (reported by pixelma)
* Fix some issues with sublines display
* Adapt to amiconn's archos player fixes. Now both the progressbars are displayed correctly.
* Some other changes I don't remember :)
Comment by Nicolas Pennequin (nicolas_p) - Wednesday, 04 April 2007, 00:37 GMT
* Memory improvements, mainly by removing the format_buffer where the WPS source was stored. The parser now uses the plugin buffer as temporary storage for the source.
* BMP loading was rewritten to make the pictures be loaded all at once after the parsing is finished
* Other improvements I can't remember
Comment by Nicolas Pennequin (nicolas_p) - Wednesday, 04 April 2007, 14:15 GMT
Small update with BMP loading.