The GStreamer core now has support for image tags (GST_TAG_IMAGE, bug 340721), and we could change RBPlayerGst's process_tag() to emit it's "info" signal for them (converting from a GstBuffer to a GdkPixbuf).

The cover art viewer could connect to the player's "info" signal and look for image metadata, and use that instead if the stream contained one. We may want to give each entry a "has embedded art" flag too, so the art plugin knows whether it should expect art to arrive from the stream or not.


Regarding picture types, I think we should treat them all equally, with the exception of the "icon" type. Making some act differently wouldn't extend very well to formats other than ID3 (which may haev similar, different or no picture types).

Hi!

This is a feature that I would really love. 

If no one has the time to pick it up I would try to tackle it, but I wonder if a patch towards rhythmbox 0.9.7 would be accepted by you guys? I ask this because I'm using Gentoo and that's the latest unstable version it has available and I would like to avoid having to build GNome from SVN.

(In reply to comment #2)
> Hi!
> 
> This is a feature that I would really love. 
> 
> If no one has the time to pick it up I would try to tackle it, but I wonder if
> a patch towards rhythmbox 0.9.7 would be accepted by you guys? I ask this
> because I'm using Gentoo and that's the latest unstable version it has
> available and I would like to avoid having to build GNome from SVN.

You don't need to build the whole of GNOME from SVN, to run rhythmbox from SVN you just need to build the rhythmbox module, rhythmbox doesn't normally change its deps that frequently and should still build against relatively old GNOME installations.  

In any case, 0.9.7 is still fairly close to SVN, so that patch against 0.9.7 might work against, although we have moved some of the files around (especially almost  all files involved with plugins are now located under plugins/), so it would be best against SVN.  Either way, it's better for somebody to work on it than nobody, IMHO... ;)

@Nuno: FWIW I'm tracking rhythmbox SVN in my overlay (ecatmur in layman) with new snapshot ebuilds every few days.

This bug should be expanded to cover two more use cases for embedded album covers:
(1. Reading and displaying embedded art)
2. Writing out embedded art into files when the user sets it
3. Writing embedded art when transcoding for transfer to media player devices

I've been thinking about how to handle this and will post once I've got my ideas together.

ISTM the right way to handle this would be with a propid RHYTHMDB_PROP_EMBEDDED_ARTWORK that holds a weak reference; if the track has embedded artwork then the propid is deserialised as NULL; when GST_TAG_IMAGE is encountered it sets the weak reference to a GdkPixbuf with a refcount of 1; when the track stops it decrements the refcount.  Something (possibly rhythmdb itself) handles returning the pixbuf to entry-extra-metadata-request while the track is playing.

That is, the existence of RHYTHMDB_PROP_EMBEDDED_ARTWORK (whether or not it is NULL) indicates that the track has embedded artwork.  When reading in tags for new tracks, the propid RHYTHMDB_PROP_EMBEDDED_ARTWORK is set to a NULL reference.  We need to add cases in rhythmdb_read_encoded_property and rhythmdb-tree.c; the propid can be serialised as a boolean or some such, doesn't really matter.

This will also work with adding embedded artwork to files:
1. set RHYTHMDB_PROP_EMBEDDED_ARTWORK to a weak reference on the new GdkPixbuf
2. call rhythmdb_entry_set_internal
3. default_sync_metadata has entry_to_rb_metadata pick up the GdkPixbuf in a GValue, which handles ref counting while writing out the tag
4. free_entry_changes unrefs the pixbuf

Writing in embedded artwork when transcoding to iPod etc needs to be handled differently:
0. API: rb_encoder_encode (and RBEncoder->encode) will have to be extended with a GList argument of extra metadata id/value pairs to be added in to the encoded file.
1. in rb_removable_media_manager_queue_transfer, emit entry-extra-metadata-request::rb:coverArt on the entry
2. if artwork is not returned immediately, delay 5s in a timeout waiting for artwork (this is needed for the case where an album is ripped and copied to the media player immediately without being played first; if the Internet searches failed then with the patches at bug 363694 a NULL metadata will be emitted so there won't be excessive delay)
3. if artwork has arrived, ref it and add it to the list to pass to rb_encoder_encode
4. rb_encoder_encode (maybe) adds it in e.g. RBEncoderGst will serialise it in add_tags_from_entry (which also needs to be extended?)
5. when rb_encoder_encode returns, unref the pixbuf and destroy the list.

Right.  This feels correct; time to implement it.

Created attachment 99233 [details] [review]
Allow an art searcher to directly return image data

I haven't noticed much progress on this, so I implemented embedded art reading as a plugin. It's not very elegant - it doesn't use Rhythmbox's builtin metadata, but rather queries the file itself through GStreamer. However, it works quite well. Currently, file support includes MP3s with id3v2 tags and Ogg/Vorbis with a base64-encoded COVERART tag.

This is a two-part patch. This first part simply adds the ability for an album art search class to return raw image data rather than a URI.

Created attachment 99235 [details] [review]
Read album art embedded in id3v2 or COVERART tags

This patch adds an EmbeddedArtSearch class to the artdisplay plugin, which will open the file with GStreamer and attempt to read album art from it.

Your patch contains a lot of garbage I'm afraid. Please use "svn diff > patch-name" to generate your patch.

I think it would be easier to:
1)
- have a utility function in lib/ that takes an artist, and an album, as done by the CoverArtDatabase
- have the metadata backend use that function, and write the image data on disk in that location

or:
2)
- have the metadata backend load up that data, and put it in the database wholesale (probably base64 encoded, that would probably get too big?)

or:
3)
- have the metadata backend load up that data, and pass it as metadata
- add code to rhythmdb to dump that cover art to a file when it has loaded the metadata

I like 1 and 3 best. 1) should be especially easy, and self contained.

Created attachment 99239 [details] [review]
Allow an art searcher to directly return image data

