Rockbox

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

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#12320 - need substitute to /.rockbox/skin_buffer_size.txt

Attached to Project: Rockbox
Opened by Sadurní (sadur) - Friday, 07 October 2011, 17:24 GMT+2
Last edited by Jonathan Gordon (jdgordon) - Monday, 14 November 2011, 12:31 GMT+2
Task Type Bugs
Category Themes
Status Closed
Assigned To No-one
Player Type iPod 5G
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Private No

Details

Since version r30718 "Revert r30599 "Check for the magic file "/.rockbox/skin_buffer_size.txt" my WPS is not loading again. With the last changes to buflib my WPS didn't work until I've used "/.rockbox/skin_buffer_size.txt" to load my WPS that is 47.2KB in size ("system > rockbox info > skin buffer usage").
Is there a way to fit my WPS on the buffer? (My theme is BigInfoAA on the Themes page)
At the moment, I switch it back to r30717.
This task depends upon

Closed by  Jonathan Gordon (jdgordon)
Monday, 14 November 2011, 12:31 GMT+2
Reason for closing:  Fixed
Additional comments about closing:  proper fix should be in svn in the next day or two
Comment by Thomas Martitz (kugel.) - Friday, 07 October 2011, 21:01 GMT+2
Can you try this patch? It optimizes one often allocated struct for size.
   beter-struct-size.diff (1.4 KiB) 
 b/lib/skin_parser/skin_parser.h |   26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

Comment by Jonathan Gordon (jdgordon) - Saturday, 08 October 2011, 15:47 GMT+2
That saves barely 2K on e200 cabbie (run the two attached .log files through lib/skin_parser/skin_buffer_debug.pl). i.e a completely pointless patch. The *only* correct fix is to figure out how to get that struct onto buflib, all other patches are a waste of time.

edit: the next biggest allocation is apps/gui/skin_engine/skin_parser.c:1851 but even that is barely 2k (on cabbie), and this is an easier one to convert to buflib but even that is pointless
   svn.log (49.7 KiB)
   patched.log (49.7 KiB)
Comment by Sadurní (sadur) - Saturday, 08 October 2011, 16:27 GMT+2
I've tried it and it didn't work for me. :'(
Comment by Thomas Martitz (kugel.) - Sunday, 09 October 2011, 12:46 GMT+2
How do you use skin_buffer_debug.pl, or how do you generate the logs?
Comment by Jonathan Gordon (jdgordon) - Sunday, 09 October 2011, 12:52 GMT+2
skin_buffer_debug.pl file.log | sort -k2 -nr
enable the debug line in libs/skin_engine/skin_buffer.h
Comment by Thomas Martitz (kugel.) - Sunday, 09 October 2011, 13:14 GMT+2
Okay, making the structs smaller obviously doesn't help since the skin buffer takes their size into account. MAX_WPS_TOKEN can be increased according to the space saved by the smaller structs.
Comment by Thomas Martitz (kugel.) - Sunday, 09 October 2011, 13:19 GMT+2
This does bump MAX_WPS_TOKEN a bit, for the space changed by decreasing the struct sizes. Makes BigInfoAA fit on the 64bit sim
   better-struct-size-v2.diff (3 KiB) 
 b/apps/gui/skin_engine/skin_tokens.h   |   11 +++++------
 b/apps/gui/skin_engine/wps_internals.h |    2 +-
 b/lib/skin_parser/skin_parser.h        |   31 ++++++++++++++++---------------
 3 files changed, 22 insertions(+), 22 deletions(-)

Comment by Jonathan Gordon (jdgordon) - Sunday, 09 October 2011, 13:28 GMT+2
How exactly is that better than the change you reverted? It is still an arbitrary limit. OK, BigInfoAA fits but some other skin wont. The only sane thing to do is put skin_element on buflib or make the buffer size configurable
Comment by Thomas Martitz (kugel.) - Sunday, 09 October 2011, 13:36 GMT+2
It's a fix to a regression introduced by the skin-engine-to-buflib changes. Without extra cost, and without introducing questionable new features.
Comment by Sadurní (sadur) - Monday, 10 October 2011, 08:01 GMT+2
This afternoon I'll give it a try.
Thanks.
Comment by Sadurní (sadur) - Monday, 10 October 2011, 17:05 GMT+2
It works for me. Is this a definitive solution, or is there anything I could try/do to help find a better solution?
Comment by Jonathan Gordon (jdgordon) - Monday, 17 October 2011, 00:43 GMT+2
The commit is a stop gap solution but not a real fix. This task shouldnt have been closed.

Loading...