Rockbox

Attached to Project: Rockbox
Opened by nicolas_p - 2006-02-19
Last edited by nicolas_p - 2007-11-11

FS#3045 - Album art display on WPS

This patch allows displaying an album art bitmap on the WPS.

For more information, please check the wiki page I’ve created : http://www.rockbox.org/twiki/bin/view/Main/AlbumArt It is more up-to-date and much easier to read than what used to be here.

The task blocks this from closing
ID Project Summary Priority Severity Assigned To Progress
4772 Rockbox  FS#4772 - Show "fullscreen" cover   Very Low Low
100%
Closed by  nicolas_p
2007-11-11 12:45
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 in a MoB version.

bugs in v1 :
- often (always ?) crashes my h300 on file change (but works
rather good in sim)
- when changing wps after displaying one with album art, the
area where the art was is all garbage

v1.1 uploaded :
a few changes but the bugs are still there
- use of strrchr
- no increment of data→img_buf_ptr after loading album art
- the loading bitmap isn’t used anymore (but the format
remains the same for now)
- enabled a debugging message which seems to help with the
crashing issues

v1.2
- added some checks to prevent unnecessary image loading
- removed debug messages

This version works quite good on sim but crashes when using
prev/next (loading from the browser doesn’t crash). On
target it remains VERY unstable.

v2
- corrected crashes by adding one more check to prevent
unexpected behaviour

This version is much more (maybe even completly) stable,
both on target (only h300 tested) and sim (there again, only
h300).
Feel free to test :)

Anonymous Submitter commented on 2006-02-21 09:03

Is it possible to resize a bitmap on the fly to a given size?
And to use the ID3v2 picture tags instead of a file?

Although they are all either in JPEG or PNG format…

Could you at least prepare your code for different formats
and differen loading locations (from file or from
metainformations)?
That would be really cool!

About file loading and file names:

I suggest the following behaviour:
 - First look for a file with same name as the songs file

name but with .bmp as extension (e.g. “T01_Pink
Floyd_Money.bmp”)

  1. if that cannot be found and an album tag is available

look for a file which has the same name as the album (e.g.
“dark side of the moon.bmp”

  1. if that cannot be found look for a file named “cover.bmp”
  2. if that still cannot be found use the filenames given

in WPS tag (is this really necessary?)

If you like my ideas or not:

Keep on going with your good work :)

v3

Changed the system used to store the album art bitmap. It is
now loaded at the same time as the track’s metadata. This
prevents additional disk-spinning and makes future ID3 album
art support possible (i’m not planning on implementing it
myself however).

There aren’t any noticeable improvements at the moment and
thre even are some issues left, so i suggest you stick to v2
if you don’t intend to help me test this patch.

Concerning the “please add support for this and that” requests, i’ll do my best to create the most flexible system
possible, in concertation with the devs (if they plan on
commiting). However some things are just impossible right
now, like supporting image formats other than BMP or scaling
the images.
The pictures being small, storing in bitmap format doesn’t
consume much space so don’t worry for your disk space. Also
there are programs that allow you to convert/resize/rename
all your covers at once, so there is no need to do it all by
hand.

v3.1
- Simplified a lot of things by removing both ‘no art’ and
‘loading’ bitmaps, which were unnecessary. This causes the
format to change : it is now simply %C|x|y|
- A few minor improvements

Problems that need to be solved :
- Currently the size is hard-coded to 75×75 and anything
else will probably cause crashes, even though i haven’t
tested. This is because i don’t know how to get the real
size of the bmp.
- There are problems with WPS changing : when loading a WPS
with album art after one without, the art for the currently
playng track isn’t loaded. When going bacj to a no-art WPS,
the art continues to be displayed, despite the check.

v3.2

Improvements on size : now all bitmaps should be displayed
correctly, but currently size is hard-limited to 100×100 because of an arbitrary buffer size choice. This has to be
discussed with the devs.
Also a maximum size can be specified in the WPS (if not, it
defaults to the LCD size, but the 100×100 limit is still
there). The new WPS tag format is :
%C|x|y|[maxwidth|maxheight|]
(what’s inside the brackets is optional)

Also the WPS changing issues are solved.

Next thing i will look at is different bitmap names, as i
can see cover.bmp isn’t enough for some ;-)

v3.3
- Added support for file-specific album art : if a bitmap is
found with the same name as the music file (only differring
by the extension), it will be loaded instead of the normal
“cover.bmp” - Added centering of the album art in the bounding box. If
the bitmap is smaller than the max size, it will be
centered. If no max size is given in the WPS, the bitmap
will be drawn normally at (x,y).

mmohr commented on 2006-02-25 00:18

Really nice - it grows and grows :-)

I made a small addition to your patch:
- now it searches first for a bitmap with same name

 as the musicfile (as in your v3.3) and then
 it looks for a bitmap with same name as the 
 album of the music file - if this contains 
 ID-tags for the album.

With that addition it’s possible to use one bitmap
file for all music files of one album and still have
more than one bitmap file in one directory - for
different albums of course :-)

I also did some small changes to your code to
optimize it a bit or where I thought it makes
sense to add some more checks ;)

I added the diff to todays CVS (only metadata.c
has been changed by me) so you can have a look at it.
It’s a complete replacement to your v3.3 patch

Massa, thanks for your modification, it’s perfect ;-) Just one thing i had to change to allow using bitmaps being used with files playing from /
Also i had to clean the patch because it contained code from something else.

v3.4 :
- made the bitmap buffer bigger, it now accepts pictures up to 125×125 pixels
changes by Massa :
- added some checks and cleaned the code (and even fixed the compiler warning :-D )
- possibility to display a bitmap named <album>.bmp, where <album> is read from ID3

Quick reminder of file priority :
1. <filename>.bmp
2. <album>.bmp
3. cover.bmp

mmohr commented on 2006-02-26 09:55

Ooops - sorry for that remaining stuff - it’s from another path ;)
(which is also really cool; support for user defined ID3 tags)

BTW, how do you calculate the memory which is needed for a certain
picture size? Are all BMPs based on 8-bit colors?

when defining the bitmap buffer :
fb_data albumart_data[15625];
here, 15625 = 125*125
We just define the number of pixels we want to be able to store…

The bitmaps used will most likely be 24-bit (8-bit per color), but the WPS system can display other formats too.
I don’t know the influence of the format on the maximum size, but i presume there isn’t any.

mmohr commented on 2006-02-26 22:49

Regarding memory sizes for the bitmap:
Here’s what I found out by studying the sources:
fb_data is defined as

one byte ("unsigned char") when LCD_DEPTH <= 8 or
two bytes ("unsigned short") when LCD_DEPTH <= 16 or
four bytes ("unsigned long") when LCD_DEPTH > 16
(currently no target sets this)

When loading a bitmap, the routine read_bmp_file always
makes sure, that the loaded bitmap fits to the native
format of the target → e.g. it changes 24-bit bitmaps
to 16-bit for H300 (and the other color targets).

As with last time I couldn’t resist and did some changes
and optimization to your code :-)

  1. the max sizes are now set to negative values on default,

this makes it possible to center the bitmaps also to

  the whole screen size.
- max width and max height are handled separately.
  So it's now possible to center the bitmap vertically
  but not horizontally.

About other things:

Was there a reason why you always incremented the buf
  pointer in wps_data_preload_tags before leaving it?
I don't see any reason to do it - am I overseeing something?

I also put the setting of the flag wps_has_albumart to the
very end of the parsing instead of the beginning.
Otherwise if something’s wrong you’ll get a “wps_has_album == true” where the variables are only partially filled (or not filled at all).

I hope you like my new changes - and I also hope I did remove all
things from other patches :-)

