After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 345975 - Show album covers embedded in files e.g. mp3 ID3 tags
Show album covers embedded in files e.g. mp3 ID3 tags
Status: RESOLVED FIXED
Product: rhythmbox
Classification: Other
Component: Plugins (other)
HEAD
Other Linux
: Normal enhancement
: ---
Assigned To: RhythmBox Maintainers
RhythmBox Maintainers
Depends on:
Blocks: 412853 538455
 
 
Reported: 2006-06-26 17:24 UTC by Peter
Modified: 2011-07-26 19:46 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Allow an art searcher to directly return image data (2.99 KB, patch)
2007-11-17 00:21 UTC, John Millikin
none Details | Review
Read album art embedded in id3v2 or COVERART tags (5.46 KB, patch)
2007-11-17 00:22 UTC, John Millikin
none Details | Review
Allow an art searcher to directly return image data (2.65 KB, patch)
2007-11-17 01:44 UTC, John Millikin
none Details | Review
Read album art embedded in id3v2 or COVERART tags (4.34 KB, patch)
2007-11-17 01:44 UTC, John Millikin
none Details | Review
Rhythmbox with attached patches applied (126.84 KB, image/png)
2008-01-05 01:04 UTC, Paul Drain
  Details
Rhythmbox showing garbled ID3 Coverart (180.76 KB, image/jpeg)
2008-01-05 02:09 UTC, Paul Drain
  Details
Read album art embedded in id3v2 or COVERART tags (v2) (5.10 KB, patch)
2008-01-05 02:27 UTC, John Millikin
none Details | Review
Updated Makefile.am to include EmbeddedArtSearch.py (5.47 KB, patch)
2008-01-05 05:27 UTC, John Daiker
none Details | Review
Sample track with cover art in 'image' tag (344.58 KB, audio/mpeg)
2008-01-05 20:50 UTC, Dan Mihai Ile
  Details
Read album art embedded in id3v2 or COVERART tags (v3) (5.32 KB, patch)
2008-01-11 04:02 UTC, John Millikin
none Details | Review
Extract album art and store URI in Rhythmdb (10.46 KB, patch)
2008-02-01 07:32 UTC, John Millikin
none Details | Review
Extract album art and store URI in Rhythmdb (10.80 KB, patch)
2008-02-09 05:05 UTC, John Millikin
none Details | Review
Updated, Semi-Working Patch for 0.11.6 (12.08 KB, patch)
2008-07-10 17:04 UTC, Paul Drain
none Details | Review
Patch used to test comment #62 (11.96 KB, patch)
2008-07-24 11:08 UTC, Paul Drain
none Details | Review
Updated Patch for GIO-enabled Rhythmbox (12.05 KB, patch)
2008-08-04 06:18 UTC, Paul Drain
none Details | Review
Updated Patch against 5852 (12.12 KB, patch)
2008-08-12 07:09 UTC, Paul Drain
none Details | Review
Updated Patch against 5874 (12.06 KB, patch)
2008-09-02 07:29 UTC, Paul Drain
none Details | Review
Updated Patch against 5957 (12.06 KB, patch)
2008-10-02 00:51 UTC, Paul Drain
none Details | Review
another approach (12.81 KB, patch)
2008-10-02 13:20 UTC, Jonathan Matthew
none Details | Review
cleaned up and updated (27.56 KB, patch)
2009-03-12 14:14 UTC, Jonathan Matthew
none Details | Review
add embedded cover art search for non-playing entries (43.40 KB, patch)
2009-04-15 03:49 UTC, Jonathan Matthew
committed Details | Review

Description Peter 2006-06-26 17:24:54 UTC
Quoting bug 307848 comment 9 
> There are a number of "standard" places to store cover art, but
> unfortunately no-one can agree on them.
> 
> a) Embedded in the files (e.g. in ID3 tags). This has the advantage that
> portable mp3 players can use the art - but it takes up more space due to
> storing the art once per track, rather than once per album. Another
> advantage is that you can copy the tracks, and the art goes with it. A
> point against is that some file formats don't have a way of embedding
> cover art.

This bug is to support reading any embedded cover images.

In particular, ID3v2 APIC tags for mp3 files can hold one or more images.

http://www.id3.org/id3v2.3.0.html#sec4.15

If more than one image is available, we should look at the "Picture type" field to make our selection:

$00	Other
$01	32x32 pixels 'file icon' (PNG only)
$02	Other file icon
$03	Cover (front)
$04	Cover (back)
$05	Leaflet page
$06	Media (e.g. lable side of CD)
$07	Lead artist/lead performer/soloist
$08	Artist/performer
$09	Conductor
$0A	Band/Orchestra
$0B	Composer
$0C	Lyricist/text writer
$0D	Recording Location
$0E	During recording
$0F	During performance
$10	Movie/video screen capture
$11	A bright coloured fish
$12	Illustration
$13	Band/artist logotype
$14	Publisher/Studio logotype

