Rockbox

Tasklist

FS#11615 - Dynamic screen size

Attached to Project: Rockbox
Opened by Maurus Cuelenaere (mcuelenaere) - Tuesday, 07 September 2010, 18:10 GMT
Task Type Patches
Category Operating System/Drivers
Status New
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 0%
Votes 0
Private No

Details

This patch converts LCD_{WIDTH,HEIGHT} into variables, allowing Rockbox to change screen size at runtime.

I've tried to make the changes as less intrusive as possible, but still, there will be some binsize increases.

Most of the #if's were converted to C if()'s which should be functionally equivalent (and if the compiler is smart enough, no increase in binsize should occur).

In order to use this, just define DYNAMIC_LCD_SIZE in your target config and compile without plugins (if the preprocessor spits errors, try clearing ENABLEDPLUGINS in your Makefile).

What still needs to be done:
* figure a way out how to compile several rockbox logos into the binary and choose the correct one at runtime
* do something about the hacks in apps/bitmaps/SOURCES and apps/settings_list.c
* get the Android port working
This task depends upon

Comment by Jonathan Gordon (jdgordon) - Thursday, 04 November 2010, 10:32 GMT
I'm looking into getting this finished, a few comments:
1) why the changes to apps/recorder/bmp.c? Seems the end result is the same?
2) ditto firmware/bidi.c ?
3) scroll engine... we aren't memory constrained on these targets so lets just set LCD_SCROLLABLE_LINES to something massive like 64 or so?
4) built in bitmaps: we could load them at runtime instead of compile time with a nice function to grab the right size (i.e <image name>.widthxheightxdepth.bmp )

I think I've freed up the skin engine which could have caused big problems so I'm going to apply this now and see how it goes (come on irc if you can :) )
Comment by Jonathan Gordon (jdgordon) - Thursday, 04 November 2010, 12:02 GMT
Synced and updated version:
- scroll engine is totally stuffed
- loads backdrops and skins with the widthxheightxdepth extension so builds will need to have all the cabbie theme files for the dimensions we want to support.

I've only fixed the sdl app for now... "--display <width>x<height>" on the command line will set the size. sims still seem to work but I'm not sure if it is correct.

to test it with the themes "cp -R backdrops wps /usr/local/share/rockbox" from your rockbox tree
Comment by Jonathan Gordon (jdgordon) - Thursday, 04 November 2010, 13:32 GMT
this is my crude attempt to fix android (apply to above patch).... it segfaults after init and I don;t know where/why
Comment by Thomas Martitz (kugel.) - Thursday, 04 November 2010, 17:21 GMT
The last patch is more complicated than needed. You can get the display metrics in the Framebuffer class too, then you can query the dimensions after creating the object.
Comment by Maurus Cuelenaere (mcuelenaere) - Friday, 05 November 2010, 09:54 GMT
1) BM_MAX_WIDTH probably depends on LCD_{HEIGHT,WIDTH}, so ba.buf can't be statically alloced
2) same reason (allocation is on-stack this time though)
3) why not just allocate this dynamically? That way, there are no limits to run into in the future and it shouldn't result in a large binsize increase (talking about firmware/scroll_engine.c changes)
4) I was thinking of modifying bmp2rb to output an array of possible images, but dynamically loading them at runtime seems OK too

P.S.: your apps/gui/skin_engine/skin_parser.c change looks unrelated + you have some whitespace changes in apps/plugin.c
Comment by Jonathan Gordon (jdgordon) - Saturday, 06 November 2010, 10:39 GMT
1,2) ah ok
3) do you mean LCD_SCROLLABLE_LINES should be dynamically alloced based on the screen size? or properly dynamically allocated as needed? the first seems a bit pointless because you still have a artificial limit (so using a hardcoded large value is just as good and simpler).
4) we don't want to load too many images into the binary if we can avoid it, what could be fun is loading them all into the end of the *audiobuffer* region and then copying the one which is actually used to some buffer_alloced() block so the ram is reclaimed.