At last a new idea: what about adding another optional parameter
to the WPS-tag; a directory where the code additionally searches
for the bitmap? The directory could be an absolute path or it
could be relative to the directory of the currently playing track?
If you think that would be a good idea, I would be glad to add it
to the patch ;)

I like your changes a lot :)

About incrementing the buf pointer in wps_data_preload_tags, i pasted that code from another tag, so i’m not sure why it was done that way… About having a default dir for bitmaps, i like the idea, but maybe as a user setting rather than in the WPS tag. It’s more user-specific then WPS-specific. I think it would be better to have it absolute, and still give the priority to files in the current dir (anyway people would probably use only one of the two systems).

I made a few modifications, especially to prevent bitmaps bigger than the max size to be shown (which is the point of a max size ;-) ). I did like the idea oh having -1 if nothing was given so i just changed the check. Also there’s no need to make sure the bitmap is smaller than the screen because if it isn’t, read_bmp_file() won’t have loaded it.

mmohr commented on 2006-02-27 22:23

I think a default dir for bitmaps as user settings rather than in the WPS tag would make it easier to code.
But my idea behind it was not to have a absolute path (this is an extra for people who wants it ;)

The idea was to have a relative path (e.g. just “covers”) which will then be interpreted for each track.
So it would use the track’s directory as base and then add the given directory.
So it would be possible to store the bitmaps in subdirectories of the track.

I thought about my solution as WPS tag. But this is not easy to implement
(the track metadata will be loaded before the WPS tag data will be read)
So your suggestion with a user setting will make it (hopefully) easier.

So I’ll code it and post it here when I’m finished.

why not both an absolute path defined by the user and a relative path (maybe also defined by the user…) ?
For testing you can just as well hard-code the two paths for the moment and add settings later ;-)

While i’m on settings, maybe the current “cover.bmp” could become a setting too.

mmohr commented on 2006-02-28 12:32

Hmm, I’m not sure if it ever will be accepted if there are two additional options.
I think one user defined directory (either absolute or relative) would be enough.

About making “cover.bmp” a setting too:

Have you read the album art thread at misticriver?
There was a discussion about the size of the bitmap.
One suggestion was to name the bitmaps "cover_75.bmp" for 75x75 pixels,
"cover_100.bmp" for 100x100 pixels and so on.

For this to work the name of the bitmaps must be configurable in the WPS.

But I don’t know how to code it.
How can I transfer a WPS related parameter to the metadata loading???
(or access WPS settings from the track related metadata loading)

New version, should only be for testing for the moment.
It works on sim but i didn’t test it on target.
Needleboy: Please don’t include this in the experimental build yet.

I’ve moved the bitmap loading code from metadata.c to playback.c, and now the file buffer is used to store the data.
I’m not sure this is the optimal way to do it, as i’m a little bit confused with playback.c.

As you can see, i’m still working on this patch, but ATM i don’t have much free time so it won’t be very fast, but i’m not giving up until it’s into CVS ;-) (hoping it will be one day :-p )
I’m still planning on adding some directories in which to look for the files, but that’s not top-priority. I’m thinking of adding support for a “covers” relative subdir, and also an “/albumcovers” absolute dir.

mmohr commented on 2006-03-08 19:48

I’m also working on this patch -
but I also have really less apare time ATM
(and I work on my own WPS, too ;)

I already added the possibility for positioning smaller images
into the box relative to the margins (works similar as X geometries)
- this is compatible to the current WPS tag.
And I’m in the progress to add support for “cover” directories
(either relative subdirs or absolute).

If you want, we could discuss and synchronize the additional work!?
(write me an email to Matthias@Mohrenclan.de if you like to discuss
the things)

Is there a conditional tag for this so that you could make use of the “wasted” space in the wps for any files with no valid cover art?

Nudel commented on 2006-03-16 21:22

Small suggestion: If it looked for .cover.bmp that would save having to manually set the hidden attribute on all the covers.

(I was about to ask the same thing as Larry as well. It would be cool for WPS to be able to adjust their layout based on whether or not there is cover art.)

Nudel commented on 2006-03-22 09:12

Hint: If you use this patch and see a black square, chances are your BMP files are not 24-bit (e.g. they’re 32-bit). Re-save them as 24-bit and they should work.

mmohr commented on 2006-03-24 21:54

Here is a small update to the album art.
Nothing really big new, just a few additions:

1.) I added a conditional tag; it works as usual :)

this is for the cover bitmap of the current tag:
  %?C<true|false>
this is for the cover bitmap of the next tag:
  %?Cn<true|false>

2.) positioning of smaller images inside the defined “size box”.

This one works similar as X11 geomtries works.
For this the existing %C tag has been extended:
  %C|x|y[|[+|-]maxwidth|[+|-]maxheight|]
If no sign is given (as before) to the maxwidth, smaller bitmaps will be centered horizonatally inside the given width.
If no sign is given (as before) to the maxheight, smaller bitmaps will be centered vertically inside the given height.
If a '+' sign is given to the maxwidth, smaller bitmaps will be positioned at the left inside the given maximum width.
If a '+' sign is given to the maxheight, smaller bitmaps will be positioned at the top inside the given height.
If a '-' sign is given to the maxwidth, smaller bitmaps will be positioned at the right inside the given maximum width.
If a '-' sign is given to the maxheight, smaller bitmaps will be positioned at the bottom inside the given height.
Example:
  %C|20|10|+90|-90|
The image box starts at x=20 and y=10 and is 90 pixel wide and 90 pixels high.
So a bitmap with 90x90 in size will start at x=20 and y=10.
A bitmap with 70x70 in size will start at x=20 and y=30 so that it is positioned left and bottom to the box.
BTW, the maxwidth and maxheight are still optional...

That’s it - for now ;)

This patch causes problems with the tagcache. I’m going to have a look and try to find whats wrong.

And thanks Massa for you work, i haven’t tested it yet but it seems really neat :)

mmohr commented on 2006-03-28 15:34

What kind of problems?

“force tagcache update” make the player crash. After generating the cache with a clean CVS build and booting my build, it started crashing a lot. I then removed the album art patch from the build and now it works fine.
Some problems were also reported on the ipod forums : http://forums.rockbox.org/index.php?topic=3205.0

Here is an updated test version. It’s based on the previous test i uploaded, but improved a little.
The album art data is now copied over to the WPS image buffer on track change (wierdly, this actually happens several times per track change).
Also i added a way to disable album art buffering if the WPS hasn’t got the album art tag.
This way of treating album art seems to eliminate the problems with tagcache, so i’ll try to finalise it as quickly as possible.
This patch is still for testing because i’m not quit pleased with it yet. It still needs some more checks and a little more thought.
Also it’s based on version 3.5, so it doesn’t include Massa’s changes, nor the ones i made to it on my side. I’ll have to merge all this…

updated again… The album art searching is now in the get_metadata() function.
There is still some work to be done, as at least some of the bitmap data is sent to the audio decoder, which causes ogg to fail and playback glitches on other codecs.

the v4 test patch seems to not display art at all or only incidental. I put it in my experimental build and this is the reports I get in the discussion thread on my build which is at http://www.misticriver.net/showthread.php?p=422099#post422099

It appears the people reporting problems salved it by clearing their settings.. I’ll stay with this one..

mmohr commented on 2006-03-30 21:27

Hmm, I tried the test_v4 patch inside the simulator.
But I couldn’t make it work.
It says “Album art found” - but it’s not visible… I didn’t have time to do deeper research - just wanted to tell you…

The conditional appears to always be false.

mmohr commented on 2006-03-31 15:37

which conditional?
“album_art_found” or “load_album_art”?
Or something else?

New test version, still not recommended for daily use : there are still some problems in playback.

%?C<true|false> - always returns false when I use it

mmohr commented on 2006-03-31 17:08