Of these $03 "Cover (front)" should be given the top priority followed by $00 "Other" which many programs including iTunes (on Windows at least) use when inserting an image.
Comment 1 James "Doc" Livingston 2006-06-27 05:37:12 UTC
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).
Comment 2 Nuno Sousa 2007-01-09 18:53:42 UTC
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.
Comment 3 Alex Lancaster 2007-01-12 03:06:21 UTC
(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... ;)

Comment 4 Ed Catmur 2007-02-23 10:18:04 UTC
@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.
Comment 5 Ed Catmur 2007-02-24 16:07:11 UTC
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.
Comment 6 John Millikin 2007-11-17 00:21:42 UTC
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.
Comment 7 John Millikin 2007-11-17 00:22:50 UTC
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.
Comment 8 Bastien Nocera 2007-11-17 00:56:52 UTC
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.
Comment 9 John Millikin 2007-11-17 01:44:04 UTC
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.
Comment 10 John Millikin 2007-11-17 01:44:48 UTC
Created attachment 99240 [details] [review]
Read album art embedded in id3v2 or COVERART tags
Comment 11 Bastien Nocera 2007-11-17 01:53:01 UTC
(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.
Comment 12 John Millikin 2007-11-17 02:54:24 UTC
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.
Comment 13 Bastien Nocera 2007-11-17 03:39:27 UTC
(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.
Comment 14 John Millikin 2007-11-17 05:09:17 UTC
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.
Comment 15 Alex Lancaster 2007-11-17 06:06:37 UTC
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.
Comment 16 Peter 2007-12-10 17:31:56 UTC
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.
Comment 17 John Millikin 2007-12-10 19:30:27 UTC
>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.
Comment 18 Dan Mihai Ile 2008-01-01 22:53:17 UTC
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.
Comment 19 Paul Drain 2008-01-05 01:04:58 UTC
Created attachment 102191 [details]
Rhythmbox with attached patches applied

Screenshot of Rhythmbox from SVN 5531 with these patches applied.
Comment 20 Paul Drain 2008-01-05 01:07:27 UTC
On Rhythmbox built on Ubuntu 7.10 with these patches, the following Traceback appears on the console:

Traceback (most recent call last):
  • File "/usr/lib/rhythmbox/plugins/artdisplay/__init__.py", line 23 in <module>
    from CoverArtDatabase import CoverArtDatabase
  • File "/usr/lib/rhythmbox/plugins/artdisplay/CoverArtDatabase.py", line 29 in <module>
    from EmbeddedArtSearch import EmbeddedArtSearch
  • File "/usr/lib/rhythmbox/plugins/artdisplay/EmbeddedArtSearch.py", line 24 in <module>
    pygst.require ("0.10")
  • File "/usr/lib/python2.5/site-packages/pygst.py", line 52 in require
    raise RequiredVersionError, "pygst.require() must be called before importing gst"
pygst.RequiredVersionError: pygst.require() must be called before importing gst

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


(rhythmbox:14400): Rhythmbox-WARNING **: Error, impossible to activate plugin 'Cover art'
Comment 21 Paul Drain 2008-01-05 02:06:57 UTC
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.
Comment 22 Paul Drain 2008-01-05 02:09:14 UTC
Created attachment 102195 [details]
Rhythmbox showing garbled ID3 Coverart
Comment 23 John Millikin 2008-01-05 02:27:38 UTC
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.
Comment 24 John Millikin 2008-01-05 02:32:40 UTC
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.
Comment 25 Paul Drain 2008-01-05 04:28:28 UTC
(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.
Comment 26 Paul Drain 2008-01-05 04:31:40 UTC
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.
Comment 27 John Daiker 2008-01-05 05:27:20 UTC
Created attachment 102198 [details] [review]
Updated Makefile.am to include EmbeddedArtSearch.py
Comment 28 Paul Drain 2008-01-05 07:02:50 UTC
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
Comment 29 Dan Mihai Ile 2008-01-05 13:06:36 UTC
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..
Comment 30 John Millikin 2008-01-05 19:15:53 UTC
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.
Comment 31 Dan Mihai Ile 2008-01-05 20:20:47 UTC
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
Comment 32 Dan Mihai Ile 2008-01-05 20:50:04 UTC
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.
Comment 33 Paul Drain 2008-01-06 02:27:22 UTC
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 :)
Comment 34 Dan Mihai Ile 2008-01-10 18:38:52 UTC
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):
  • File "/usr/lib/rhythmbox/plugins/artdisplay/EmbeddedArtSearch.py", line 67 in on_message
    extended = extended_comment_to_dict (tags['extended-comment'])
  • File "/usr/lib/rhythmbox/plugins/artdisplay/EmbeddedArtSearch.py", line 34 in extended_comment_to_dict
    return dict (split (comment) for comment in comments)
  • File "/usr/lib/rhythmbox/plugins/artdisplay/EmbeddedArtSearch.py", line 34 in <genexpr>
    return dict (split (comment) for comment in comments)
  • File "/usr/lib/rhythmbox/plugins/artdisplay/EmbeddedArtSearch.py", line 31 in split
    key, value = comment.split ('=', 1)
ValueError: need more than 1 value to unpack
(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?
Comment 35 John Millikin 2008-01-11 04:02:39 UTC
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.
Comment 36 Dan Mihai Ile 2008-01-11 11:04:15 UTC
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.
Comment 37 Paul Drain 2008-01-16 05:27:30 UTC
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.
Comment 38 John Millikin 2008-02-01 07:32:19 UTC
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.
Comment 39 John Millikin 2008-02-09 05:05:21 UTC
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://.
Comment 40 Paul Drain 2008-02-11 05:27:28 UTC
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)
Comment 41 Sven Anders 2008-02-19 00:34:10 UTC
Please add this to the next release....
Comment 42 Dan Mihai Ile 2008-02-19 10:27:14 UTC
I also think that this patch is quite stable, and it should be added into the next release.
Comment 43 Paul Drain 2008-03-03 01:10:41 UTC
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.
Comment 44 Jonathan Matthew 2008-03-16 11:38:50 UTC
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.
Comment 45 John Millikin 2008-03-17 19:44:19 UTC
> 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.
Comment 46 Jon Dowland 2008-03-19 13:23:11 UTC
(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.
Comment 47 Paul Drain 2008-04-29 04:45:37 UTC
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)
Comment 48 John Millikin 2008-04-29 06:06:09 UTC
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.
Comment 49 Paul Drain 2008-04-29 09:15:27 UTC
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.
Comment 50 John Daiker 2008-04-29 15:54:31 UTC
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
Comment 51 Paul Drain 2008-04-30 00:50:08 UTC
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.
Comment 52 John Millikin 2008-04-30 01:33:34 UTC
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.
Comment 53 Paul Drain 2008-04-30 02:23:35 UTC
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 :)
Comment 54 Paul Drain 2008-07-10 16:12:39 UTC
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?