the skin_parser change is relevant, (It needs a comment). skin images are loaded from a directory with the same name as the skin file (i.e cabbiev2.wps -> wps/cabbiev2/) now with the above changes we could be loading cabbiev2.60x480x16.wps but we want the images to still be in the wps/cabbiev2 folder.. so that change does this.
Comment by Thomas Martitz (kugel.) - Saturday, 06 November 2010, 11:48 GMT
Aren't the images in the wps' subfolder also screen size dependent? I.e. the 480x800 cabbie has larger icons (and a backdrop) in the wps/cabbiev2/ folder than the 320x480 one. So I'd think if we load cabbiev2.640x6480x16.wps (assuming there's another .wps for a different screen size in the same folder) we also want the suitable images from wps/cabbiev2.640x480x16.wps.

I think we should package cabbiev2 separate, somehow. How about we put cabbiev2 into libcabbiev2.so (either libcabbiev2 which has it for all dimensions, or one libcabbiev2.XXXxYYYxZZ.so for each) and extract that separatenly, possibly to the sdcard directly? Or we make it so that the right cabbiev2 is downloaded on the first run?
Comment by Jonathan Gordon (jdgordon) - Saturday, 06 November 2010, 12:00 GMT
well the skin images all have their screen size on the filenames anyway which is why it works. packaging up cabbie into a zip seems like more effort than needed, and will break if/when we do remote screen widgets which will need them also and wont be know which to use untill widget creation which could be after rockbox is up and running.
I thought we really didnt want to be downloading anything on first run? Althoughnot having to package the fonts into the zip would be very helpful
Comment by Maurus Cuelenaere (mcuelenaere) - Saturday, 06 November 2010, 16:09 GMT
3) LCD_SCROLLABLES_LINES *is* dynamically based on the screen size. LCD_WIDTH & LCD_HEIGHT are dynamic now, remember? Basically, all changes in my original patch address static allocations based on LCD_{WIDTH,HEIGHT} (even if that isn't very clear from the patch POV).
4) INIT_ATTR & friends could be used for that, but if loading the bitmaps at runtime is an easier way out I won't disagree (there would be a need for default bitmaps compiled into the binary though, so Rockbox still is "usable" without any additional files).

I'm for the "download the correct default theme at first start"-idea, if we don't want to waste binsize by putting all possible resolutions in the APK (not sure how big that would be?).
Comment by Jonathan Gordon (jdgordon) - Saturday, 06 November 2010, 23:34 GMT
3) right, so there is still an artificial limit here, so we don't actually gain anything by making it based on the detected screen size instead of just using a static size which is excessively large

with all the cabbie files (all screen sizes and depths so more than required) and all fonts, rockbox.apk ends up at about 9MB which is too big. however having to download 5MB of fonts/themes on first run sucks just as much.

very busy week at work so unlikely to be able to touch this till Wednesday night :(
Comment by Thomas Martitz (kugel.) - Sunday, 07 November 2010, 01:22 GMT
I think we should concentrate on the dynamic screen size first. removing artificial limits should be a separate patch.
Comment by Jonathan Gordon (jdgordon) - Sunday, 07 November 2010, 12:13 GMT
I got android working :) still could be neater though.scrolling is still broken.
http://jdgordon.info/rockbox/rockbox.apk is prebuilt with a manually hacked rockbox.zip so it works. it contains most of the big fonts and all cabbie files. comes to about 7MB which is probbaly too big, we can work out how to fix that later.
Comment by Jonathan Gordon (jdgordon) - Sunday, 07 November 2010, 12:41 GMT
and this fixes the scroll engine.
Comment by Jonathan Gordon (jdgordon) - Wednesday, 09 March 2011, 13:24 GMT
bump time. I've stripped out the android changes and only enabling this for the sdl app (change the screen size with --display WIDTHxHEIGHT).

scrolling is broken but otherwise looks good (scrolling is broken on sim also)
Comment by Jonathan Gordon (jdgordon) - Wednesday, 09 March 2011, 13:38 GMT
micor cleanup of LCD_HEIGHT/WIDTH, scrolling and plugins still problematic
Comment by Jonathan Gordon (jdgordon) - Sunday, 13 March 2011, 10:03 GMT
fixed scrolling.