rotundo: that’s because my additions are currently not included in the test patches
and therefore the %?C also does not work!

Here is a version which includes that again… (nothing else changed)

BTW, the v5-test still does not show any albumart in my simulator… (will have a look at it later this evening)

Bono commented on 2006-04-01 19:02

I still have data abort erros with this patch (the last version) and current cvs. I reverted it and i got no data abort errors. Those errors where here only in the WPS.

After talking with Slasheri, i decided to do this another way until the metadata system is changed. Storing the bitmaps in the file buffer was a rather good method but created too much problems (that was in the ‘test’ versions).
However, i found something else, probably much better than storing the bitmaps in a fixed size buffer for each track (that’s what was done beofre in the non-test versions) : finding if an album art exists is done by get_metadata(), but loading the bitmap is done just before the track is played, and it’s stored directly in the WPS image buffer. There is only one bitmap loaded at a time, and the loading happens only if the bitmap changes.
The only drawback i can see to that method is battery consumption when the bitmap is loaded, as the disk shouldn’t be spinning at that time (i’m not quite sure though), and it has to be started to read the bitmap.
So finally, that version should be much more adapted for people who don’t use album art, as they won’t lose memory nor battery life. For people who use it, the only thing they *might* lose is battery life.

Things planned are adding Massa’s changes (conditional tag and better placing), and Paprica’s resizing code.

Bono commented on 2006-04-01 19:27

This last patch (album_art_v4.0.patch) completly solved my problems with Data abort errors. Great job !

This version solves the problem where album art would never be shown if the rwps was other than rockbox_default.

mmohr commented on 2006-04-02 10:48

Here is my patch which is based on album_art_v4.0.patch
It reenables my additions (%?C conditional and positioning
of smaller images inside the defined max. box - for details
see above).

AND: I also solved the problem with the rwps :D
Nicolas, I think my solution is more clean than yours ;)
I also removed the playback.c flag and I removed the additional
parameter to get_metadata - but my solution is still able to
detect if loading of albumart is necessary or not!
I created an additional function gui_load_albumart which does
the detection based on exisiting flag wps_has_albumart in wps_data
structure.

Have a look at - I like it :D
the solution which I had last night was a bad hack -
this one here is much better!

The gui_load_albumart() function looks the right way to do it - but I think the convention for functions that deal with all the screens is to use the gui_sync prefix - so you should probably rename that function to be gui_sync_load_albumart(). Or maybe gui_sync_wps_needs_albumart() is more descriptive.

Also, get_metadata() is used by tagcache when building the database - so I think the parameter to tell get_metadata() to load the album art should stay. This will be false when called by the tagcache code, and the result of gui_sync_wps_needs_albumart() when called by the playback code.

mmohr commented on 2006-04-02 15:13

Here is another version - this one makes the changes which linuxstb suggests
(renamed function to gui_sync_wps_has_albumart and added again an additional
parameter to get_metadata which tells the get_metadata if it’s been called by
the tagcache)

I also removed of another flag variable albumart_loaded -
it’s not necessary, we could also use the albumart_data pointer
(NULL or not NULL) to ask for this funtionality.

It’s still not perfect - but it seems to work :)
What I still don’t like are the many variables which are needed in the mp3entry
structure - it’s a big waste of memory if someone doesn’t use album art bitmaps…

NOTE: the patch is in my next post (after paulheu’s post)…

I’m getting lot’s of warnings from ‘bool gui_sync_wps_has_albumart();’ .. From what I figured that should read ‘bool gui_sync_wps_has_albumart(void);’ ?? (which makes the warnings go away)

mmohr commented on 2006-04-02 16:35

I don’t get any warnings - but you’re right, it’s more correct to use the void as parameter.

I changed it…

mmohr commented on 2006-04-08 09:02

Sync to todays CVS (2006/04/08)…

Synched with current CVS.
This version is basically the same as Massa’s, there are just a few minor internal changes.

Next version will hopefully be 4.1 with Paprica’s bitmap scaling :)
However i can’t assure you it will come soon, as i’m starting a really intense period of exams soon.

mmohr commented on 2006-04-09 22:24

I hoped, Paprica will create a more common patch -
which adds the bitmap scaling to the bmp code
and not only to the album art.

BTW, the old scaling code which I know has one big problem:
it needs an image buffer to hold the complete original
bitmap. And the original bmp code only loads bitmaps
in the size of the screen because its buffer is only
that big… But maybe Paprica solved that problem?!

updated for current CVS.

About the scaling, i don’t really know what’s Paprica’s current status on it, but the last version he sent me was only for the album art and not generic. Also the size problems haven’t been solved…

Erm, newbie here… how do I apply the patch? I tried reading the documentation at http://www.rockbox.org/twiki/bin/view/Main/WorkingWithPatches but I don’t understand it. Many thanks in advance… :)

The D commented on 2006-05-10 08:40

Does the patch load the first bmp which fits in the wps tags (e.g 100×100 “album”.bmp does not fit tags, load 75×75 cover.bmp)?
Or does it just try to load the first (it finds in the folder) on its list and if that one does not fit it gives up?
Would it be possible to make it so that it looks for say a bmp called “cover100.bmp”(for 100×100 art) if it fits the wps it will load it, but if it does not fit it looks for “cover75.bmp”(for 75×75 art) and if it fits it will load it ect.

Cheap trick I know, but it would be a good stop gap between what we have now and bmp scaling.

The D commented on 2006-05-10 09:56

I just got a better idea, what if you give the wps a new tag (or just add it on to the album art tag) say %C|x|y|maxwidth|maxheight|”file you want to look for first”.bmp| which would tell rockbox to look for the file in the tag first then <filename>.bmp ect.

senab commented on 2006-05-12 10:33

Very nice patch, thank you very much.

Just one comment for any users: On my 5G iPod, Rockbox seems much more responsive when 8bit bitmaps are used instead of 24bit (or 16bit for that matter). To be honest, I can’t really tell the difference when they’re resized down to 100×100 anyway, plus they’re a third of the size.

mmohr commented on 2006-05-15 11:10

TheD: look above and you’ll see that I also had that idea :-) But it’s not so easy because the parsing of the WPS occurs after
the loading of the metadata and therefore the bitmap has already
been loaded at that moment.

Currently the work on the patch has been delayed after v3.0 and
after the viewport implementation will find it’s way to the CVS

Hi, I’m an iPod video owner who got frustrated with the inability for the this patch to display the album art because of the frankly rediculous iTunesDB folder structure.
I:\\iPod_Control\\Music\\F01
I:\\iPod_Control\\Music\\F02
etc
However, I have come up with a solution to this.
Just by simply adding a few lines of code to the end of the find_albumart() function, the plugin will also search the parent directory of where the mp3 resides for a bmp with the album name as a filename (i.e. I:\\iPod_Control\\Music\\albumname.bmp)
Now all my albumarts can just be in one place!

If anyone cares, this is how I did it.
If there is enough demand, I could make a .patch

takka commented on 2006-07-14 19:08

album art auto resize test for H300

takka commented on 2006-07-18 21:17

album art auto resize patch(only 24bit bmp)
cover.bmp is resized by the automatic operation according to wps.
When reducing, the interpolation reduction is done.(only 16bit LCD)

how to use
patch -p0 < album_art_v4.03.patch
patch -p0 < album_resize.patch

takka commented on 2006-07-20 19:04

album art auto resize patch(only 24bit bmp)
cover.bmp is resized by the automatic operation according to wps.
When reducing, the interpolation reduction is done.(16bit/gray/mono LCD)
bmp file MAX Width is 1024.
same bug fix.

takka commented on 2006-07-20 19:30

Sorry.
v0.8 is memory over.
bmp file Max Width is 512.

