Rockbox

Tasklist

FS#10535 - Simple BMP Viewer

Attached to Project: Rockbox
Opened by Michael Ignatiev (megamike) - Monday, 17 August 2009, 15:13 GMT
Last edited by Teruaki Kawashima (teru) - Thursday, 18 February 2010, 15:27 GMT
Task Type Patches
Category Plugins
Status Closed
Assigned To No-one
Operating System Sansa e200
Severity Low
Priority Normal
Reported Version Version 3.3
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

This plugin is standart simple BMP viewer.
This patch enable view BMP-files on Rockbox as know file type
You have put bmpview.c and bmpview.h in apps/plugins/ directory
This task depends upon

Closed by  Teruaki Kawashima (teru)
Thursday, 18 February 2010, 15:27 GMT
Reason for closing:  Accepted
Additional comments about closing:  in r24754.
Comment by Michael Ignatiev (megamike) - Monday, 17 August 2009, 15:32 GMT
Oops, there is the files of my patch
Comment by Gman (Thecoolgman) - Monday, 17 August 2009, 23:26 GMT
I had a feeling this was going to happen after the png viewer, good thing though. I'm glad I don't have to open them with rockpaint.
Comment by Maurus Cuelenaere (mcuelenaere) - Monday, 17 August 2009, 23:54 GMT
There's really no need for the plugin_name buffer, just replace 'plugin_load(plugin_name,buf)' with 'plugin_load(PLUGIN_DIR "/viewers/bmpview.rock", buf)' and it should work.
Comment by Michael Ignatiev (megamike) - Tuesday, 18 August 2009, 03:02 GMT
Ok Maurus, here is edited version :
Comment by Michael Ignatiev (megamike) - Tuesday, 18 August 2009, 17:07 GMT
Here is new version with fixed some bugs...
Comment by Maurus Cuelenaere (mcuelenaere) - Tuesday, 18 August 2009, 17:43 GMT
Could you please include bmpview.{c,h} in your patch?

If you don't know how to add a file to svn: svn add <file>
Comment by Gman (Thecoolgman) - Tuesday, 18 August 2009, 21:49 GMT
Dose this work for the iPod 5G? because it claims I need bmpview.o
Comment by Michael Ignatiev (megamike) - Wednesday, 19 August 2009, 07:49 GMT
Of course, Maurus, I included bmpview.c and bmpview.h in patch file.
Here it is:
Comment by Michael Ignatiev (megamike) - Wednesday, 19 August 2009, 07:57 GMT
Gman, I'm not sure about iPod 5G, but suppose it must.
Try last version of my patch - v.1.1.
Comment by Michael Ignatiev (megamike) - Wednesday, 19 August 2009, 08:51 GMT
Gman, I'm not sure about iPod 5G, but suppose it must.
Try last version of my patch - v.1.1.
Comment by Maurus Cuelenaere (mcuelenaere) - Wednesday, 19 August 2009, 11:51 GMT
I see you only added credits to bmpview.h, but you didn't use your full name.

Rockbox policy requires you to put your full name there though, see http://svn.rockbox.org/viewvc.cgi/trunk/docs/CONTRIBUTING?view=raw .

Other than that, I don't see anything wrong (haven't actually tested your patch though).