I'd really like to commit this in the next few days, so comments welcome. This does disable some plugins again but I really dont think it is a big deal (people that actually care about the missing plugins can disable the config option.) I'm going to look into fixing the plugins later.
Comment by Maurus Cuelenaere (mcuelenaere) - Sunday, 13 March 2011, 12:00 GMT
Is the change in apps/gui/skin_engine/skin_parser.c related to this patch?
Otherwise this looks fine (also see my comments on the mailing list).
Comment by Jonathan Gordon (jdgordon) - Sunday, 13 March 2011, 12:07 GMT
pretty sure that is an artifact from the sync.. need to double check that is still needed.
Comment by Jonathan Gordon (jdgordon) - Tuesday, 22 March 2011, 11:28 GMT
synced, android lcd is broken, no idea why though. Needs someone to take a look
Comment by Maurus Cuelenaere (mcuelenaere) - Thursday, 24 March 2011, 00:42 GMT
I quickly hacked this up, seems to work on the emulator..
Comment by Jonathan Gordon (jdgordon) - Thursday, 24 March 2011, 01:29 GMT
I cant actually look at your changes now but going by the diffstats I assume you made it work for android? there is opposition to enabling this for anything ubt sdl for now, so did you notice anything obvious why it wasnt working with HAVE_DYNAMIC_SCREEN_SIZE was not enabled?
Comment by Maurus Cuelenaere (mcuelenaere) - Thursday, 24 March 2011, 09:25 GMT
I didn't test that case, only made it work with HAVE_DYNAMIC_SCREEN_SIZE enabled. With it disabled, there shouldn't be much additional changes though, just setting lcd_framebuffer to _lcd_framebuffer and doing some s/lcd_WIDTH/LCD_WIDTH/ etc.

Will look at it later.
Comment by Jonathan Gordon (jdgordon) - Sunday, 27 March 2011, 11:19 GMT
my version causes wierdness in the android sim and outright crashes on target, your version seems to work so im inclined to look into making it compile for non-HAVE_DYNAMIC_SCREEN_SIZE and commit :)
Comment by Jonathan Gordon (jdgordon) - Wednesday, 30 March 2011, 12:14 GMT
fixed android and disabled it by default to not stire the hornets nest...
Comment by Maurus Cuelenaere (mcuelenaere) - Wednesday, 30 March 2011, 12:45 GMT
I'd move the

+#if !defined(HAVE_DYNAMIC_LCD_SIZE) && \
+ (CONFIG_PLATFORM != (PLATFORM_HOSTED|PLATFORM_ANDROID))
+ lcd_framebuffer = &_lcd_framebuffer[0][0];
+#endif

hunk to firmware/target/hosted/android/lcd-android.c

You've also left some DEBUGF statements in lcd-bitmap-common and lcd-android.c

And the "lcd_framebuffer = malloc(sizeof(fb_data) * lcd_width * lcd_height);" line in lcd-android.c can be moved into the HAVE_DYNAMIC_LCD_SIZE #ifdef (you overwrite it later on anyways).
Comment by Jonathan Gordon (jdgordon) - Wednesday, 30 March 2011, 23:42 GMT
that code you pasted is for not android... maybe its unecessary, I'll clean that up before commitign
Comment by Maurus Cuelenaere (mcuelenaere) - Thursday, 31 March 2011, 10:57 GMT
Hmm right, I misread that line.

Anyway, if the alloc'ing of the framebuffer in lcd-android.c is properly excluded on #ifdef HAVE_DYNAMIC_LCD_SIZE, there should be no need to have that special case for Android.
Comment by Jonathan Gordon (jdgordon) - Thursday, 31 March 2011, 11:04 GMT
its only needed because android mallocs the buffer regardless, but that shuold be changed to clean it up.
it looks like a bunch of other lcd drivers need fiddling because of the lcd_framebuffer arrray->pointer change which is mildly annoying... i'll get it done over the weekend hopefully
Comment by Jonathan Gordon (jdgordon) - Wednesday, 20 April 2011, 13:57 GMT
sync and make ipd video compile, but it whitescreen and data aborts on boot
Comment by Björn Stenberg (zagor) - Tuesday, 11 October 2011, 22:10 GMT
Synced against r30742.
Changed to only enable dynamic code for targets with HAVE_DYNAMIC_LCD_SIZE. Other targets are (should be) unaffected.
sdlapp (with dynamic size) builds and runs. sansac200, ipodvideo and archosplayer (without dynamic) all build, haven't run them yet.
A few malloc calls still remain.
Comment by Björn Stenberg (zagor) - Saturday, 15 October 2011, 22:37 GMT
This patch (against r30759) is tested and runs fine on sdlapp, clip and c200.

Loading...