takka commented on 2006-07-20 20:23

FIX memoey over.
bmp file Max Width 1024.

takka commented on 2006-07-21 03:56

album art auto resize patch(support 24bit bmp/8-bit palette bmp)
cover.bmp is resized by the automatic operation according to wps.
When reducing, the interpolation reduction is done.(ALL LCD type)
bmp file MAX Width is 1024.

Not support MONO bmp file.
Not optimize Source.

how to use
patch -p0 < album_art_v4.03.patch
patch -p0 < album_resize.patch

takka commented on 2006-07-22 05:42

album art auto resize patch(support 24bit bmp/8-bit palette bmp/Mono bmp)
cover.bmp is resized by the automatic operation according to wps.
When reducing, the interpolation reduction is done.(ALL LCD type)
bmp file MAX Width is 1024.

v0.85→v0.90 update
8-bit palette bmp file bug fix. THANKS Ashen!
Mono bmp support.

Not optimize Source.

how to use
patch -p0 < album_art_v4.03.patch
patch -p0 < album_resize_v0.90.patch

mmohr commented on 2006-07-22 08:40

takka: really nice patch!
That’s IMHO how it should work - resizing integrated in the bmp code!
Good job!

I think it would be best to separate the bmp resize code from its
usage for the albumart.
Please produce a separate patch (including its own flyspray number :-)
just for the bmp resizing!
I assume if you make it clean and optimize it, it’ll soon find it’s
way into CVS ;-) And it could be used for much more things
(e.g. integrate bmp format in the jpeg viewer to also display BMPs…)

takka commented on 2006-07-23 02:27

album_art_v5.00.patch
This patch need bmp resize pach( http://www.rockbox.org/tracker/task/5697 )

Takka, thanks for your work !
It works really well and i think a lot of people are going to like it :D

I haven’t been able to get album_art_v5.00.patch to work on my Ipod nano. Attempting to play a song with associated cover art causes rockbox to hang (requiring a reset). album_art_v4.03.patch works great, and bmp_resize_v0.99.1.patch by itself seems to work fine (bmpviewer resizes images). But bmp_resize with album_art_v5.00 together just causes the whole thing to hang when I try to play anything. fwiw, all the covers currently on the player are already the right size (created before the resize patch), so no resizing is even happening yet.

senab commented on 2006-07-25 20:00

@iconoclast:
I’m also having this exact problem for my experimental builds (tried on 5G). May have to resort back to v4.03 for a bit.

Same problem here.

Sorry guys, i forgot to give you more info.
So, iPod frozes on loading album art with the following patches:
- album_art_v5.00.patch
- bmp_resize_v0.99.1.patch
using iPod 5g.

Hope this helps.
Thanks!
Julius

takka commented on 2006-07-26 22:49

Ver.Up bmp resize patch.
( http://www.rockbox.org/tracker/task/5697 )
I haven’t got iPod. So feedback is welcome!

I am going to try applying today takka, with the new bmp resize.

will let you know how it goes!

Ok… run some tests - all using daily build from today: 27.07.2006

Device: iPod 5g - 60GB

Clean patch with Album Art v4.03 = cover.bmp (100×100) displays CORRECTLY (as predicted! :D)

Clean patch with BMP Resize v0.99.2 + Album Art v5.0 = NO artwork displayed. This includes 100×100, and a larger test BMP.
However, playback is now fine, and I am not getting any crashes at the moment.

I might check if BMP viewer is resizing the images later to confirm that the BMP resizer patch is working, but I think it will (consiering ironclast’s comment above).
So it would still appear to be something in the relationship between the two.

Thanks for the work takka!

takka commented on 2006-07-27 11:55

Thank you for the report.
Simple %C|0|0|150|150|How when it is WPS of it is possible to hold?
It survives when it compiles by attaching DEBUG, and the error log is
given.

takka,

Apologies, I am not sure what you would like me to do? I have just changed the WPS to the settings you suggested and will report back.

Is the error log from your DEBUG compile clear?

Yotto commented on 2006-08-10 04:33

I just posted this in #2429 and then saw that this Flyspray topic received far more traffic.

Currently, at least it appears, Album Art must be on its own line (Like the progress bar) or it won’t show at all. it would be nice to be able to use a conditional to determine if AA should be displayed, or even where.

For example (Code that looks like it should hide the Album Art with the hold switch turned on):
# AAHide.bmp is a 100×100 white box
%xl|A|AAHide.bmp|1|1|
%?mh<%xda|%C|1|1|[100|100|]>

This currently does not work, neither does:
%?mh<|%C|1|1|[100|100|]>
To conditionally show Album Art or not.

The 2nd is preferable and cleaner, but the 1st acts like all other images.

mmohr commented on 2006-08-10 20:13

Wesley, you’re the second person who misinterprets the comments to AA :-).
The “[” “]” around the last two parameters only means that they’re
optional and that it’s correct syntax not to use them - it does
NOT mean that you have to add the “[” “]” pair around them… (actually it’s usual syntax used by many programs..)

And that’s also one of the problems with using the tag in conditionals.
The code does not know when the tag is finished and therefore cannot
distinguish between the last parameters of the %C tag and the following
tag.
Another problem is that in the current source structure the Album Art
tag has to be a “preload tag” (like backdrop image, preloading images,
statusbar display, …) which are only meant for static things.
Therefore it’s not possible to use the %C in conditionals…

I’ll think about it - maybe I find a solution for this dilemma :-)

mmohr commented on 2006-08-10 21:32

I restructued and updated the description above to reflect
the various changes and additions which have been made.

Updated patch to compile agains CVS HEAD 12.08.2006

senab commented on 2006-08-12 14:58

@Maxwen: This patch has this been synced from? V5.0? does this still require the BMP Resize patch?

Yes it is just an update to compile against CVS HEAD from today
no other changes made

Dont know if the version number should change?

I also added a changed BMP resize patch
see http://www.rockbox.org/tracker/task/5697

senab commented on 2006-08-12 15:06

@Maxwen: Nope it shouldn’t, it’s still V5.00 except it’s been synced again.

Ok so just upload with the same name?

senab commented on 2006-08-12 15:14

@Maxwen: There’s not much point now, my comments should alert people to that it’s just a resync.

mmohr commented on 2006-08-20 20:03

Here is an update to the albumart patch.
It adds some new functionality and I added some internal structural changes.
It has still some glitches and I’m not sure if it works on every device
so I call this new beast a “TEST version”!

Here are the changes/additions:

  1. additional search for cover bitmaps in parent directory of the title (<album>.bmp as well as cover.bmp).
  1. resizing of bitmaps or not - defined in the tag

For this the syntax of the tag has been changed:

  %C|x|y|[l|c|r|d|i|s]maxwidth|[t|c|b|d|i|s]maxheight|
  (what's inside brackets is optional)
  NOTE: of course the brackets itself may _NOT_ be added!
  The maxwidth parameter have got additional possible flags:
    l = align left
    c = align center (default)
    r = align right
    d = decrease - resize bitmap's width if it is bigger than given maxwidth
    i = increase - resize bitmap's width if it is smaller than given maxwidth
    s = combination of d & i - always resize bitmap's width to given maxwidth
  The maxheight parameter als have got additional possible prefixes:
    t = align top
    c = align center (default)
    b = align bottom
    d = decrease - resize bitmap's height if it is bigger than given maxwidth
    i = increase - resize bitmap's height if it is smaller than given maxwidth
    s = combination of d & i - always resize bitmap's height to given maxwidth
  NOTE: the old +/- flags are still valid, but I declare them as deprecatated :-)
ATTENTION: the maxwidth/maxheight fields itself MUST be given now (but can be empty)!
Example1:
  %C|50|70|||
  displays the found bitmap at position x=50, y=70 - no maxwidth/height!