edit:
small nitpick: you used // comments instead of /* */ comments in some places
Comment by Nils Wallménius (nls) - Thursday, 20 August 2009, 10:45 GMT
Hi, the plugin looks nice but i have a few comments
Please move the contents of the .h file into the .c file to be more in line with the other single file plugins.
There are some very long lines i your code, our guidelines state a maximum of ~80 chars.
It doesn't compile for ipod video for example as it has no BUTTON_POWER, you can look at other plugins to see how they have managed keymaps for different targets.
The hunk in filetree.c should not be needed, instead please add a line for bmpview to viewers.config (I see that as it is now this will not work but i think dropping bmp from the builtin filetypes is better than this special case.
Quite a few images triggered a " Bitmap too large for buffer" ...
Please do not include commented out lines of code unless they have some special meaning or are otherwise motivated.

Some suggestions too, while i'm at it.
Clearing the backdrop and using a plain coloured background would look much better i think.
Centering small images would also look nicer on large displays.
Comment by Michael Ignatiev (megamike) - Thursday, 20 August 2009, 17:51 GMT
Hi, Nils. Thank you for your attention. Your suggestions are very useful, and I will embody them in the next version necessarily.
Comment by Michael Ignatiev (megamike) - Saturday, 22 August 2009, 06:16 GMT
Hi everybody!
Here is new version of my bmp_viewer:
Comment by Michael Ignatiev (megamike) - Sunday, 23 August 2009, 17:42 GMT
In this version :
1. Centering of small images
2. Full LCD-size of resized big images
Comment by Maurus Cuelenaere (mcuelenaere) - Sunday, 23 August 2009, 18:01 GMT
I see you do a rb->plugin_get_audio_buffer() at plugin_start(), would it be possible to first try with a rb->plugin_get_buffer() call and if buf_size is too small, then try rb->plugin_get_audio_buffer()?
That way audio doesn't get paused when you view (small) BMP images.

Also I spotted some typos like ", Could not load %s" -> "Could not load %s", "Error: No parametrs."->"Error: No parameters.", Impementation -> Implementation and cb_progess -> cb_progress.
Comment by Gman (Thecoolgman) - Wednesday, 26 August 2009, 21:59 GMT
Glad this works again, I test it out on my 5G. It works alright but it still needs a lot of work. It also needs more of a jpeg viewer like surface.
Comment by Michael Ignatiev (megamike) - Friday, 04 September 2009, 16:32 GMT
Hi everybody!
I have tried to consider all your remarks as much as possible and even have added a 2x zoom.
It is the following version of my plugin:
Comment by Teruaki Kawashima (teru) - Friday, 18 December 2009, 16:08 GMT
Rewirte plugin, most part of code is from jpeg. would have almost same UI as jpeg viewer.
I haven't tested much.
Comment by Johannes Schwarz (Ubuntuxer) - Friday, 18 December 2009, 18:02 GMT
It works fine for me. But the menu shouldn't be called Jpeg Menu and I noticed that I can't zoom in big images more than one time.

I would like to call your attention to the task  FS#6321 , which has got the purpose to merge the image viewers.
The advantage would be that the user can make a slideshow about different image formats and we avoid dublicate code.
Comment by Teruaki Kawashima (teru) - Saturday, 19 December 2009, 04:14 GMT
I forgat to change title of menu, thanks. fixed.

For the second issue, it is intended behaviour that you can't zoom to actual size of image for too big image.
Such image is resized to the size to fit memory while loading image. other wise you can't view the image at all.
If you stop audio before use bmpviewer, you will be able to veiw the image in larger size.
If you are sure that the image can be opened without resizing, please report detail, the size of image (width, height, and maybe depth) and target/simulator you use.

you know that merging these plugins to one plugin cannot be done easily because available size of memory is limited.
Comment by Teruaki Kawashima (teru) - Sunday, 20 December 2009, 12:58 GMT
Update patch.
Now this would also work on mono target, at least on simulator. not tested on a real device. but i don't actualy know how it works...
Comment by Johannes Schwarz (Ubuntuxer) - Sunday, 20 December 2009, 13:56 GMT
>For the second issue, it is intended behaviour that you can't zoom to actual size of image for too big image.
>Such image is resized to the size to fit memory while loading image. other wise you can't view the image at all.
Ok thanks for the information. I'm quite impressed with bmpviewer, it displays an image which has a size of 2592x1944 and 14.4MB. (Fuze, Simulator)

>you know that merging these plugins to one plugin cannot be done easily because available size of memory is limited.
I saw that you also wrote patches for the other image viewers, so I just wanted to draw your attention to it.
Comment by Teruaki Kawashima (teru) - Monday, 18 January 2010, 15:01 GMT
resync patch.
* use imageviewer.
Comment by Teruaki Kawashima (teru) - Saturday, 13 February 2010, 04:45 GMT
resync.

Loading...