Comment 55 Paul Drain 2008-07-10 16:57:55 UTC
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.
Comment 56 Paul Drain 2008-07-10 17:04:10 UTC
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.
Comment 57 Paul Drain 2008-07-19 05:26:28 UTC
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?
Comment 58 John Millikin 2008-07-19 06:18:04 UTC
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.
Comment 59 Paul Drain 2008-07-19 11:38:52 UTC
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.
Comment 60 Paul Drain 2008-07-21 04:11:11 UTC
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).
Comment 61 John Millikin 2008-07-21 04:28:30 UTC
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.
Comment 62 Paul Drain 2008-07-24 07:58:33 UTC
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?
Comment 63 Paul Drain 2008-07-24 11:08:13 UTC
Created attachment 115160 [details] [review]
Patch used to test comment #62
Comment 64 Paul Drain 2008-07-24 12:39:47 UTC
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"
---
Comment 65 Paul Drain 2008-08-04 06:18:06 UTC
Created attachment 115801 [details] [review]
Updated Patch for GIO-enabled Rhythmbox

Ammended patch that works with SVN (5832) and extracts cover art correctly
Comment 66 Paul Drain 2008-08-04 06:23:17 UTC
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.
Comment 67 Jonathan Matthew 2008-08-11 11:03:36 UTC
The call to gnome_vfs_get_uri_from_local_path in rb_metadata_load_gst_image_tag needs to be replaced.
Comment 68 Paul Drain 2008-08-12 05:11:54 UTC
Yep, it does -- don't know how I missed that.

Updated patch forthcoming :)
Comment 69 Paul Drain 2008-08-12 07:09:00 UTC
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.
Comment 70 Paul Drain 2008-09-02 07:29:14 UTC
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
Comment 71 Paul Drain 2008-10-02 00:51:06 UTC
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.
Comment 72 Jonathan Matthew 2008-10-02 13:20:15 UTC
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.
Comment 73 John Millikin 2008-10-13 00:01:42 UTC
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.
Comment 74 Mattias Eriksson 2009-02-20 18:40:04 UTC
What is the status of this? Is there any review planned for the patches?
Comment 75 Jonathan Matthew 2009-03-12 14:14:23 UTC
Created attachment 130520 [details] [review]
cleaned up and updated
Comment 76 Jonathan Matthew 2009-04-15 03:49:26 UTC
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.
Comment 77 Raul Acevedo 2009-04-23 08:00:02 UTC
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!
Comment 78 Jonathan Matthew 2009-04-29 08:30:52 UTC
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.

Comment 79 Luc Pi 2010-07-30 08:11:51 UTC
fixed in Rhythmbox 0.12.2  "We Are Underused". Thank You Jonathan & all!