Example2:
  %C|50|70|s100|s100|
  displays the found bitmap at position x=50, y=70 - resize smaller and larger bitmaps to 100x100
Example3 (does not really make sense :-):
  %C|50|70|r100|d100|
  displays the found bitmap at position x=50, y=70 - 
  only bitmaps with a maximal width of 100 will be used;
  bitmaps with smaller width will be positioned right to the defined maxwidth (they end at x=150)
  bitmaps with smaller height will keep their height
  bitmaps with bigger height will be resized to 100.
  NOTE: combinations of the flags are currently not allowed - will be fixed in next version.

- added the possibility to use the albumart tags inside other tags

   and also in conditionals.
 For this I had to change the parsing mechanism of the wps preloading tags.
 Example:
   %?ps<%C|2|2|90|90||>
 means "display album art only if the shuffle mode is set".
 Currently it only works in parts:
 it does check for the condition - but only when the WPS screen will be entered.
 It does not switch off the albumart if you switch off the shuffle mode.
 I'll have a look at it and fix it for the next version...

To apply the patch you first need to apply the BMP resize patch in the version 0.99.4-MM (see #FS5697)
Then you should apply the wps_preload_tags_20060820.patch (this one changes the general parsing of the WPS preload tags).
After this you can apply the albumart patch itself.

Please tell me if it works or not - and what exactly doesn’t work ;)

Update 2006-08-30: removed the not working v5.1.0-TEST patch file!

Hi Matthias!
Hmm. I don’t get it to work:
cvs sources from Tue, 22 Aug 2006 08:39:11 +0200
bmp_resize_v0.99.5-MM.20060821.patch
wps_preload_tags_20060820.patch
album_art_v5.1.0_TEST-MM_20060820.patch
No other patches
loading a theme where the wps file *ONLY* contains
%C|12|40|s75|s75|
hangs indefinitely at “Loading …“. Reset button necessary.

Ciao
Norbert

mmohr commented on 2006-08-22 08:56

I did some more tests and found some horrible bugs!
So give me some time to fix them…

–> ATM: DON’T USE THE PATCH v5.1.0!!!

To be honest, I was unable to reproduce your particular problem -
but others (and my code has slightly changed since 5.1.0…)

mmohr commented on 2006-08-23 15:03

O.K. - here is some feedback about my current progress.

I again had to stop because I found another limit in the current
WPS parsing code which makes it impossible to make the AA tag
properly working inside conditionals :-(

So I came finally to the conclusion, that in the current
WPS parsing implementation it’s impossible.

Here’s what I do next: I’ll strip down the v5.1.0-TEST so that
it works properly but _NOT_ inside other tags.
Another problem is the resizing code - it seems it does not
allow all features which I need. But this may be fixed by takka…

After that I’ll try to rewrite the whole WPS parsing code -
maybe on base of safetydans patch ( FS#4826 ), which may take some
time.

Nicolas, when are you online in IRC?
I want to discuss some ideas with you.
Or write me a PM at the forum…

mmohr commented on 2006-08-30 21:19

Now here is another incarnation of the albumart patch.
I call it v5.1.1 and it does change the tags again!

They now work similar to the %xl/%xd tags - will say,
there is a tag “%Cl” (which has to be on it’s own line)
with which you define the position, dimensions and resize
behaviour of the albumart bitmaps.

Then you use the “%C” tag (currently without any parameter)
to actually display the albumart.
The bad news about this change is that it’s no longer
compatible to the older tags and every WPS which uses it
have to be changed.

The good news is, that it’s now able to use the “%C” tag inside
conditionals and switched in on and off on the fly!

So the new “%Cl” tag has the same syntax as the v5.0.0 “%C” tag,
the new “%C” tag has no arguments!
The “%?C” tags are the same as before…

I hope I didn’t break anything… Test it and tell me if something does not work!

The v5.1.1 still needs the v0.99.5 bitmap patch but does not need
the wps_preload patch!

It is able to resize the bitmaps - but only within the limits of
the resize patch (only decrease, not with different width/height factors,
…)

Enjoy!

P.S.: I removed the not working v5.1.0-TEST files from above posting…

I just tested with todays CVS repo., and this patch (v5.1.1) was the only patch I applied that makes changes to /apps/playback.o … I get this error:

CC playback.c
playback.c: In function ‘audio_load_track’:
playback.c:2391: error: too few arguments to function ‘get_metadata’ make[1]: * [/home/Rob/rockbox-devel/build/apps/playback.o] Error 1
make:
* [all] Error 2

I applied the bmp resize patch, but that doesn’t seem to be the problem. Am I doing something horribly wrong, or is there really something smelly about get_metadata?

I forgot to mention: if it matters, I use Cygwin. And in my comment above, I meant playback.c of course, not playback.o

tuwe commented on 2006-09-01 10:43

rob,

the patch does not work with the latest cvs dropout anymore. take a look at the messages coming from the patch command:

patching file apps/playback.c
Hunk #1 succeeded at 2524 (offset 833 lines).
Hunk #2 FAILED at 2609.
1 out of 2 hunks FAILED – saving rejects to file apps/playback.c.rej

this is because of some rearrangement in the code, see cvs activity, 1 Sep 07:59.

you have to apply the patch manually. this means you have to replace line 2391 of playback.c with the following:

  if (get_metadata(&tracks[track_widx],current_fd,trackname,v1first,true))

for me work fine…
I test this build:rockbox-daily-20060830, rockbox-daily-20060901… with this patches:
bmp_resize_v0.99.5-MM.20060821.patch
album_art_v5.1.1-MM_20060830.patch
menu_buttons.patch
onplay.c.patch
progressbar_ycoord_extension2.1.patch
(all -p0)

in wps I use this:
%Cl|0|47|s79|s79|
%C
very thanks…

mmohr commented on 2006-09-01 22:18

Only a day old and already unpatchable :(
Here’s a sync to today’s CVS..

BTW, it works best with at least bmp_resize patch v0.99.6
(this one should overcome the earlier limits and e.g. also
allow to increase pictures :-)

mmohr commented on 2006-09-01 23:05

I updated the patch descriptions above to reflect the
changes and additions which have been made in v5.1.1

Maybe a sytupid question :-) Is it normal and wanted that the func wps_data_albumart_display is
called on every format_display call?

I just tried it in the simulator with the BlackGlass
http://www.rockbox.org/twiki/pub/Main/WpsIaudioX5/BlackGlass_V2.zip theme and changed it to use

%Cl|3|20|s75|s75|
%C

But now the album display has some ‘empty spots’ where the background is displayed.

Does the wps need to be changed?

Ok If I put the %C at the end of the wps file
it is displayed correct

mmohr commented on 2006-09-03 21:17

maxwen: yes it’s normal that the func wps_data_albumart_display is
called on every format_display call - if you have a %C tag in your WPS.
That’s what format_display does - go through the WPS tags and do what
their action is. For the albumart bitmap the action is to “display” it :-)

About the “empty spots” - I assume it happens because the bitmaps
contain some color information which means “transparent” (purple)
to the display routine - but why didn’t it happen before?
And why not when you put the %C at the end of the WPS?
Currently I don’t know - I’ll dig into and search for a reason
(but I don’t know if I have spare time this week to do it - so it may
take some time…)

It happens after the update to 5.1.1 if I use 5.0.1
with the same wps it is displayed correct

And the empty spots in the image are at lines where text
is displayed right of the image. They also have the same height
as the text.

If the %C is put after all the other conditionals it seems
to be ‘painted last’

mmohr commented on 2006-09-04 21:22

maxwen: I assume you’re able to compile your own build.
Could you please make a test and check, if this solves the spot problem?
Go to the end of the function wps_data_albumart_display and change the

line
gwps->display->transparent_bitmap
to
gwps->display->bitmap

Tell me if it then works… (I’m unable to reproduce your problem).

BTW, the scroll margin patch also has a problem when combined with albumart.
I have a WPS where I tried to use margins with a bitmap right to it.
If the line starts to scroll the line flickers inside the bitmap…

Yes I use also the scroll_margin patch
and yes I also noticed the flickering but not on every wps

I will try your suggested fix asap

I am back at albumart_5.01 for the moment and need a little
time to get all my builds done before I can try 5.1.1 again

Could you create something like:

%Cl|4|54|d40|d40| become %Cl|a|4|54|d40|d40|
and %C become %Ca, would be great to show different sizes of the album art if the device is hold or not.

Possible?

Is there a buffer limit?

I set the album art to resize down to 100×100, but Ive had to lose 7 small images in the wps for it to load the cover art. Excluding the 7 images, I have 42 images in the wps (including the background image) amounts to (putting both sizes as stated in XP Properties for the files) Size: 143 KB (146,888 bytes) Size on Disk: 236 KB (241,664 bytes). Anymore images added would stop the album art from loading. Any way to increase the limit?

ps. I use a Nano.

Basically your problem comes from the fact that the album art bitmap is currently stored in the WPS image buffer. In fact there is no real limit to the size of the bitmaps you can load, only the space available in the WPS image buffer.
To overcome this problem you have two solutions :
1. use less pictures in your WPS
2. use a custom build with a bigger WPS buffer

“2. use a custom build with a bigger WPS buffer”

How would I go about doing that? I can edit the source and compile etc, but where could I find information on how to change the WPS buffer?

mmohr commented on 2006-10-15 10:34

Here is a sync to today’s CVS

@ipodfoo: IMG_BUFSIZE in apps\\gui\\gwps.h

Hi. I changed the number 8 to 2 on the line:

#define IMG_BUFSIZE 1)

1) LCD_HEIGHT*LCD_WIDTH*LCD_DEPTH/8)
                 + (2*LCD_HEIGHT*LCD_WIDTH/2))
