Rockbox

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 :) )

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

   dynamic-screen-size.diff (36.5 KiB)

this is my crude attempt to fix android (apply to above patch)…. it segfaults after init and I don;t know where/why

   android-changes.diff (9.3 KiB)

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.

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

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.

Aren’t the images in the wps’ subfolder also screen size dependent? I.e. the 480×800 cabbie has larger icons (and a backdrop) in the wps/cabbiev2/ folder than the 320×480 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?

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

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?).

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 :(

I think we should concentrate on the dynamic screen size first. removing artificial limits should be a separate patch.

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.

   dynamic-screen-size.diff (41.9 KiB)

and this fixes the scroll engine.

   dynamic-screen-size.diff (41.9 KiB)

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)

   dynamic_lcd_size.patch (35.8 KiB)

micor cleanup of LCD_HEIGHT/WIDTH, scrolling and plugins still problematic

   dynamic_lcd_size.patch (35.1 KiB)

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.

   dynamic_lcd_size.patch (36 KiB)

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).

pretty sure that is an artifact from the sync.. need to double check that is still needed.

synced, android lcd is broken, no idea why though. Needs someone to take a look

   dynamic_lcd_size.patch (38.2 KiB)

I quickly hacked this up, seems to work on the emulator..

   dynamic_lcd_android_hack.diff (43.5 KiB)

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?

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.

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 :)

fixed android and disabled it by default to not stire the hornets nest…

   dynamic_lcd_size.diff (46 KiB)

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).

that code you pasted is for not android… maybe its unecessary, I’ll clean that up before commitign

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.

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

sync and make ipd video compile, but it whitescreen and data aborts on boot

   changes.diff (45.3 KiB)

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.

   dynamic.patch (41.7 KiB)

This patch (against r30759) is tested and runs fine on sdlapp, clip and c200.

   dynamic2.patch (43.2 KiB)