Rockbox

Tasklist

FS#6991 - Reorganise data structure for WPS

Attached to Project: Rockbox
Opened by Alexander Levin (fml2) - Saturday, 07 April 2007, 12:57 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

Details

Here is my take on what has been proposed on irc on Apr 5 2007, about 13:00 (according to the irc log).

This did reduce the size of the binary, at least on my H120.

Here's the extract from the irc log:

13:04:02 crop I'd reorganise the wps_data struct.
13:04:24 crop 1. make progress_xxx vars unsigned
13:05:15 crop 2. intriduce struct subline with the fields start_token_idx, possibly num_tokens, line_type, and time_mult
13:06:04 crop Introduce struct line with the fields start_subline_idx, possibly num_sublines, subl_expire_time, and curr_subline
13:07:12 crop The idea is that all sublines are stored in one array and the fields point to that array. Now it's assumed that each line can have up to 12 sublines which is rarely the case I'd argue and this wastes space
13:07:43 crop Tokens are already stored this way.
13:08:07 crop Structuring the data into structures would also make the code more readable IMHO
13:10:09 crop And one more thing: why do we need tokens for NOP and EOL? Why not just store the tokens that belong to the subline? In my proposal, start_subline_idx would say where the subline starts (index in the tokens array). The num_tokens isn't strictly needed since a subline ends where the next begins. The same idea applies to lines.
13:10:58 crop This would simplify the drawing code since you have clear bounds and don't have to watch out if we've reached EOL. This should be done at parsing time IMHO
This task depends upon

Closed by  Nicolas Pennequin (nicolas_p)
Sunday, 08 April 2007, 04:09 GMT
Reason for closing:  Accepted
Additional comments about closing:  Committed, thanks
Comment by Nicolas Pennequin (nicolas_p) - Saturday, 07 April 2007, 13:58 GMT
Nice!
Do you come on the IRC channel sometime ? I'd like to discuss your patch in further detail.
I've read most of it and two things caught my eye:
* You don't seem to have respected how subline timeout values are refreshed, especially in the case of conditionals. This caused me problems and was the reason I added get_subline_timeout(), which you seem to have removed and not rewritten.
* Maybe having a "num_tokens" field in the subline struct would make things simpler and remove the need for wps_last_token_index()
* When I tested, the RTC tag caused a crash. That's because the displaying code relies on the fact that there will be a WPS_TOKEN_RTC before any other RTC token.
Comment by Dominik Riebeling (bluebrother) - Saturday, 07 April 2007, 14:02 GMT
from a quick look, wordForNext() catched my eye as it doesn't comply to our code policy. Just thought I mention it ;-)
Comment by Nicolas Pennequin (nicolas_p) - Saturday, 07 April 2007, 16:24 GMT
OK, I made some changes to the patch. I think it's almost ready to commit, but I'd like you to review it first :)
* The main change was to make sublines work as they should. update_curr_subline() is back in its original state (only with the necessary changes to access the data struct), because your version with a while loop could cause infinite looping when no sublines are to be displayed, which is very possible. This lead me to make a few changed related to the time_mult values.
* There was a mistake in the RTC tag handling which I fixed.
* I renamed wordForNext().
* I made a few other minor changes.

I decided it wasn't worth adding a "num_tokens" field in the subline struct, as the code is pretty clear as it is.
Also, binsize seems to shrink a fair bit, which is very nice :)

I hope to commit this soon.
Comment by Alexander Levin (fml2) - Saturday, 07 April 2007, 18:38 GMT
Nico, I think that the subline timeout parsing (and handling) should be slightly changed. Timeout can also be specified outside of a conditional. It should then be applied to all cases of the conditional.

For example: %t3%?it<%it|%fn> (compare with %?it<%t5%it|%t10%fn>)

Is this correctly handled?

And what about %?it<%it|%fn>%t3, i.e. if the time_mult is specified at the end?
And this: %it%t3 , i.e. without conditionals at all?

In general, I find the code with 'only_one_subline' too tricky. Why is this fragment so complicated? The goal is simple: find the next subline with display time > 0. Apart from the fact that I understood time tags wrong, I find my impl simpler.

I think, one of the cases above doesn't work. Try this: %t4%it;%t1%ia
Comment by Nicolas Pennequin (nicolas_p) - Saturday, 07 April 2007, 18:49 GMT
Yes, subline timeout values are handled fine even when they are outside conditionals.
About the 'only_one_subline' code, we need to find the next subline with display time > 0, but sometimes there isn't any so in your code there was an infinite loop.
Your %t4%it;%t1%ia example works fine for me.
Comment by Alexander Levin (fml2) - Saturday, 07 April 2007, 21:23 GMT
> but sometimes there isn't any so in your code there was an infinite loop.

Hm... I thought I checked whether the starting point has been reached and leave the loop then.

I also don't quite understand the stuff with 100*HZ as in

data->lines[line].subline_expire_time = (reset_subline ?
current_tick : data->lines[line].subline_expire_time) + 100 * HZ;

And why the expiration time is calculated this way. Why isn't time_mult added to the current tick?
Comment by Alexander Levin (fml2) - Saturday, 07 April 2007, 21:29 GMT
OT: @Nico: have you seen my comment about the funny definition: %cp is AM/PM and %cP is am/pm (lowercase p is for uppercase AM/PM and vice versa). Shouldn't we change this?
Comment by Nicolas Pennequin (nicolas_p) - Saturday, 07 April 2007, 22:08 GMT
I agree P for lowercase and p for uppercase is weird, but I'm not sure it's worth changing it now. It's not the main purpose of this patch anyway so we can discuss it later.
About the current subline calculation code : the expiration time is another value. I'm not completley sure of its purpose but it does seem necessary.
If you agree I'd like to commit the patch soon. Do you have any other changes to make ?
Comment by Nicolas Pennequin (nicolas_p) - Sunday, 08 April 2007, 04:08 GMT
I committed this with a few changes, the main ones being a mistake fixed in update_curr_subline() and much smaller struct wps_line and struct wps_subline.
If you have other changes to suggest, please submit a new patch :)

Loading...