in the apps/gui/gwps.h. This seems to work. All the images loads fine now. But Id like to know why it was set to 8 in the first place, and have I altered things in a way that affects something else? Many thanks for all the help.
mmohr commented on 2006-10-23 09:12

@ipodfoo: Huh? The IMG_BUFSIZE should already have that size???
(the bmp_resize-patch extends it!)

And it affects the memory usage (which may affect other things
if they don’t get enough memory - but I didn’t heard about such
a case…)

@ Massa: I changed the first “8”. So it now looks like this #define IMG_BUFSIZE ((LCD_HEIGHT*LCD_WIDTH*LCD_DEPTH/2)

I actually have it now set to 6 to save memory (i assume it does, Im not a techie!), and that works too. 8 or above and the wps wont load the album art. Loading the album art seems a bit sluggish, it takes about 7 seconds for the album art to show up when loading an album. Currently using Senabs 060921 source. But its a sacrifice Im willing to take! :)

I hope you’ll forgive a pretty naive user posting here, but I could do with a bit of reassurance I’m not wasting my time trying to do something that’s not supposed to work in the first place. I’ve patched in bmp_resize_v0.99.6-MM.20061102.patch & then the album_art_v5.1.1_MM_20051015.patch, using today’s cvs. It appeared to compile OK, so I transferred the build to my X5. I put 3 wps themes on the box: X5_Album_Art_100x100_SNAP, boeselhack_v2.02 & Hillside. I put 24-bit 100*100 cover.bmp files (30KB apiece) in the directories holding my music files, and also a 100*100 24-bit [filename].bmp in one directory that holds [filename].flac. The themes work OK but won’t display the bmp files, acting as if the files aren’t there at all (ie not showing blank spaces, but using the alternative formatting where a theme has one set up). In fact the X5 won’t display the bmp files if I select them directly either, although it’s perfectly happy to display jpg files. All my music is flac, so the tags are Ogg Vorbis format with no artwork references.
This is the first time I’ve tried to patch Rockbox, so I’m quite prepared to go back through the whole process again, but should I expect this to work at all, or does this patch assume ID3 tags are present? And incidentally, do I really need to stick with bmp files when the X5 can handle jpg perfectly well?
Thanks for any hints anyone can give..

Correction to my post yesterday: The themes don’t act as if the files aren’t there at all. If the file is there, then the theme sets out the screen as if it was, but doesn’t display the bmp file.

I’m starting to create a wiki page for this patch. This flyspray entry is becoming quite big so it’s hard to find information here. I hope the wiki page will make it easier. Also i feel it will be easier to keep track of the developement that way.
In its current state, the wiki page is still very basic and i will improve it after the week end (when i get back from my trip to Barcelona :D ). Actually i was trying to create some sort of “personal draft”, but being a newbie in wiki writing i failed miserably :(

I made some progress on the wiki page : http://www.rockbox.org/twiki/bin/view/Main/AlbumArt Comments are welcome :)

mmohr commented on 2006-11-13 21:25

@Andy R: Sorry - I don’t have any idea what’s going on at your device.

@nicolas_p: I had a short look at the wiki page - really nice and visually
much better to read than the descriptions here :-)

@all: I’m really busy with may real life’s work atm., so I don’t have

much time to look after my patches.
I try to synchronize them whenver the CVS breaks them,
  but I'm not always as fast as usual.
I also try to fix bugs - when I'm able to reproduce them easily - but it
  may also take some time...

BTW, are the patches (AA and BMP-Resize) still working?

I didn't test it for a week or so...

Matthias: the patches still apply fine :-) ATM i’m around with some time so i can take care of keeping them updated.

Andy R: Are you still having problems ? One thing you can try is to create a basic WPS to test… Just put the %C and %Cl tags, a few lines of info and no pictures. If that works, it means your themes have too much pictures. If not, then it’s something else…

v5.2:
- A few minor changes to make WPS changing almost work (in some cases garbage is displayed until the next track is played).
- Added some #ifdefs for consistency and to allow building tagcache on the computer.

Will this be applied to CVS eventually?

mmohr commented on 2006-12-02 09:29

I assume it’s currently not able to apply the album art patch.
The bmp_resize needs to be redone because of bigger changes in bmp.c
(have a look there for details)

@netmasta10bt: I don’t think album art will soon be applied to CVS.
It still has some issues which needs principle changes/enhancements in the
base code to get fixed (look at the wiki for more information).
Although I hope the bmp resize patch will soon find it’s way to CVS
(after I reimplemented it).

I’ve done a version of the patch that doesn’t need the bmp resize patch :

Do you still need the bmp resize patch to be able to resize different sized cover.bmp’s?

SebZ commented on 2006-12-04 13:27

Ok, I’m a newbie to this.
How do I add this patch to my current H340 RockBox build?

Billy: the resize patch is no longer in sync with CVS so you have to choose between having an up-to-date version of rockbox or the BMP resizing feature.
What I meant when I said the BMP resize patch was not needed was that the feature was (hopefully temporarily) disabled.

SebZ: I think you should check out the wiki page about patching and compiling Rockbox : http://www.rockbox.org/twiki/bin/view/Main/SimpleGuideToCompiling


Here is an updated version of the patch *without bitmap resizing capabilities*

The above appears to be brocken now with the builds from Dec 13th & 14th both CVS & Daily builds on Ipod Video 5G

err, it works for me… can you elaborate ?

Please see code snippet from few minuets ago
This is what I Get when I try to install Patch