Apologies for the garbage, apparently colordiff doesn't strip that out when directing to a file.

I do not much like the idea of storing the cover in RhythmDB. If we were to do so, it would have to be a scaled-down version to prevent songs with unusually large embedded art from rendering the exported DB a massive mess. I also do not like the idea of writing out covers to the music folder as (1) seems to suggest, since if the music was organized one-album-per-folder reading the embedded art wouldn't be needed anyway.

If the cover is to be cached, there needs to be a way to map the cover to the song even if the song contains no metadata aside from the cover. Ideally, Rhythmbox should also prevent a single cover from being cached more than once. Perhaps a (Song SHA-1) -> (Cover SHA-1) -> Cached file mapping, but that seems a lot of additional effort.

Created attachment 99240 [details] [review]
Read album art embedded in id3v2 or COVERART tags

(In reply to comment #9)
> Created an attachment (id=99239) [edit]
> Allow an art searcher to directly return image data
> 
> Apologies for the garbage, apparently colordiff doesn't strip that out when
> directing to a file.
> 
> I do not much like the idea of storing the cover in RhythmDB. If we were to do
> so, it would have to be a scaled-down version to prevent songs with unusually
> large embedded art from rendering the exported DB a massive mess. I also do not
> like the idea of writing out covers to the music folder as (1) seems to
> suggest, since if the music was organized one-album-per-folder reading the
> embedded art wouldn't be needed anyway.

No, 1) would write it at the same location as the other cached covers downloaded from Amazon for example, so "~/.gnome2/rhythmbox/covers/<artist name> - <album name>.jpg"

The problem with your current way is that care has been taken to avoid running the metadata processing in the same process as Rhythmbox to avoid crashes, memory gobbling and cpu burning. Having to do the processing again, after having imported the file, seems like a waste of resources.

Having the cover processing in the metadata processing means that we could also add support for setting the coverart in formats that support it.

I've no objection to pre-extracting the cover along with the other metadata, only with storing it in the DB export file. This extension to the artdisplay plugin is not intended to be a permanent solution, only a hold-over until something more proper can be implemented. I do not want to wait more months and months until I can finally view album art for my music.

Also, I think the "<artist> - <album>.jpg" scheme is fundamentally broken. Not all songs have a full set of tags, or have all tags normalized ("Guns and Roses" v. "Guns & Roses", for example). There needs to be some way to link the cover to a song by some other means, such as the hash chain in #9.

