Rockbox

Tasklist

FS#9493 - Add a png decoder to rockbox

Attached to Project: Rockbox
Opened by Christophe Gouiran (BeChris) - Wednesday, 15 October 2008, 22:05 GMT
Last edited by Frank Gevaerts (fg) - Saturday, 25 July 2009, 21:39 GMT
Task Type Patches
Category Applications
Status Closed
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 100%
Votes 0
Private No

Details

Hi, here's a work in progress png decoder for rockbox.

I still have some difficulties to do the conversion from 32 bit unsigned char data given by the decoder to the rockbox pixel format.

If anyone want to help, it'd be greatly appreciated.

   diff.txt (74.3 KiB)
This task depends upon

Closed by  Frank Gevaerts (fg)
Saturday, 25 July 2009, 21:39 GMT
Reason for closing:  Accepted
Additional comments about closing:  Committed (version 8) as r22037
Comment by Paul Louden (Llorean) - Wednesday, 15 October 2008, 23:14 GMT
Why not add it to the pluginlib, and maybe make the jpegviewer a generic image viewer?
Comment by MichaelGiacomelli (saratoga) - Thursday, 16 October 2008, 23:24 GMT
Reading the IRC logs, it was suggested that this task should wait for JPEG inclusion. I disagree.

We should decide if we want PNG support in core a priori, and either reject or accept the feature independent of the JPEG decoder. I tentatively support adding this (I think the feature is worth having), but want to see what the memory hit looks like. The impression I have is that PNG is reasonably complicated compared to JPEG. The format may not be a good fit for rockbox if it cannot be efficiently implemented.
Comment by Peter D'Hoye (petur) - Saturday, 18 October 2008, 22:05 GMT
anyway, if the jpeg viewer is to become a generic image viewer, this code should become a lib, and the viewer code split up..... It would be a good idea to extract the jpeg decoding from the image viewer, maybe the image decoding can become a core plugin just like the codecs?
Comment by Dave Chapman (linuxstb) - Monday, 20 October 2008, 07:52 GMT
Looking at this patch, the 640KB static buffer jumps out and punches me in the face. Do you know how this value (MAX_SCANLINES) was derived?

I think a first step needs to be to investigate the memory usage of this decoder, and see how far it can be reduced. I don't know anything about the PNG format, so don't know the answer to that question.

To pack data, you can use the LCD_RGBPACK(r,g,b) macro - where r, g and b are 8-bit values. There is no alpha support in Rockbox. See firmware/export/lcd.h

Comment by Christophe Gouiran (BeChris) - Tuesday, 21 October 2008, 06:24 GMT
This MAX_SCANLINES was added by me and is used to only allow decoding of image up to 400x400.
For the moment, I had to use 2 static buffers:
- unsigned char idat[MAX_IDAT_SIZE] (MAX_IDENT_SIZE being set to 300000) : this is the buffer in which content of png IDAT chuncks is written.
- unsigned char scanlines[MAX_SCANLINES] : a placeholder to decompress the zlib stream (which was written into idat just before) which is embedded in all png files.

With a friend we are currently trying to eliminate both buffers and thus reduce the momeory footprint of the decoder.

Comment by Taylore (trailblaze) - Sunday, 26 October 2008, 01:01 GMT
I'm not sure what this patch does? I patched rockbox with the patch and i don't see any changes.. Does this patch make it possible to look at .png pics on da ipod?
Comment by Christophe Gouiran (BeChris) - Wednesday, 29 October 2008, 10:23 GMT
Not for the moment, this was only a proof of concept for people who may have been interrested.

I will rework it as a plugin.
Comment by Gman (Thecoolgman) - Tuesday, 31 March 2009, 06:12 GMT
Any update? Yes? No?
Comment by Christophe Gouiran (BeChris) - Tuesday, 16 June 2009, 22:05 GMT
Hello everybody, after several month far from coding on my computer, I'm now glad to announce that I suceed in creating a working version of my png viewer as a plugin.
Current limitations:
- Worked OK on my Sansa E200. Should only work on target with a 16bit LCD.
- The plugin uses the audio buffer. Therefore it will stop plaback when viewing a png file.
- No resizing (someone interrested in giving help ?).
- Lacks some tests on access out of memory buffer boundaries.

How to use it:
Select a png file.

Messages like that should appear:
Available memory : 32866712
blackhole.png:
loading 692 bytes
decoding image
Color conversion requested
Decoder OK