Administrator@the-wheelers ~/rockbox-devel
$ patch -p0 <album_art_v5.2_nobmpresize_20061213.patch
patching file apps/metadata.c
patching file apps/metadata.h
patching file apps/playback.c
patching file apps/tagcache.c
patching file apps/gui/gwps-common.c
patching file apps/gui/gwps.c
Hunk #1 succeeded at 673 (offset 11 lines).
Hunk #2 succeeded at 703 (offset 11 lines).
Hunk #3 succeeded at 888 (offset 11 lines).
patching file apps/gui/gwps.h
Hunk #1 FAILED at 24.
Hunk #2 succeeded at 38 (offset -261 lines).
Hunk #3 succeeded at 104 (offset -261 lines).
Hunk #4 succeeded at 201 (offset -274 lines).
1 out of 4 hunks FAILED – saving rejects to file apps/gui/gwps.h.rej
patching file firmware/export/id3.h

Administrator@the-wheelers ~/rockbox-devel
$

As you can see there is still an error
However this is much better than before and indeed will now “make” etc just tested in sim and it works!

haha, look at hunk 1 of gwps.h and you’ll see why it still compiles :)
This problem was in the previous patches and it started causing real trouble with a recent commit. Actually, I thought I had removed it in the patch right above, but apparently I did something wrong… Anyway, here is a better version (with a few other parasites removed) :

any chance of putting bmp resize back in?

Updated to current SVN.

I’ll try to look at the new BMP resize patch.

Needs another sync, it can’t patch gwps.c

SebZ commented on 2007-02-25 15:23

No support on Flyspray

Just in case there are sync problems, here is my local version. It still lacks BMP resizing.

It’s not working anymore for the latest build. When will rockbox team incorporate this into their build?

Patches and Compiles with no errors fo me on latest build
Are you using it with other Patches?

Alex: I tried using this patch a couple of days ago and it wouldn’t take. Tried today, loading it first on a clean build, and everything works well.

I think it’s just my inexperience. I used the downloaded build using SVN and it does not work. The build from build.rockbox.org works. Sorry.

This Is Now Broken by the new WPS tokenizer in the current SVN

I’ll sync it as soon as possible.

I gotta give it up to you guys that work on these patches. In an odd moment of non-laziness I thought I’d take a look at syncing the multifont patch since all that API mayhem threw it out of whack on the 25th and lo and behold after I tweak it to the point of possibly testing it I apply this patch and discover the new tokenizer mayhem. Congrats to all our fine patchers, I really have no idea how you find the time to do this since you gotta familiarize yourself with huge blocks of new code every few days to stay current.

Sync’d to latest svn. I’ve rewritten the code to fit in with the new tokenizer. This is not well tested, so post any problems you find with the new patch here please.

Minor update to avoid patch conflicts against latest version of scrolling margins patch

Fixed a fairly major bug, whoops. Black Glass works ok now.
Does anyone have an example WPS using either the conditional or the %Cn constructs?

This still has no bmp resize included? would be great to have those two patches together as separet they kinda suck!

What’s so bad about them being separate? They both work as they’re intended to do, you just have to patch twice.

Ya but it means that one has to manually do the .rej files. Seems to me that when 1 is synced, the other has to be too, since the bmp_resize needs the albumart patch. Makes more sense to just have 1 patch!

Fixed conditionals (%?C and %?Cn). Fixed default width,height (e.g. %C|x|y||| )

Hepdog - I don’t use bmp_resize so I’m not bothered about that patch. If someone is prepared to maintain that patch I’m sure they’ll step up

If you knew what you were talking about, you would know that patches with seperate functions are intentionally kept seperate.

idak commented on 2007-04-10 15:28

Fixed Aligns.