(In reply to comment #12)
> I've no objection to pre-extracting the cover along with the other metadata,
> only with storing it in the DB export file. This extension to the artdisplay
> plugin is not intended to be a permanent solution, only a hold-over until
> something more proper can be implemented. I do not want to wait more months and
> months until I can finally view album art for my music.

It should be pretty straight forward to implement. Only thing would be it needing to be implemented in C.

> Also, I think the "<artist> - <album>.jpg" scheme is fundamentally broken. Not
> all songs have a full set of tags, or have all tags normalized ("Guns and
> Roses" v. "Guns & Roses", for example). There needs to be some way to link the
> cover to a song by some other means, such as the hash chain in #9.

And what would you hash? Hashing the location of the files isn't interesting, as you'd need to re-extract the file if the location changed. The filename matches what's currently used for caching covers, and works as well as the other potential sources. In all cases, the filenames would match whatever people have in their library, which is what's important.

You're missing that many people, including myself, have songs with incomplete tags. I don't want part of my library to all have the same image because it's reading a cached "Unknown - Unknown.jpg", when they have correct embedded covers.

Hash the contents of the cover. Store the cover hash in the RhythmDB, and name the file in ~/.gnome2/rhythmbox/covers/ after the hash. If a media file's location was changed, it would have to be re-extracted anyway, so that's not a big problem.

Maybe we could take a look at how some other open-source projects handle this.  I looked into Amarok's cover handling some time back, and I noticed that they have quite a sophisticated cover manager which generates a unique string and is unified to handle both cover art in ID3 tags as well lookups vis Amazon or MusicBrainz.

I'm a little surprised that John implies in comment 6 that he has files with embedded covers, but missing album/artist tags.  Never the less, this is an pathological example RB should cope with.

Right now the offered patch hooks into the existing art display plugin as an additional search method.

Can I suggest an alternative model?  I am under the impression that while playing a track, if and when there is an embedded image, gstreamer signals this to RB with some sort of message/event.  The art display plugin needs to be able to catch this event, abort any outstanding searches, and simple fetch the embedded image from gstreamer and use that.

i.e. Embedded art is used as and when gstreamer tells RB it exists; and this replaces any image the plugin was using.  In this scheme there is no need to cache anything, or extend the database.

However, this does not help any future enhancement which may want to show cover images in the album browser.

>I'm a little surprised that John implies in comment 6 that he has files with
embedded covers, but missing album/artist tags.  Never the less, this is an
pathological example RB should cope with.

The most common case for this is when the tags have been stripped out by some poorly-coded encoding software, but the covers left in the music's directory. It's certainly a pathological case, but the normal ones are dealt with easily by existing local art search code.

>Can I suggest an alternative model?  I am under the impression that while
playing a track, if and when there is an embedded image, gstreamer signals this
to RB with some sort of message/event.  The art display plugin needs to be able
to catch this event, abort any outstanding searches, and simple fetch the
embedded image from gstreamer and use that.

GStreamer supports id3v2 tags, but not Vorbis/FLAC style (which is the majority of my library). Ideally support for these tags would be written into the Vorbis plugin itself, but I don't know enough about GStreamer to do so.

Interestingly, even though GStreamer *does* support id3v2 art, Rhythmbox does not pick it up. Perhaps the signal trigger is broken.

So what is the status on this, will it be releaset in the artdisplay plugin?
I have almost all my library with covers inside the music files (mostly mp3) and would really need this feature.

Created attachment 102191 [details]
Rhythmbox with attached patches applied

Screenshot of Rhythmbox from SVN 5531 with these patches applied.

On Rhythmbox built on Ubuntu 7.10 with these patches, the following Traceback appears on the console:

Traceback (most recent call last):

+ Trace 184116

(rhythmbox:14400): Rhythmbox-WARNING **: Could not load plugin artdisplay


(rhythmbox:14400): Rhythmbox-WARNING **: Error, impossible to activate plugin 'Cover art'

Definately a hacky solution, but if I comment out the lines on 52-53 in /usr/lib/python2.5/site-packages/pygst.py 

# if sys.modules.has_key('gst'):
#     raise RequiredVersionError, "pygst.require() must be called before importing gst"

The code actually works :)

However, when playing a file with a Cover added with EasyTag 2.1.2, the cover looks garbled -- as seen in the attached screenshot.

Looks like a really nice feature though, especially if the garbled cover issue can be fixed.

Created attachment 102195 [details]
Rhythmbox showing garbled ID3 Coverart

Created attachment 102196 [details] [review]
Read album art embedded in id3v2 or COVERART tags (v2)

> On Rhythmbox built on Ubuntu 7.10 with these patches, the following
> Traceback appears on the console:
> ...
> pygst.RequiredVersionError: pygst.require() must be called before importing gst

It seems PyGST assumes that it will only have its version checked in one place in an entire program. I've worked around this by checking if 'gst' is in sys.modules (the same check PyGST does), but ultimately it might be a good idea to contact the PyGST team and ask them why they choose to do this.

Updated version, uses above mentioned sys.modules hack and also uses gst.STATE_PAUSED to find metadata. Previously, I used STATE_PLAYING, but that would cause high CPU usage if a file was encountered with no embedded art. Thanks to the Rhythmbox metadata extractor for serving as a better example of how to extract metadata.

Auugh, you people, you post too quickly!

> However, when playing a file with a Cover added with EasyTag 2.1.2, the cover
> looks garbled -- as seen in the attached screenshot.

> Looks like a really nice feature though, especially if the garbled cover issue
> can be fixed.

I believe this to be a bug in EasyTag, or in the ID3 library it uses. It seems to perform some kind of escaping of the APIC image data not called for in the ID3 spec, which results in mangled art. A few images are damaged in such a way that GdkPixbuf can parse them anyway, though obviously with erroneous results.

My solution was to use another tagging program (iTunes), which outputs correct APIC tagging. I've not tried embedding .PNG art using EasyTag, so that might be another solution for you.

(In reply to comment #24)
> Auugh, you people, you post too quickly!
> 
> > However, when playing a file with a Cover added with EasyTag 2.1.2, the cover
> > looks garbled -- as seen in the attached screenshot.
> 
> > Looks like a really nice feature though, especially if the garbled cover issue
> > can be fixed.
> 
> I believe this to be a bug in EasyTag, or in the ID3 library it uses. It seems
> to perform some kind of escaping of the APIC image data not called for in the
> ID3 spec, which results in mangled art. A few images are damaged in such a way
> that GdkPixbuf can parse them anyway, though obviously with erroneous results.
> 
> My solution was to use another tagging program (iTunes), which outputs correct
> APIC tagging. I've not tried embedding .PNG art using EasyTag, so that might be
> another solution for you.
> 

Bug in Easytag i'd suspect, I just installed EyeD3 and tried editing a file that way (same libid3tag library) and the cover works fine that way.

I should add, all the cover art i've added so far has been from Amazon in their 240x240 JPEG format too, rather than PNGs.

Created attachment 102198 [details] [review]
Updated Makefile.am to include EmbeddedArtSearch.py

Just tried the newer patch, the pygtk error doesn't appear now -- but neither does the coverart.

On a file without embedded artwork, I get:

(17:34:38) [0x80dd408] [EmbeddedArtSearch.search] /usr/lib/rhythmbox/plugins/artdisplay/EmbeddedArtSearch.py:220: searching for embedded art for <gnomevfs.URI 'file:///home/paul/My%20Music/Infected%20Mushroom%20-%20Vicious%20Delicious/(01)%20Infected%20Mushroom%20-%20Becoming%20Insane.mp3'>
(17:34:38) [0x80dd408] [on_message] /usr/lib/rhythmbox/plugins/artdisplay/EmbeddedArtSearch.py:81: Finished metadata with no cover art

However, on a file with a known good 240x240 Cover (Picture Type $03) I get:

(17:41:38) [0x80dd408] [EmbeddedArtSearch.search] /usr/lib/rhythmbox/plugins/artdisplay/EmbeddedArtSearch.py:220: searching for embedded art for <gnomevfs.URI 'file:///home/paul/My%20Music/Motorhead%20-%20Ace%20Of%20Spades%20(Re-Issued%20with%20Bonus%20Tracks)/01%20-%20Mot%C3%B6rhead%20-%20Ace%20of%20Spades.mp3'>
(17:41:38) [0x80dd408] [on_message] /usr/lib/rhythmbox/plugins/artdisplay/EmbeddedArtSearch.py:99: Got cover art in 'image' tag
(17:41:38) [0x80dd408] [LocalCoverArtSearch.search] /usr/lib/rhythmbox/plugins/artdisplay/LocalCoverArtSearch.py:108: searching for local art for file:///home/paul/My%20Music/Motorhead%20-%20Ace%20Of%20Spades%20(Re-Issued%20with%20Bonus%20Tracks)/01%20-%20Mot%C3%B6rhead%20-%20Ace%20of%20Spades.mp3

I just tested the newer patch also and shows no errors but as the last post says neither the cover art.

The command line output is exactly as the last post and I've tested it with mp3 files that has cover art from iTunes, in Banshee getting the cover artfor those files works tho..

I am unable to replicate this new bug, using either 0.11.2 or SVN 5531. Would it be possible for you to create an empty mp3 file (60 seconds of silence, say), apply your cover art to it, and then upload the resulting test file?

Also, please ensure you have applied the "Allow an art searcher to directly return image data" patch. I don't know what would happen without it, but depending on the error handling in CoverArtDatabase the search might simply skip to the next engine.

sweet!
It works with my cover art's created by iTunes.

What I did was go to rhythmbox page, download the source for my 0.11.2 Rhythmbox that I have installed in Gusty, replaced the files I already have for the artdisplay plugin with the ones downloaded and then applied the 2 patches ("Allow an art searcher to directly return image data", "Updated Makefile.am to include EmbeddedArtSearch.py")

Now everything seems to work just fine!

Output from the terminal:
mp3 with cover art:
(20:14:39) [0x80fb408] [EmbeddedArtSearch.search] /usr/lib/rhythmbox/plugins/artdisplay/EmbeddedArtSearch.py:220: searching for embedded art for <gnomevfs.URI 'file:///home/mihai007/Music/2%20Pac/Greatest%20Hits%20CD%201/09%20Unconditional%20Love.mp3'>
(20:14:39) [0x80fb408] [on_message] /usr/lib/rhythmbox/plugins/artdisplay/EmbeddedArtSearch.py:99: Got cover art in 'image' tag


mp3 without cover art:
(20:17:52) [0x80fb408] [EmbeddedArtSearch.search] /usr/lib/rhythmbox/plugins/artdisplay/EmbeddedArtSearch.py:220: searching for embedded art for <gnomevfs.URI 'file:///home/mihai007/Music/Gentelman/Mp3/00.%20Superior.mp3'>
(20:17:52) [0x80fb408] [on_message] /usr/lib/rhythmbox/plugins/artdisplay/EmbeddedArtSearch.py:81: Finished metadata with no cover art
(20:17:52) [0x80fb408] [LocalCoverArtSearch.search] /usr/lib/rhythmbox/plugins/artdisplay/LocalCoverArtSearch.py:108: searching for art local to
(20:17:52) [0x80fb408] [LocalCoverArtSearch.search] /usr/lib/rhythmbox/plugins/artdisplay/LocalCoverArtSearch.py:108:  
(20:17:52) [0x80fb408] [LocalCoverArtSearch.search] /usr/lib/rhythmbox/plugins/artdisplay/LocalCoverArtSearch.py:108: file:///home/mihai007/Music/Gentelman/Mp3/00.%20Superior.mp3

Paul, I recommend that you do the same and see what happens

Created attachment 102235 [details]
Sample track with cover art in 'image' tag

This is one of my brother's remixes that used to create one of their songs and in his iTunes we dragged an bmp cover art for the image but I think that iTunes converts that to jpg pefore putting it into the mp3 tag but i'm not shure about that. The thing is that I imported the sample in my Rhythmbox and cover art just worked.

The new patch does work, I recompiled trunk (5533), removed my .gnome2/rhythmbox/ directory and rebooted the machine.

Fired up Rhythmbox and the covers appear normally (except those tagged by Easytag, as previously mentioned).

Nice work John :)

Ok after all those days testing different songs, the plugin never failed.

But just as a note, there are some songs that makes the plugin show this in the terminal:

(18:31:04) [0x80fb408] [EmbeddedArtSearch.search] /usr/lib/rhythmbox/plugins/artdisplay/EmbeddedArtSearch.py:220: searching for embedded art for <gnomevfs.URI 'file:///home/mihai007/Music/tmp/Unknown%20Artist/Unknown%20Album/21%20Mystikal%20-%20Smashing%20the%20Gas%20(Get.mp3'>
Traceback (most recent call last):

+ Trace 184835

(18:31:05) [0x80fb408] [on_message] /usr/lib/rhythmbox/plugins/artdisplay/EmbeddedArtSearch.py:81: Finished metadata with no cover art
(18:31:05) [0x80fb408] [LocalCoverArtSearch.search] /usr/lib/rhythmbox/plugins/artdisplay/LocalCoverArtSearch.py:108: searching for art local to
(18:31:05) [0x80fb408] [LocalCoverArtSearch.search] /usr/lib/rhythmbox/plugins/artdisplay/LocalCoverArtSearch.py:108:  
(18:31:05) [0x80fb408] [LocalCoverArtSearch.search] /usr/lib/rhythmbox/plugins/artdisplay/LocalCoverArtSearch.py:108: file:///home/mihai007/Music/tmp/Unknown%20Artist/Unknown%20Album/21%20Mystikal%20-%20Smashing%20the%20Gas%20(Get.mp3


Well depite of the message the plugin works well but I just wanted to mention this, is it normal?

Created attachment 102569 [details] [review]
Read album art embedded in id3v2 or COVERART tags (v3)

It's normal in the sense that it's expected and not dangerous - not all extended comments are in the expected key=value format. In this case, it's doubly not a problem - MP3 files have cover art support in GStreamer itself, so they don't depend on metadata hacks like Ogg/Vorbis does.

Here's an updated version that will print a nicer error message for you when it fails to parse extended comments.

yes, now it works great.
Great job!

I hope those patches will get into the final rhythmbox plugin, as this bug now got it's fix.

Been running v3 for a few days now and have no issues with it, it prints a decent error message on broken extended comments and returns art much faster than v1 -- looks really good.

Looking forward to seeing these in SVN soon.

Created attachment 104170 [details] [review]
Extract album art and store URI in Rhythmdb

New patch, which implements this in Rhythmbox's existing GST metadata extractor rather than a plugin.

Created attachment 104769 [details] [review]
Extract album art and store URI in Rhythmdb

Slight update from above - modifies the cover art plugin to avoid caching covers if their URI starts with file://.

Builds on an Ubuntu box running Hardy, but because:

hexdigest = g_compute_checksum_for_data (G_CHECKSUM_SHA1, buffer->data,
	                                         buffer->size);

depends on GLib 2.16, it might be a while before this gets any reasonable testing (my desktop boxes are all still glib 2.12 or 2.14)

Please add this to the next release....

I also think that this patch is quite stable, and it should be added into the next release.

Indeed, i've found this patch quite stable building against GARNOME recently -- it should certainly go in when the other glib 2.16 dependant stuff goes in.

There are a couple of things I don't like about this.

- saves stuff to disk without me asking it to
- doesn't work for things that aren't imported into the library (daap/upnp, mostly)
- adds yet another field to RhythmDBEntry that is likely to be sparsely used
- the extracted image filename scheme doesn't seem right, but I can't put my finger on what's wrong with it

rb_get_extension_for_mime_type probably works for just about all formats as it is, but gstreamer media types (as returned by gst_structure_get_name) are not mime types, even though they look like they are.  I'm not sure what the alternative is.

> There are a couple of things I don't like about this.
> 
> - saves stuff to disk without me asking it to
> 
The alternative seems to be re-extracting the cover art every time a track is played. This is the method my old plugin-based patch used, and most of the commenters agreed that this was a bad idea. I suppose the art could be encoded, sent over D-BUS to the main Rhythmbox process and then cached in RAM, but that seems wasteful and inefficient to me.

> - doesn't work for things that aren't imported into the library (daap/upnp,
mostly)
> 
If I read the DAAP/UPNP code correctly, metadata is provided by the server and not actually read from the streamed media, right? If so, then metadata handling for DAAP/UPNP and local files is entirely separate.

> - adds yet another field to RhythmDBEntry that is likely to be sparsely used
> 
Usage pattern will obviously depend on whether the user has added album art or not. If the number of fields in a RhythmDBEntry structure is an issue, perhaps it could be modified to have an "extra data" section which can store rarely-used field values.

> - the extracted image filename scheme doesn't seem right, but I can't put my
finger on what's wrong with it
> 
The only issue I have with it is that the art for a particular album can't be easily determined - you'd have to scan the album for the first track to contain art. Aside from that, the digest-based filenames automatically eliminate duplicates and prevent mis-tagged files from having conflicting art.

(In reply to comment #39)
> Created an attachment (id=104769) [edit]

Patch applies against trunk today (r5628) with --ignore-whitespace and with manual fixup of the indentation in ./plugins/artdisplay/artdisplay/CoverArtDatabase.py.

I'm interested in this patch to see the per-track art embedded in the new NIN release "Ghosts" - ghosts.nin.com. This is a good test case because each track has a unique, 1000x1000 APIC, meaning you need scaling and per-track art stuff, but also track 01 has TWO APICs, one for the album and one for the track. This patch shows the album one. I'm not sure if there's any metadata/convention/etc. that stipulates that the second one should be picked out or not, etc.

the first 9 tracks of ghosts are CC-BY-SA-NC so you anyone can download them to test the patch.

This patch no longer seems to work against HEAD (5690), all my local MP3 files have embedded coverart (both as '$03' and '$00' so they are embedded in gtkpod, and mediamonkey and itunes on win32).

Is there anything else that needs to be applied to make this work, or does it need an update? (Am I right in assuming the patch in comment #6 doesn't need to be applied with this particular version, because that particular patch has been obseleted)

The only patch you should need is the one in comment #39. As far as I know this still applies to trunk -- I would think that any changes in SVN that broke the patch would cause it to outright not compile. If you're applying the patch in #39 to a clean trunk and it doesn't work, I'll try to poke it a bit and see what's up.

That's exactly what i'm doing John.

Clean checkout of 5670, apply your patch in #39, which applies cleanly with slight offsets -- autogen && make && make install then run, but it seems to skip any embedded artwork and go straight to amazon to try and download them.

Paul,

Just a quick idea... are you sure that you are executing the correct executable.  For the longest time, I had a rhythmbox package installed from my OS (Ubuntu Gutsy, at the time), while I was compiling a rhythmbox executable into another folder.

Personally, I like to:
./autogen.sh --prefix=/home/daiker/gnome-svn && make && sudo make install

My $0.02

As for the patch not working... I have never had it work correctly in the past, IIRC.

John

I'm pretty positive, I actually build clean checkouts of rhythmbox on a weekly-ish basis on a sandbox I use for package maintainence and turn them into actual packages to install over my standard distribution ones if they do what I expect them to (fedora and ubuntu).

I had the 'old-style' version of this patch working, the new one that uses the metadata extraction style patches and compiles cleanly, but doesn't actually seem to work.

Back to the original question, is there a -D "something" line I can use to look for a debugging string that would help, my library is over 10000 tracks, which makes it painful to just run rhythmbox with -d.

So, the patch that uses metadata extraction has never worked for you? It's working on my system with r5670.

It could be that the metadata extractor simply needs to be run first. Try updating the mtime of some music with "touch", and then see if the art is displayed properly.

If that fails, please check in $HOME/.gnome2/rhythmbox/covers to see if the art was extracted at all.

I removed everything from .gnome2/rhythmbox/covers and played a single with embedded coverart, I then had:

"Artist - Album Name".rb-blist with the content "AmazonCoverArtSearch"

(Which it couldn't find, incidently, because the album is out-of-print now)

I removed that file and went and touch'ed the MP3 file and tried playing it again, which gives me a file called:

5bf2d9f5672185f21c8654962db8e311a8a45dc.jpeg

but no cover was displayed.

Trying another, untouched but known good track with a known good cover downloaded:

Flunk - Morning Star.jpg

and displayed that.

I then quit rhythmbox, touch'ed the directory the file was in, re-played the file -- and the cover appeared correctly!

I've now run " touch * && touch */* " in my music folder, and re-started rhythmbox to find that the covers/ directory is now rapidly filling up with jpeg files of the coverart for each of my folders.

All the dates on all files and directories are now Wed Apr 30 12:20PM and it seems to be happy displaying all the correct artwork now.

Apologies for doubting you, I guess :)

Just tried this patch against the 0.11.6 release and it seems to patch correctly, although I had to run the touch hack in my above comment to make it start displaying the embedded artwork again.

Any chance this patch can be integrated?

Actually, make that 'most' of the music seems to display the embedded artwork.

John, If I were to add debugging information on the success of metadata extraction, would it be best to add it after:

+   /* Calculate the checksum of the cover art's contents. This way,
+    * multiple copies of the same art are not cached.
+   **/
+   hexdigest = g_compute_checksum_for_data (G_CHECKSUM_SHA1, buffer->data,
+                                            buffer->size);
+   file_name = rb_cover_art_build_cache_file_name (hexdigest, mime_type);
+   g_free (hexdigest);


Or somewhere else, because it seems to fail on a different album each time -- from a test run of 704 albums, doing an 'rm -rf .cache/rhythmbox/covers' each time, I get 224, 308 and 665 in three runs -- and am trying to figure out any common failures.

Created attachment 114327 [details] [review]
Updated, Semi-Working Patch for 0.11.6

This patch is slightly updated to work with the changes in SVN 5748 by adding _emd_ to the relevant places, but is otherwise the same as the existing one.

After some additional testing, it seems the failures are all in music that has only 'Cover' art.

Around a third of my music collection does this, the rest has the front cover as 'Cover' and 'Other' (which iTunes uses anyway) -- and this patch works flawlessly on those.

Infact, if I remove the offending third from the library, the other two-thirds come up exactly (816 albums, 816 covers).

Can anyone else test this patch and say if it has the same behaviour for them?

If there is consistent failure for specific files, I would guess the issue lies not in the Rhythmbox patch, but in GStreamer's cover extraction. The RB patch does not differentiate between cover art based on type (it should, but does not currently). Are other media players able to extract/display the art correctly? If so, could you please create a sample media file with embedded art that fails, so I can test it? Just generate a second of silence in Audacity or something.

Something to note: if you are adding covers to mp3 files with Easytag, there is a known issue that causes invalid cover art to be written to the file. Such incorrect files cannot be read by any media software I've tested with, including iTunes.

Lastly, instead of re-touching all your media files, it might be easier to run with a separate database file using "rhythmbox --no-registration --dry-run --rhythmdb-file=test-db.xml", which will always re-scan your library.

I knew about the bug in Easytag, which seems to be fixed in the latest release 2.1.5, but this seems to drop out regardless of which tagger I use (iTunes, GTKPod, mediamonkey or Easytag)

Banshee is the only other music player software I have ready-access to, but i'll make some tests with iTunes next time I have access to a Windows box and update you here.

Just tested a Windows box, Mediamonkey (3.0.3) and iTunes (7.6) displays the covers with 'Cover' or 'Other' or Both on files tagged with Easytag.

One other thing I forgot to mention in the last report was that it happens on a random file each time, the only constant issue seems to be that it always fails on a file that _only_ has a 'Cover' tag.

Banshee 1.0 doesn't seem to support embedded coverart, so I can't test if it's a GStreamer problem or one with this patch (I'm on Ubuntu 8.04, with GStreamer 0.10.17).

Try putting a printf in rb_metadata_gst_load_tag() in the if(tag=="image") case. If it triggers for a file but the cover does not display, the error is in my code. If it does not trigger for a file, the error is (probably) in GStreamer.

Odd, I can't duplicate it with an empty database.

I added:


---
+	if (strcmp (tag, "image") == 0)
+	{
+		const GValue *image_value;
+		image_value = gst_tag_list_get_value_index (list, tag, 0);
+		rb_metadata_load_gst_image_tag (image_value, md);
+		rb_debug ("image extracted correctly, #345975");
+		return;
+	}
+	
---

To make sure, I removed the .cache/rhythmbox/covers directory.

Then started Rhythmbox with:

rhythmbox --debug --no-registration --dry-run --rhythmdb-file=test-db.xml > rb-startup.txt 2>&1

I imported two albums to the empty database, one tagged with Easytag 2.1.5 and one with iTunes 7.7, with a 'Cover' image but no 'Other' image -- both albums had no displayed covers before -- but now I get:

---
(17:05:27) [0x8062440] [rb_metadata_gst_load_tag] rb-metadata-gst.c:748: uri: file:///home/paul/My%20Music/MP3/Elastica/Elastica/01%20-%20Line%20Up.mp3 tag: image 
(17:05:27) [0x8062440] [rb_dot_dir] rb-file-helpers.c:100: unable to create Rhythmbox's dot dir
(17:05:27) [0x8062440] [rb_metadata_load_gst_image_tag] rb-metadata-gst.c:736: Got cover art URI: file:///home/paul/.gnome2/rhythmbox/covers/9f47656f48c3fe38cb162586dcc5bc917ce11e32.jpeg
(17:05:27) [0x8062440] [rb_metadata_gst_load_tag] rb-metadata-gst.c:762: image extracted correctly, #345975
(17:05:27) [0x8062440] [rb_metadata_bus_handler] rb-metadata-gst.c:1057: message of type state-changed from flump3dec0
---

Loading up the normal database for the same file:

---
(17:51:50) [0x8062440] [rb_metadata_gst_load_tag] rb-metadata-gst.c:748: uri: file:///home/paul/My%20Music/MP3/Elastica/Elastica/01%20-%20Line%20Up.mp3 tag: image 
(17:51:50) [0x8062440] [rb_dot_dir] rb-file-helpers.c:100: unable to create Rhythmbox's dot dir
(17:51:50) [0x8062440] [rb_metadata_bus_handler] rb-metadata-gst.c:1057: message of type state-changed from flump3dec0
---

Could that mean there's some sort of database corruption going on?

Created attachment 115160 [details] [review]
Patch used to test comment #62

As a theory, i've just tried one of the failing directories with the new version of EasyTAG (2.1.6), which "applied automatic corrections to file" when I loaded the directory, I then added the 'Other' image (it was previously a directory with only 'Cover' image) and saved the files -- now I get:

From a blank database, using the command line from comment 62:

---
(22:06:01) [0x80de408] [rhythmdb_process_one_event] rhythmdb.c:2163: processing RHYTHMDB_EVENT_STAT
(22:06:01) [0x80de408] [rhythmdb_process_stat_event] rhythmdb.c:1809: queuing a RHYTHMDB_ACTION_LOAD: file:///home/paul/My%20Music/MP3/Wall%20Of%20Voodoo%20-%20Dark%20Continent/08%20-%20Call%20Box%20(1-2-3).mp3
(22:06:01) [0x80de408] [rhythmdb_process_one_event] rhythmdb.c:2163: processing RHYTHMDB_EVENT_STAT
(22:06:01) [0x80de408] [rhythmdb_process_stat_event] rhythmdb.c:1809: queuing a RHYTHMDB_ACTION_LOAD: file:///home/paul/My%20Music/MP3/Wall%20Of%20Voodoo%20-%20Dark%20Continent/03%20-%20Animal%20Day.mp3
(22:06:01) [0x80de408] [rhythmdb_process_one_event] rhythmdb.c:2163: processing RHYTHMDB_EVENT_STAT
(22:06:01) [0x80de408] [rhythmdb_process_stat_event] rhythmdb.c:1809: queuing a RHYTHMDB_ACTION_LOAD: file:///home/paul/My%20Music/MP3/Wall%20Of%20Voodoo%20-%20Dark%20Continent/11%20-%20Crack%20The%20Bell.mp3
(22:06:01) [0x80de408] [rhythmdb_process_one_event] rhythmdb.c:2163: processing RHYTHMDB_EVENT_STAT
(22:06:01) [0x80de408] [rhythmdb_process_stat_event] rhythmdb.c:1809: queuing a RHYTHMDB_ACTION_LOAD: file:///home/paul/My%20Music/MP3/Wall%20Of%20Voodoo%20-%20Dark%20Continent/07%20-%20Tse%20Tse%20Fly.mp3
(22:06:01) [0x80de408] [rhythmdb_process_one_event] rhythmdb.c:2163: processing RHYTHMDB_EVENT_STAT
(22:06:01) [0x80de408] [rhythmdb_process_stat_event] rhythmdb.c:1809: queuing a RHYTHMDB_ACTION_LOAD: file:///home/paul/My%20Music/MP3/Wall%20Of%20Voodoo%20-%20Dark%20Continent/10%20-%20Good%20Times.mp3

... [ snipped ] ...

(22:06:01) [0x8062440] [rb_metadata_gst_load_tag] rb-metadata-gst.c:748: uri: file:///home/paul/My%20Music/MP3/Wall%20Of%20Voodoo%20-%20Dark%20Continent/08%20-%20Call%20Box%20(1-2-3).mp3 tag: image 
(22:06:01) [0x8062440] [rb_dot_dir] rb-file-helpers.c:100: unable to create Rhythmbox's dot dir
(22:06:01) [0x8062440] [rb_metadata_load_gst_image_tag] rb-metadata-gst.c:736: Got cover art URI: file:///home/paul/.gnome2/rhythmbox/covers/5a1e540fd24dd2d32e2e3f3dd32da24b12947a7d.jpeg
(22:06:01) [0x8062440] [rb_metadata_gst_load_tag] rb-metadata-gst.c:762: image extracted correctly, #345975
---

On the normal database, it now says:

---
(22:06:46) [0x80de408] [rhythmdb_process_one_event] rhythmdb.c:2163: processing RHYTHMDB_EVENT_STAT
(22:06:46) [0x80de408] [rhythmdb_process_stat_event] rhythmdb.c:1788: not modified: file:///home/paul/My%20Music/MP3/Wall%20Of%20Voodoo%20-%20Dark%20Continent/07%20-%20Tse%20Tse%20Fly.mp3
(22:06:46) [0x80de408] [rhythmdb_process_one_event] rhythmdb.c:2163: processing RHYTHMDB_EVENT_STAT
(22:06:46) [0x80de408] [rhythmdb_process_stat_event] rhythmdb.c:1788: not modified: file:///home/paul/My%20Music/MP3/Wall%20Of%20Voodoo%20-%20Dark%20Continent/08%20-%20Call%20Box%20(1-2-3).mp3
(22:06:46) [0x80de408] [rhythmdb_process_one_event] rhythmdb.c:2163: processing RHYTHMDB_EVENT_STAT
(22:06:46) [0x80de408] [rhythmdb_process_stat_event] rhythmdb.c:1788: not modified: file:///home/paul/My%20Music/MP3/Wall%20Of%20Voodoo%20-%20Dark%20Continent/04%20-%20Full%20of%20Tension.mp3


... [ snipped ] ...

(22:06:57) [0x80de408] [new_playing_stream_idle_cb] rb-shell-player.c:3491: new playing stream: file:///home/paul/My%20Music/MP3/Wall%20Of%20Voodoo%20-%20Dark%20Continent/04%20-%20Full%20of%20Tension.mp3
(22:06:57) [0x80de408] [rb_shell_hidden_notify_markup] rb-shell.c:3101: shell is visible, not notifying
(22:06:57) [0x80de408] [CoverArtDatabase.get_pixbuf] /usr/lib/rhythmbox/plugins/artdisplay/CoverArtDatabase.py:311: Could not load pixbuf from specified URI: file:///home/paul/.gnome2/rhythmbox/covers/5a1e540fd24dd2d32e2e3f3dd32da24b12947a7d.jpeg
(22:06:57) [0x80de408] [LocalCoverArtSearch.search] /usr/lib/rhythmbox/plugins/artdisplay/LocalCoverArtSearch.py:141: searching for local art for file:///home/paul/My%20Music/Downloaded/MP3/Wall%20Of%20Voodoo%20-%20Dark%20Continent/04%20-%20Full%20of%20Tension.mp3
(22:06:57) [0x80de408] [rb_shell_player_sync_with_source] rb-shell-player.c:2947: playing source: 0x8315e90, active entry: 0xb45f3320
(22:06:57) [0x80de408] [rb_shell_set_window_title] rb-shell.c:2151: setting title to "Wall Of Voodoo - Full of Tension"
---

Created attachment 115801 [details] [review]
Updated Patch for GIO-enabled Rhythmbox

Ammended patch that works with SVN (5832) and extracts cover art correctly

I've had success with the patch from comment #65, which I altered to use g_get_user_cache_dir, instead of the old way -- from a test sample of 200 albums, I get 200 embedded cover art files, which are also uploaded to my iPod if I also use the code in #529863

For some reason, some of the files still appear in ~/.gnome2/rhythmbox/covers -- but I haven't managed to track that down yet.

The call to gnome_vfs_get_uri_from_local_path in rb_metadata_load_gst_image_tag needs to be replaced.

Yep, it does -- don't know how I missed that.

Updated patch forthcoming :)

Created attachment 116400 [details] [review]
Updated Patch against 5852

Updated GIO-supported patch, with gnome_vfs_get_uri_from_local_path replaced -- which correctly stores covers in .cache/rhythmbox/covers.

Created attachment 117826 [details] [review]
Updated Patch against 5874

I've changed _EMD_ to be 'embedded' throughout the patch to make it more readable and rediffed it against current SVN.

Extracts all the embedded album art I have correctly to ~/.cache/rhythmbox/covers

Created attachment 119749 [details] [review]
Updated Patch against 5957

Updated against a more recent SVN snapshot, still works flawlessly on artwork stored as 'Cover' or 'Other' from a variety of taggers.

Created attachment 119775 [details] [review]
another approach

This extracts embedded images during playback instead.  I'm not sure how this should interact with the existing cover art search code, or with the cover art cache.  The current situation probably isn't very good - it'll start and then cancel the cover art search every time a file with embedded images is played.  I don't know.

Your patch works very well for me, with one tweak -- check for GST_TAG_PREVIEW_IMAGE as well as GST_TAG_IMAGE in the backend message handlers.

An idea for the search cancelling issue: Launch local searches immediately, but remote searches after 100ms timeout or so. Not a big delay compared to how slow remote searches are, and it prevents unneeded searches.

What is the status of this? Is there any review planned for the patches?

Created attachment 130520 [details] [review]
cleaned up and updated

Created attachment 132673 [details] [review]
add embedded cover art search for non-playing entries 

This adds a new search module to the cover art plugin that uses gstreamer to extract embedded cover art.  It only searches local files, and it only runs if the entry being searched for is not currently playing.  This allows us to use embedded cover art when transferring to devices.

Another user wondering if this will be available soon.  I have spent many, many, many hours embedding the correct art to my 60GB music collection under iTunes, and now that I'm using Linux again it sucks Rhythmbox can't use the art in the MP3 files.

Thank you!

It depends how much you want it.  This feature is now available in git master.

commit dce0829835741b0562ed09726473892bca41a05c
Author: Jonathan Matthew <jonathan@d14n.org>
Date:   Sat Apr 18 02:15:23 2009 +1000

    artdisplay: add embedded cover art search
    
    This uses a GStreamer pipeline to read embedded images.  Since the
    player will already read images for the playing entry, it only runs for
    entries that are not playing.  It only operates on local files.
    
    Fixes the rest of #345975.

commit bfdd5663d51955d55e46588f369a815038fba214
Author: Jonathan Matthew <jonathan@d14n.org>
Date:   Sat Apr 18 02:12:42 2009 +1000

    artdisplay: allow searches to return pixbufs directly
    
    This allows cover art searches to return pixbuf objects as well as URIs
    for cover art images.  Also passes through a flag indicating whether the
    entry being searched for is currently playing.

commit bd4c4919da0ccf7abe46bce905eab7c62d855e7f
Author: Jonathan Matthew <jonathan@d14n.org>
Date:   Sat Apr 18 02:11:16 2009 +1000

    xfade: only process tags for a stream once it has prerolled

commit 95d5ed9fb1fe752440c6a60ee950e0fd8e3628a7
Author: Jonathan Matthew <jonathan@d14n.org>
Date:   Sat Apr 18 02:03:59 2009 +1000

    process embedded images received during playback
    
    - add a new signal on the RBPlayer interface to make embedded images
      available as GdkPixbufs
    - move tag processing code to rb-player-gst-helper.c, add code to handle
      converting images from GstBuffers to GdkPixbufs
    - process GST_TAG_IMAGE and GST_TAG_PREVIEW_IMAGE tags, emitting the new
      'image' signal
    - emit rb:coverArt extra metadata notification when an image is received
      from the player, causing the art display plugin to use the embedded
      image.
    
      Fixes half of #345975.

commit 5379d240c35ec51bd3e221fe5d3abdd7cde465b5
Author: Jonathan Matthew <jonathan@d14n.org>
Date:   Sat Apr 18 01:29:37 2009 +1000

    shell-player: stop using idle handlers for processing new streams
    
    This is pretty much unnecessary, and any audio output problems that
    we were avoiding by using idle handlers and timeouts appears to have
    mostly been fixed.  UI is a bit snappier on track changes now too.

commit b8b806365d7196c0f3ded057feb720f9d5ad8ba6
Author: Jonathan Matthew <jonathan@d14n.org>
Date:   Sat Apr 18 00:48:59 2009 +1000

    player-gst: no need to use idle handlers to emit signals
    
    The GStreamer bus callback is already running on the main thread, so
    doing everything in idle handlers just makes it more complicated.

fixed in Rhythmbox 0.12.2  "We Are Underused". Thank You Jonathan & all!