Rockbox

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

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")
- 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"
- if that cannot be found look for a file named "cover.bmp"
- 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 75x75 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 100x100
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 100x100 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).

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 125x125 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

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.

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

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.

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.

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?

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

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.

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

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

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.

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

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)

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.

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.

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.

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)

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

I changed it...

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.

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

Does the patch load the first bmp which fits in the wps tags (e.g 100x100 "album".bmp does not fit tags, load 75x75 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 100x100 art) if it fits the wps it will load it, but if it does not fit it looks for "cover75.bmp"(for 75x75 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.

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.

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 100x100 anyway, plus they're a third of the size.

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

album art auto resize test for H300

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

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.

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

FIX memoey over.
bmp file Max Width 1024.

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

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

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

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.

@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

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 (100x100) displays CORRECTLY (as predicted! :D)

Clean patch with BMP Resize v0.99.2 + Album Art v5.0 = NO artwork displayed. This includes 100x100, 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!

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?

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 100x100 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.

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

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

@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

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

Ok so just upload with the same name?

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

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:
- additional search for cover bitmaps in parent directory of the title (<album>.bmp as well as cover.bmp).

- 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

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

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

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

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

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

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

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'

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 100x100, 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?

Here is a sync to today's CVS...

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