Rockbox

  • Status Closed
  • Percent Complete
    100%
  • Task Type Patches
  • Category LCD
  • Assigned To No-one
  • Operating System All players
  • Severity Low
  • Priority Very Low
  • Reported Version Daily build (which?)
  • Due in Version Undecided
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by idak - 2008-10-05
Last edited by Unhelpful - 2008-12-26

FS#9458 - resize bitmap on load with smooth scaling

This patch can resize bitmap on load with smooth scaling.
This is quick and dirty hack,
but I think this is good start point to do this…

The task blocks these from closing
ID Project Summary Priority Severity Assigned To Progress
9665 Rockbox  FS#9665 - pictureflow resize-on-load  Very Low Low
100%
9683 Rockbox  FS#9683 - pluggable scaler output for greylib  Very Low Low
100%
9689 Rockbox  FS#9689 - greylib pictureflow for greyscale targets  Very Low Low
100%
Closed by  Unhelpful
2008-12-26 08:02
Reason for closing:  Accepted
Additional comments about closing:   Warning: Undefined array key "typography" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 371 Warning: Undefined array key "camelcase" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 407

Committed as r19592.

Project Manager

Some remarks:

- Comments in the code explaining what things do

- There are many > 80 columns lines

- There are // comments

- There are some #if 0 lines

In general I think it looks a fine effort to really add this feature “the right way”!

With this added, would it be possible to remove the current load bmp function? It seems like a waste to have two…

Akio,

You describe this as “a good start”. Are you continuing to work on it? What improvements are you planning?

As Daniel said, my first reaction was also that we probably won’t want two read_bmp functions.

My other reaction was the static “cache” buffer. I haven’t looked at the code in detail, but am wondering if it would make sense to have three “unsigned char[]” arrays (one each for R, G and B), rather than a single 32-bit buffer. This would cut the size of the array by 25%.

But I guess that’s what we’re looking for in general - optimisations to try and both simplify the code, and reduce the memory requirements (for both code and cached line data).

Dave.

idak commented on 2008-10-09 16:08

Thank you for some comments.
# Yes, old one is my first quick hack.

First, I merged those two functions into one.

idak commented on 2008-10-18 15:53

Finally, I add 2bpp-LCDs resize function.
I think smooth scaling algorithm is overkill for 2bpp-LCDs.
So I use simple scaling algorithm (nearest neighbour).

That’s all I’m planning…

idak commented on 2008-10-25 15:02

1) separate resize algorithm into another file.
2) use three “unsigned char[]” arrays (one each for R, G and B)
instead of a single 32-bit buffer.
#but I use a single 32-bit buffer to read one line…

Firstly, I think the patch is looking good - separating the resize functionality into other files is a good idea.

However, I also think we need to make this patch as efficient as possible before it can be committed. I’m also thinking that maybe it needs to be committed in smaller parts, to make it easier to review (and to justify all the changes).

I’ve just tried compiling this patch for the ipod color - it adds 5024 bytes to the binary size (i.e. code and initialised data), and a total
of 17056 bytes of RAM usage. (taking the numbers from the rockbox-info.txt file created with current SVN and SVN with this patch).

Looking at the code, I can see the following buffers:

uint32_t bmpbuf[MAX_WIDTH] - this used to be on the stack (and hence in IRAM - meaning faster access on some targets). IIUC, MAX_WIDTH is LCD_WIDTH for mono targets, and MAX(500,LCD_WIDTH) for other targets. This means a maximum of 2000 bytes for most targets.

You also have 5 cache lines, with each of those lines being an array of MAX_LINE 3-byte structs, plus a 16-bit y value - meaning 5 * (3 *
MAX_WIDTH + 2) bytes = a maximum of 7510 bytes.

Adding these up, I get 2000+7510+5024 = 14534, so I’m not sure where the other 2522 bytes have gone….

A MAX_WIDTH of 500 seems too small to me - I would expect people to want to use larger bitmaps, and if we support resizing, I think we should try and support any bmp size. I also have some album art which is just slightly more than 500 pixels (501 or 502). So I would want 512 as a minimum. My thoughts are that this should be 2*LCD_WIDTH, and then for any images larger than that, simply skip pixels in the horizontal direction when reading a line in order to reduce the size to LCD_WIDTH*2. This obviously isn’t ideal, but I think that a non-optimal resize for very large images is better than simply rejecting them.

Also, why is the value 5 chosen for the cache size? The comment in the code says “must be > 2”, so why isn’t it 3?

A general comment on the code - I see that you’re using “short” in a lot of places This often has the effect of increasing memory usage, rather than decreasing it - especially when these variables are local (i.e. allocated on the stack). Whereever possible, just use “int”, as by definition, that’s the easiest integer type for the target CPU to deal with.

Looking at just the changes to bmp.c, a major change appears to be to change the format of the bitfields[] array. This seems to be adding a lot of extra code - are there savings to be made elsewhere by changing this format? If not, then I would leave it as it was.

I would like to see this committed, and my approach would be to commit in smaller parts as follows:

1) Refactor bmp.c to incorporate all the changes required for resizing, but without actually adding the resizing itself. This patch should have negligible impact on binary size and memory usage, and the resizing patch itself should then only make minimal changes to this file.

2) Add the resizing feature, but without using it in the core - so we can see the impact of that feature by itself, and if possible, find optimisations.

3) Finally, add the code which uses the new resize feature for album-art (and elsewhere, if applicable).

idak commented on 2008-10-28 16:19
Warning: Undefined array key "sepchar" in /home/rockbox/flyspray/plugins/dokuwiki/inc/pageutils.php on line 84 Warning: Undefined array key "useslash" in /home/rockbox/flyspray/plugins/dokuwiki/inc/pageutils.php on line 93 Unhandled exception: strtr(): Argument #2 ($from) must be of type array, string given
This should never happen, please inform Flyspray Developers.

If you are an Administrator of this Flyspray installation you might enable temporarly! DEBUG_EXCEPTION in constants.inc.php for more details.