Then, you have to press right button to display the image during 2 seconds.
Any other button will make the plugin exit whithout displaying the image.

Enjoy this first version !

I'm waiting for your feedback.
Thanks in advance.
Comment by Gman (Thecoolgman) - Wednesday, 17 June 2009, 05:57 GMT
Cool, I'll try this out. Thanks for Rescripting!
Comment by Gman (Thecoolgman) - Wednesday, 17 June 2009, 07:40 GMT
I tried this on my 5th Gen iPod. Here are the results:

It compiles with Cygwin, but I got quite a few errors (see "png compiling.png")
But it works great. Some Features I request, Zoom In/Out, Added a custom icon (Or you could just use the jpeg or bmp icon), Layout Like JPEG Viewer, Rotate possibly? (Maybe use: FS#9497) Tomorrow I may try this on my Nano.

Nice Patch.

Comment by Gman (Thecoolgman) - Wednesday, 17 June 2009, 07:43 GMT
Opps silly me. I meant to say "Add" instead of "Added".
Comment by Christophe Gouiran (BeChris) - Friday, 19 June 2009, 22:23 GMT
Hello, I made some improvements:
- No more annoying compilation warnings.
- Better protection against memory out of bounds accesses.
- Behaviour exactly like JPEG plugin (same button layout, slideshow, progress while decoding, menu) except see below.

Current limitations:
- Resizing not yet implemented :( => Maybe I'll have a look at bilinear interpolation.
- It seems decoding some files when a tune is plaing led to corrupted display.

Apply this second patch on a fresh rockbox source tree i.e rollback previous patch before.

Enjoy.

Any comments ?

Comment by MichaelGiacomelli (saratoga) - Friday, 19 June 2009, 22:38 GMT
> Resizing not yet implemented :( => Maybe I'll have a look at bilinear interpolation.

I presume you're aware of smooth_resize_bitmap in the plugin lib?
Comment by Gman (Thecoolgman) - Saturday, 20 June 2009, 00:54 GMT
Nice! If this keeps up It will have to be commited.
Comment by Christophe Gouiran (BeChris) - Sunday, 21 June 2009, 21:49 GMT
I'm definitely not able to use resizing (thanks saratoga for smooth_resize_bitmap hint).
My problem is, if I well understood, that output of png decoder is an unsigned char buffer (pixel packed as RGB).
But to use the resizing algorithm, I must convert it to fb_data buffer which I'm not able to do (I tried several approch, I always get scrambled display content).
This is the same problem if I want to include in the core a png loader : I'm not able to produce a valid stuct bitmap (especially the data field) : all is scrambled :(
I'm seriously considering to ask help on IRC.

A final path altough because the former one accidentaly impacted unrelated files:
- jpeg.c (only inserted blank caracters).
- SOURCES (lua plugin was no more compiled)

And it add the possibility to cancel the decoding of a png image by pressing the MENU button.
Comment by Gman (Thecoolgman) - Sunday, 21 June 2009, 22:09 GMT
Oh thanks for that. I had to PNGs in one folder and and couldn't read them and it kept going on and on trying to read them.
Comment by Christophe Gouiran (BeChris) - Sunday, 21 June 2009, 22:18 GMT
Gman: when you cancel the decoding of a file it will skip to the next one (except, if there is only one file in the folder, the plugin will terminate).
So it won't solve your problem.
I will consider to terminate the plugin when a decoding is aborted regardless of the number of files in the folder.

You can get this behaviour by changing line:
if (decoder.error == PLUGIN_ABORT && entries == 1) {
By:
if (decoder.error == PLUGIN_ABORT) {

Thanks for testing and feedback.
Comment by Gman (Thecoolgman) - Sunday, 21 June 2009, 23:35 GMT
Okay will do.
Comment by Christophe Gouiran (BeChris) - Tuesday, 23 June 2009, 22:12 GMT
Yeeeeessss ! It works
I finally got the resizing to work !
After having spent a lot of time tracking a crash on the target (It worked OK in the simulator) I noticed I forgot to align some buffers.

I'm really proud of this plugin :)

What I have to do now is to clean it up a bit (remove encoder part, ...) and check whether all king of png files are loaded OK.

Try it and give me your feedback.

(I have incorporated modification above : in case of MENU key pressed while a file is decoded, the plugin will exits)
Comment by Gman (Thecoolgman) - Wednesday, 24 June 2009, 19:30 GMT
Yay! Im going to try this right now.
Comment by Christophe Gouiran (BeChris) - Thursday, 25 June 2009, 23:00 GMT
Hi, another major update.
Definitively this plugin is only for player which have a 16bit LCD, sorry no gray display.

Changelog:
- All merged in one file (png.c) + removed unused encoder part.
- Better protection against memory access fault (I hope I found every bugs forgotten before).
- Some image type were not correctly displayed.
- Drastically reduced needed memory to decode/display image.
- Better error messages.
- This plugin displays correctly all images of the PngSuite test suite (http://www.schaik.com/pngsuite/).

I think we are very close to a stable plugin now.

Please let me now if you find a file which crashes the plugin or which is not displayed as expected.
Comment by Gman (Thecoolgman) - Friday, 26 June 2009, 07:52 GMT
Will this also fix the bug on images bigger than 1000 in width/length? It loads them but nothing happens.
Comment by Gman (Thecoolgman) - Friday, 26 June 2009, 11:12 GMT
Hey every thing works good! Every thing loads, finally! I think you just need to fix the time to load an image it needs to be quicker but everything is good!
Comment by Johannes Schwarz (Ubuntuxer) - Saturday, 27 June 2009, 19:07 GMT
Hi, great work. I'm looking forward to seeing your png viewer in rockbox.
I tested your patch on a e200 and it works fine.
But please avoid the following compiler warnings:
CC apps/plugins/png.c
/home/johannes/Rockbox/rockbox/apps/plugins/png.c: In function ‘LodePNG_decode’:
/home/johannes/Rockbox/rockbox/apps/plugins/png.c:2185: warning: cast from pointer to integer of different size
/home/johannes/Rockbox/rockbox/apps/plugins/png.c:2185: warning: cast to pointer from integer of different size
/home/johannes/Rockbox/rockbox/apps/plugins/png.c: In function ‘get_image’:
/home/johannes/Rockbox/rockbox/apps/plugins/png.c:2722: warning: cast from pointer to integer of different size
/home/johannes/Rockbox/rockbox/apps/plugins/png.c:2722: warning: cast to pointer from integer of different size
LD png.rock
Comment by Christophe Gouiran (BeChris) - Saturday, 27 June 2009, 22:50 GMT
Hi, another update.

Changelog:
- Moved the plugin to its own directory (apps/plugins/png).
- A bit of optimizations on Huffman tree generation (altough rare impact).
- In case of "File too big" error message, the plugin could sometimes loop indefinitely.
- Propose, as made by the JPEG decoder, to stop playback in case of lack of memory.
- Added -O3 for compilation of this plugin : it gives unbelievable performance gain !

The last point is very important and gives incredible results.
Times given are times taken to decode the image, not the last resizing step:

For a 1536x2048 png file:
With -O CFLAGS (the default) : 50.85s
With -O2 : 37.58s
With -O3 : 22.75s

With a 1680x1050 png file:
With -O : 15.95s
With -O2 : 11.19s
With -O3 : 7.8s

Even if -03 adds 2Kb to the size of the .rock file, it's worth enabling it !

And Ubuntuxer : I don't get any compilation warnings here, try again with this patch on a fresh rockbox source tree.

As usual, comments ? suggestions ?
Comment by Gman (Thecoolgman) - Sunday, 28 June 2009, 02:07 GMT
Hmm this doesn't compile.
Comment by Maurus Cuelenaere (mcuelenaere) - Sunday, 28 June 2009, 10:31 GMT
This patch does some unrelated svn:ignore changes, please remove those.
Comment by Gman (Thecoolgman) - Sunday, 28 June 2009, 10:33 GMT
Could you do that he has gone on vacation till monday.
Comment by Christophe Gouiran (BeChris) - Sunday, 28 June 2009, 19:55 GMT
Hi, for compilation problem did you rollback previous patch and applied the latest on a fresh rockbox source tree ?
Indeed, in patch_png6.txt, png plugin has been moved in its own directory.

And, could you tell me:
- For which platform you are compiling (platform number as given when invoking configure script).
- An extract of the error you get.

Thanks.
Comment by Gman (Thecoolgman) - Sunday, 28 June 2009, 23:41 GMT
Okay so I finally got this to compile and I must say that this is the best version yet. It is so fast and thanks for fixing the memory issue. I think you should send this over to the RockBox Admins to see if they want to commit this.
Comment by Jonas Häggqvist (rasher) - Sunday, 28 June 2009, 23:50 GMT
I think it would be preferable if this plugin and the jpeg viewer could be merged, to achieve a uniform interface and featureset.

An alternative solution could be to use the same interface code for both, and build two plugins from that code, which then include either the jpeg decoder or the png decoder. That way, the interface is the same, and you don't have to maintain two sets of interface code. The advantage is that the plugin wouldn't be twice the size it is now (which is a concern with regards to memory usage).

Discussed in IRC: http://www.rockbox.org/irc/log-20090629#01:49:00
Comment by Christophe Gouiran (BeChris) - Monday, 29 June 2009, 04:07 GMT
Hi, many thanks for those positive comments.
I'd like to do even more optimization before asking for inclusion if I can figure out how Rockbox profiling is working.
When activating it, all the plugins are freezing on my Sansa e260v1.
It seems to come from the -finstrument-functions gcc flag.

Someone out there could have a explanation on it ?
What a pity if I can't use the rockbox profiling capabilities !
Comment by Maurus Cuelenaere (mcuelenaere) - Monday, 29 June 2009, 08:18 GMT
Have you tried doing profiling in the simulator? If that still crashes, you could at least use GDB to analyse the crash..
Comment by Christophe Gouiran (BeChris) - Wednesday, 01 July 2009, 11:44 GMT
Hello, I didn't try profiling under the simulator but put several time measure and found where it would be worth an effort of optimisation.
I already succeed in getting up to 10% performance gain for some image in the scanline unfiltering.
I'm now looking at some other possible coding tricks but don't expect too much gain.

Stay tuned, I'll keep you informed.
Comment by Dave Hooper (stripwax) - Friday, 03 July 2009, 00:02 GMT
BeChris - you can read the document in doc/TECH which has a section on profiling right at the end.
Quick summary: you need to
1. ensure the makefile of the thing you're compiling has -DPROFILE_OPTS in the CCFLAGS somewhere (or you can just add it in the call if it's not there)
2. put a call to profile_thread() at the start of the thing you want to profile, and a call to profstop() at the end of the thing you want to profile (e.g. the start and end of your main plugin entry function)
3. create a new build directory, and using ../tools/configure set up the Advanced build options to add Profiling
Comment by Christophe Gouiran (BeChris) - Friday, 03 July 2009, 22:30 GMT
Hello, I denitively can't do profiling.
When enabled, my plugin randomly crashes and I get strange behaviours.

However, I worked on optimisations and got impressive results.

Changelog:
- A bit of optimizations on scanlines unfiltering (can give up to 10% on certain images).
- Now available for all LCD color bitmap capable players.
- Took inflate algorithm from origianl zlib distribution (image decoding got a boost of 40-50% !).

I only need to taylor a bit error messages which must take into account error coming from the zlib inflate algorithm.
Comment by Christophe Gouiran (BeChris) - Wednesday, 15 July 2009, 21:32 GMT
Hello, a small update of previous patch because I forgot png.h in patch_png7.txt.
Now, errors coming from the inflate decompression are correctly displayed.

Give a try at patch_png8.txt (as usual : apply on a fresh source tree).

Benchmark on same files as for patch_png6.txt:
For a 1536x2048 png file: 11.23s
For a 1680x1050 png file: 4.60s

It goes almost twice faster !

For your information, I also worked on the adaptation of the official libpng to see whether I could get even more optimisation from it.
For that, I used dbestfit memory (dis)allocation routines from Daniel Stenberg.

And I was very surprised by the results and was very disappointed to noticed that it was a lot slower than the hybrid implementation LodePng/zlib introduced in patch_png7.

For those interrested, you can find it in patch_png9.txt

Comment by Nils Wallménius (nls) - Friday, 24 July 2009, 10:04 GMT
I would really like to se this merged with the jpeg viewer into an image viewer.
Do you think that is feasible ad if so do you think it would be easier to do after a merge into the rockbox svn or before?
Comment by Christophe Gouiran (BeChris) - Friday, 24 July 2009, 12:43 GMT
Hi Nils, I would prefer a merge in the svn before working on the unification.
It would be an honor for me to see this patch officially integrated but who decides this ?
Comment by Gman (Thecoolgman) - Saturday, 25 July 2009, 00:35 GMT
Someone like jdgordon

Loading...