Idak, your last sync patches perfect, but if I’m using an line with albumart in my wps it doesn’t work.
For example “%C|34|3|100|100|”: it doesn’t show the albumart but “343100100” on the display. :(
I’ve an x5.

ColdSphinX - the Album Art syntax changed (a very long time ago). Scroll up this page to see the documentation. Search this page for “Comment by Matthias Mohr (aka Massa) (mmohr) - Wednesday, 30 August 2006, 11:19PM “. Now you need two tags:
%Cl on one line, and then %C on the next.
e.g.

%Cl|34|3|100|100|
%C

You need to update your wps appropriately

Idak - thanks for fixing my mistakes in the new parse_albumart_load - do you have a good example WPS that uses this?

I don’t understand why draw_album_art also changed – it looks like Right alignment is now always disabled because of your if(0) and Center alignment is always used because of your if(1) ?

idak commented on 2007-04-10 21:49

Sorry, I forgot to delete debug code… I fixed aligns again.
I use WPS like this:
%Cl|34|3|c100|c100|
%C

Thanks a lot Dave and Akio.
Now I have to edit every wps downloaded and selfmade. hehe

Akio - oops, I found another mistake (in my patch, but also in yours) - the ‘s’ handler for height mistakenly sets the flags for xalign not yalign (darn copy+paste..). Should be fixed now in attached.

I need a little bit of assistance on this. I have spent the last week or so going over all of the information provided about this patch, and I still can’t get the album art to work. I have a Gigabeat F40, if that makes any difference. I’m running the most recent Rockbox with the most recent art patch (album_art_20070409-v7.patch) and have added the art line (%Cl|83|182|) as well as the line under it (%C) but have been unable to see the album art. I have put the cover.bmp file into the album folder, as well as the artist folder, and have tried naming it after the album and the artist, to no avail. The image itself is definitely able to be displayed by the WPS - I can load it using a %xl command (from the WPS bitmap folder) just fine. The thing I’m trying to figure out is which other patches (bitmap resize and preload tags) may have to be applied to get everything to work. The problem is that none of those older patches seem to apply correctly - which I assume is because they’re for older builds of Rockbox. If someone could indicate the exact list of patches that need to be applied, in the proper order, or if there is some other thing I’m doing incorrectly, it would be greatly appreciated. Thank you.

@Mike
I believe you only need the album_art patch to have album art display, but your tag should be %Cl|83|182|||
The last two empty parts of the tag are for resizing (eg %Cl|83|182|120|120|) which doesn’t work without an additional patch.

Have a look here:
http://www.rockbox.org/twiki/bin/view/Main/AlbumArt#The_Cl_tag

Actually both that link and several of the comments above specifically state that you’re NOT supposed to add the || at the end of the line if you’re not resizing the bitmap. The bitmap I’m using is already made to be the right size. But I’m a little confused now, about which is correct. Either way, I’ve tried it both ways, without the ||, with it, and putting actual dimensions in between them also…

Ok you were right, after all. Despite my confusion on whether or not that link and the above comments state that you have to have the ending brackets, you definitely do. What I figured out the problem was, and I feel REALLY stupid, but I was using a lower case c on the tag (%cl|83||182|||) instead of a capital one. Now, I’d swear that I had tried it both ways, but I was obviously wrong. Anyway, thanks for the quick input. Everything is working fine now… :-D

There are errors (see below)
$ patch –binary -p0 < 3045.patch
patching file apps/gui/gwps-common.c
Hunk #1 succeeded at 73 (offset 1 line).
Hunk #2 succeeded at 374 (offset 4 lines).
Hunk #3 succeeded at 574 (offset 4 lines).
Hunk #4 succeeded at 987 (offset 15 lines).
patching file apps/gui/gwps.c
Hunk #1 succeeded at 790 with fuzz 1 (offset 34 lines).
patching file apps/gui/gwps.h
Hunk #2 succeeded at 199 (offset 9 lines).
Hunk #3 succeeded at 325 (offset 9 lines).
Hunk #4 succeeded at 446 (offset 14 lines).
patching file apps/gui/wps_parser.c
Hunk #1 succeeded at 114 with fuzz 2 (offset 7 lines).
Hunk #2 FAILED at 271.
Hunk #3 succeeded at 587 (offset -14 lines).
Hunk #4 succeeded at 1027 (offset 10 lines).
Hunk #5 FAILED at 1045.
2 out of 5 hunks FAILED – saving rejects to file apps/gui/wps_parser.c.rej
patching file apps/metadata.c
Hunk #2 succeeded at 2108 (offset 39 lines).
Hunk #3 succeeded at 2521 (offset 39 lines).
patching file apps/metadata.h
patching file apps/tagcache.c
Hunk #1 succeeded at 1622 (offset 18 lines).
patching file apps/playback.c
Hunk #1 succeeded at 2762 (offset 13 lines).
Hunk #2 succeeded at 2904 (offset 13 lines).
patching file firmware/export/id3.h

The patch fell out of sync at r13265 I believe

sync’d. if using scrolling margins, use latest scrolling margins patch too to avoid patch conflicts when applying both

The Patch Now Produces This Warning

CC plugin.c
plugin.c:494: warning: initialization from incompatible pointer type

From the latest SVN build first seen yesterday

Well it’s not album art, because the patch doesn’t have anything to do with plugin.c. Try using clean source.

I did use a clean source and tested without this patch no warnining with this patch the abocve warning
note I did this for an Ipod Video 60GB and inputted 64 for the ammount of memory present maybe its this resent change and the patch?

The 64 mb memory is probably the problem, try 32 mb or another target like the gigabeat.

I think the problem is, that this patch adds an additional parameter to the function get_metadata and this function got added to the plugin api recently (without the additional parameter of course). So in order to fix this warning you propably have to change the prototype of get_metadata in apps/plugin.h accordingly and add the additional parameter in the function call in apss/plugins/test_codec.c (i guess “false” would be appropriate here, but i’m not sure).

Thanks PaulJam that sounds like what is going on incidently I did try 32MB and it did the same

synced after metadata.c split

I’m getting failures at apps/plugin.h and apps/plugins/test_codec.c

You tried to patch with -p0, -p1, -p2 … ?? (I’m not sure it’s the good argument, but the idea is here ^^)

I seem to be having some trouble getting the album art to the size that I want. For some reason the maximum size I can set for the album art with it actually displaying is 209×209, yet anything bigger will result in a blank space where the album art should be. I know there is at least one other WPS that uses larger AA than this, with 240×240, although it does require EvilG’s custom build to work. Perhaps someone may be able to shed some light on this issue for me? I much appreciate any assistance. Here’s a link to the WPS I’m currently working on if it’s any help: http://rapidshare.com/files/40380884/beatMP_large_aa.rar.html

It seems that some tags dont update when in the %?C<|> line, this using the %pc for current time.

[IMG]http://img.photobucket.com/albums/v282/gregken/dump070715-150457.jpg[/IMG]

%e|1|32|158|2|BDBDBD|%al%s%?C<%e|50|32|109|2|BDBDBD|%pc/%pt%ar[%pp:%pe]|%pc/%pt%ar[%pp:%pe]>

%?C<%e|50|54|109|2|BDBDBD||%e|1|54|158|2|BDBDBD|>%al%pc/%pt%ar[%pp:%pe]

Same 2 lines but reorder, one in and one out of the conditional.

Eh sorry It didnt like embeded picture.
Heres the correct link.
http://img.photobucket.com/albums/v282/gregken/dump070715-150457.jpg This is what the code looks like.

I dont know why, but every time I’m trying to set the tags up, it shows the image in the wrong place.
I set the x and y to 0,0 but its still partially out into the screen, and I can move it left and down, but I can’t use any negative values to fix this problem.

Nevermind, I got it working correctly.

mede commented on 2007-08-07 10:23

PROBLEM DISPLAYING COVER
I have compiled the album_art_20070722.patch with the daily rockbox build from 6.08.2007

I used the simple command
%Cl|50|50|||

in the simulation output is written that album art found the image cover.bmp which have
a size of 100*100 pixels but the cover is NOT SHOWED ON THE DISPLAY. the same is on my ipod photo
30G. I also tried different bitmaps form 4 to 24 byte.

I have to use this rockbox build because there is a important bugfix.
Patching and Compilation worked without any problem

What’s the bit depth of your cover art? I know the wiki says you can have a 32 bit bmp but I’ve never had them show on my iPod 5.5g. I use 24 bit bmps myself. Try that.

mede commented on 2007-08-07 18:40

I use also 24bit

Mede, you should try a different theme that supports cover art. Some themes might support it, but it doesn’t always show up.
I’m not exactly sure why, but just try a different theme.

mede commented on 2007-08-08 08:25

hm it compiled this patch because i’am working on a owen theme … but i tested with a WPA which contains only the line

%Cl|50|50|||

and this still not work

Remember as well as the %Cl tag you need the %C tag also

The %C tag
This tag can be placed anywhere in the WPS file, including within a conditional tag. It displays the picture.

Examples

%C : just displays the album cover.
%?mh<%C|> : only displays an album cover if the main device’s hold switch is on.

idak commented on 2007-08-25 06:46

Broken albumart image is displayed when wps was changed.
So I fix this.

idak commented on 2007-10-05 16:42

Add #ifdef HAVE_LCD_BITMAP.
No functionality changes for this patch…

Wow, that was fast! You must’ve been one of the testers or contributors to metadata on buffer idak.

idak commented on 2007-10-26 11:46

I see. I’ll check that implementation ;)

I’m planning on implementing album art on top of MoB quite soon. This will be an almost complete rewrite. Of course if one of you feels like doing it, patches are welcome ;)

Are you planning to support JPEG or embedded AA, or just the way this patch was?

The first step will be to make it work like the current patch. Then maybe embedded AA but that still mostly relies on being able to do JPEG, which is not really planned. I might try to do it but it’s in no way one of my priorities.

jeton commented on 2007-10-26 17:05

He said he might try. That’s a reason to keep an eye on this patch. ;)

I have a quick question: will the implementation of album art on MoB feature resizing, or will the bmpresize patch still be needed for people that want that?

Resizing will probably be included, but maybe not in the first commit. Also there’s a good chance we’ll use a better resizing algorithm than the current resize patch. However I can’t make firm predictions because I haven’t even started thinking about the implementation. ATM I’m focusing on fixing bugs MoB introduced.

Not a problem, we understand that dealing with the MoB is your first priority. (couldn’t resist that double entendre) I’m just stoked that the main barrier preventing album art from getting committed is gone now.

I’d like to create a new WPS. Actually, i’d like to cut the pictures, only have a 128x80px picture, verticaly centered. It seems that we can’t do that right now… Could someone add this feature ?

Somebody has added that feature. It’s called bmpresize. I don’t remember the exact fs number for it is but if you go back to the main page and type “bmpresize” in search I’m sure you’ll find it.

yes but it’s not implemented on the WPS Album Art Tag, wich would be great :)

Yes it is, it’s the ‘s’ part. I believe %Cl|xcord|ycord|s100|sc100| would be what you would need to have that vertically centered in a 100×100 space.

I didn’t explained well maybe ^^

See the attached file, i think it’s more clear than what i said :)

Ah, you want a patch that will clip your picture. I don’t see that happening any time soon. If you centered the image vertically in a space smaller than the actual image without sizing it you would get clipping. You could play around with different sizes until you found the one that would clip that exact region. But you really should edit your images to be exactly what you want to display beforehand instead of expecting rockbox to do it.

kva commented on 2007-11-01 13:42

I tried to implement album_art_20071026.patch. Simulator works fine but on my H340 “real” build crash with “*PANIC* Stkov audio” when I try to play file. I tried to clear settings and used default theme… but player crash always.

Is there any chance that this patch breaks replay gain? or is it more likly another bug in the MoB.
This on an Ipod Video latest build and just using this patch.

Loading...

Available keyboard shortcuts

Tasklist

Task Details

Task Editing