GNOME Bugzilla – Bug 307848
Show and download album covers
Last modified: 2006-07-28 06:43:33 UTC
A quite big patch to add the support of the cover art in Rhythmbox. The added features are : * Show the cover of the currently playing album in the bottom left corner * Show small covers in the albums list * Search for a cover on the local disk (in the song directory) * Search for covers on amazon.com/fr/de/ca/uk/jp * Save cached covers in ~/.gnome2/rhythmbox/covers/ * Automatically redownload amazon covers every 3 months to respect the Amazon Web Services Licensing Agreement Screenshot : http://perso.enst.fr/~pavot/Capture.png If you want more details about it, you can look at my posts on the rhythmbox-devel list. Marc
Created attachment 47836 [details] [review] The patch In the .tar.gz archive, there is a diff file with all the modifications made to already existing files and there is also all the new files added for the cover support.
Created attachment 47838 [details] The patch (bis) I repost the patch because I made a mistake with the filetype. In the .tar.gz archive, there is a diff file with all the modifications made to already existing files and there is also all the new files added for the cover support.
Created attachment 47927 [details] A new patch This new patch replace the previous one. It should fix some syncing problems with newer versions of Rhythmbox CVS and fix some bugs.
Created attachment 49617 [details] Patch that applies cleanly to head The provided patch did not apply cleanly to head anymore. This patch has the same format as the previous patch: i.e. a tar.gz file containing the patch and a directory containing all the new files. This patch corrects the following 1. the diff is now against head of 23 July 2. Removed gcc 4.0 signedness warnings in rb-cover.c 3. Performed a chmod -x on the new files
Forgot to mention that the patch of Marc works fine for me, but can I suggest to enable the automatic searching of covers by default. I was confused that the covers didn't come up automatically
Can one of the maintainers of rhythmbox have a look at the patch here for album covers. I bet many users are eager to have this feature. Thanks
There was some discussion on IRC the other day about this. My view on what was decided is that a) we want to get 0.9.0 out Real Soon - so this, the play queue and a couple of other things can wait until 0.9.1 and b) we should get together with some of the Gnome usability people and discuss a couple of UI issues with thi s and the play queue. Hopefully this will get in shortly after 0.9.0
I'm not a rbox developer, (yet), but I'd like to throw my 2 cents. I'd like KDE & Gnome to agree on a common place to store the album covers instead of .gnome2/somefile.foo or .kde/longpath/file.foo . Example: User has been listening in amarok which has all the album covers from amazon downloaded. User then listens to collection using rhythmbox. User then has to re-download all of the album covers which seems an unnecessary duplication to me. Perhaps there could even be a library since there seems to be code duplication going on.
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. b) In the same folder as the tracks. This works great if you have your music sorted hierarchically, but doesn't work at all if you don't. This has the advantage of keeping the art in the same place as the music. c) In some other location (e.g. ~/.gnome2/rhythmbox/covers). This would work well if every application could agree on a place to put it, but it will require some agreement on where. One disadvantage is that if you have a music directory shared between multiple users, the cover art will be per-user. The current version of the patch will look at (b) first, and then go to Amazon (et al) if none was found there.
In response to Comment #9 from James. I think a should definately be supported as it seems to be a fairy well used option (I know iTunes uses it, and while that alone isn't necessarily a reason itself it does make it fairly well used elsewhere). Another advantage of using it is support of easily displaying album art across daap shares as its already embedded in the metadata (not sure if this is supported or whether it works in the iTunes implementation of daap).
Being able to read covers out of a file would be good to support, although I'm not sure if gstreamer's tag support extends this far - it might, but I haven't checked. The big question is if art isn't found locally and we have to retrieve it (from amazon et al) where do we store it? Currently the patch puts it in (c), which means that other apps won't see any art that RB downloads.
One bug I noted with this patch: If you enable the "Show covers in the album list" option and then disable it again, the rows are not scaled down to normal size. Also, select local cover seems to use an old file chooser, why? Apart from that, works great and still applies cleanly (apart from some changed offsets).
*** Bug 133459 has been marked as a duplicate of this bug. ***
Created attachment 53162 [details] [review] updated patch patch updated to apply to cvs
Created attachment 53163 [details] updated tarball updated tarball. rb-shell-coverdownloader and rb-shell-coverselector have been moved from shell/ to widgets/
0.9.3 is out, this feature isn't in? :'(
Nope. It's been sitting around waiting for someone to update to current cvs, and fix a few minor issues. Ideally it would be reworked, to deal with art from podcasts, ipods et cetera, but it could probably be added with just the basic stuff done.
Created attachment 60288 [details] Small C file to accompany cover art plugin Simple. Handles international characters, uses libxml2, gnome-vfs and wget (until gnome-vfs is fixed).
Comment on attachment 60288 [details] Small C file to accompany cover art plugin Cover art grabber, reads rhythmdb.xml using libxml2, figures out ASIN and nabs the cover thumbnail from Amazon. Uses wget 'til gnome-vfs is fixed. Stores ,jpgs in ~/,gnome2/rhythmbox/covers
Created attachment 62353 [details] [review] basic python art plugin This is a basic art display plugin in Python I whipped up. Definitely needs for work, but it's a start.
Hey James, might be something on my side but I get the following when I try to enabled the plugin in the plugin dialog: Traceback (most recent call last):
+ Trace 67347
import rhythmdb, rb
(rhythmbox:1744): Rhythmbox-WARNING **: Could not load python module artdisplay (rhythmbox:1744): Rhythmbox-WARNING **: Error, impossible to activate plugin 'Art Display'
My guess would be that you need to run "cvs up -dP", to get the rhythmdb python bindings, which I only added to cvs yesterday.
Created attachment 63002 [details] [review] updated art plugin Now resizes the art to the right size, and mostly handle the sidebar being resized.
Created attachment 63423 [details] [review] updated patch Updated to current cvs.
Created attachment 63942 [details] updated python plugin which does amazon lookup Here's an updated plugin which uses parts from pyamazon in order to do amazon album art lookup and saves them to HOME/.gnome2/rhythmbox/covers/ It's very basic at the moment and is missing a lot of features still.
Created attachment 63974 [details] updated plugin 2 Here's a tiny update to the plugin. It now detects a bad image file (sometimes amazon returns an empty image) and also has some REALLY rudimentary stripping of certain strings like '(Disc 1)' and '(Volume 1)' in order to get more accurate results from amazon.
This is working pretty good for me and since we are at the beginning of a development cycle, I tossed it in to CVS for some more testing. Gareth - as you have more updates, patches against CVS would be ideal :)
Created attachment 64380 [details] [review] Various fixes and more matching heuristics Various updates. *Use set_from_pixbuf(None) in place of clear() for compatibility with pygtk 2.6 (on FC-4) *Use regular expression matching for stripping Disc/disc Volume/Volume [1-9]+ type names before doing search. *Improve matches by looking for exact titles in <ProductName> in the list matched albums, if not found, default back to first match (current behaviour). This finds better matches when the actual album is not the first (often happens with self-titled albums). *Add heuristics for doing searches on compilations by using "Various" in place of artist name. *Include a locale "us" for xml.amazon.com, can't be left blank.
I'm not sure about anyone else, but all the albums I have with "Volume N" in the name have completely different covers.
This is (Volume N) in brackets, but yes, (Disc/disc N) is probably safer than stripping out (Volume N). (Volume/volume) was in the original plugin, I was just updating it to use regexp. Perhaps it should not remove (Volume N) (even in brackets).
Created attachment 64409 [details] [review] updated patch * Uses a sequence of searches (fallbacks). * Improves searches with artist or album == Unknown * Use Medium size image if Large is unavailable * Fix spacing
I went ahead and committed this since it has been working very nicely for me.
Created attachment 64617 [details] [review] try to auto-detect the locale
Created attachment 64733 [details] [review] artdisplay patch adding async search/loading, modularity NOTE: The patch already contains the auto-detect locale patch from above. Works pretty good for me and does not break compatibility with previous artdisplay script. Changes: * Add asynchronous loading with gnomevfs and urllib fallback (thx to Jonathan Matthew) * Fix Rhythmbox UI blocking while loading/searching cover art * Split logic into CoverArtDatabase, AmazonCoverArtSearch * Initial approach to allow additional "art search engines" * Further modularization in a couple of places * Minor fixes for empty entries (iradio, album, artist etc.) (* Callbacks for world-domination) Some ideas for improvement: - Improve overall search algo to avoid getting wrong covers (simplify) - Clean up code handling regarding "entry" vs "st_artist, st_album" - Introduce CoverArtSearch base class - Add further search engines - Add configuration dialog (empty cover cache, search severity) - Add playlist menu item: Reset cover art (removes cached file)
Created attachment 64738 [details] [review] patch v2 adding async search/loading, modularity Changes: * Fix small bug callback of data loader getting overwritten on fast connections * Add asynchronous loading with gnomevfs and urllib fallback (thx to Jonathan Matthew) * Fix Rhythmbox UI blocking while loading/searching cover art * Split logic into CoverArtDatabase, AmazonCoverArtSearch * Initial approach to allow additional "art search engines" * Further modularization in a couple of places * Minor fixes for empty entries (iradio, album, artist etc.) (* Callbacks for world-domination)
I've committed the updated patch to cvs.
There's a typo on line 173: blist_location = self.build_art_cache_filename(self.search_engine.st_album, self.search_engine.st_artist, "rb-list") should be: blist_location = self.build_art_cache_filename(self.search_engine.st_album, self.search_engine.st_artist, "rb-blist") Also line 28 is problematic: # replace quote characters for char in ["\"", "'"]: st_artist = st_artist.replace (char, '') st_album = st_album.replace (char, '') Removing the single quote means that album names like "Let's Swing" get truncated to "Lets Swing" and can break the search. Either "'" should be removed or made to only remove if there is a leading or trailing space before the "'" (even that could be problematic).
Created attachment 64744 [details] [review] fade between covers This patch makes the art widget fade between covers. Switching to/from having no cover still had the widget appear/disappear which doesn't look so good.
Created attachment 64763 [details] [review] fixes typo, improve search, include fading patch NOTE: I included the patch from above ;) The fading has a minor "jumping" height issue when finished fading. The current search improvements mainly improve search if the tagging is not perfect (if it is, the search works very good). However, I came to the conclusion that an algo testcase-suit has to be written in order to have a full fledged search algo (comparing amaroK the one here is already more advanced). Alongside that, the current "st_artist, st_album" handling should be re-written to work entirely on "entry" instead (which could use other properties to improve search aswell, such as the song title). Changes: * Fixed typo "rb-list" -> "rb-blist" file extension * Improved search heuristics to increase success locating cover art * Include fading between cover images
(In reply to comment #39) > Created an attachment (id=64763) [edit] > fixes typo, improve search, include fading patch Work nicely for me. Fade works with the minor jumping issue, which isn't a big deal, IMHO. This patch doesn't address the removal of single quotes (') which will cause some album searches to fail because it removes them if used as a possesive (e.g. Let's != Lets). > However, I came to the conclusion that an algo testcase-suit has to be written > in order to have a full fledged search algo (comparing amaroK the one here is > already more advanced). Alongside that, the current "st_artist, st_album" > handling should be re-written to work entirely on "entry" instead (which could > use other properties to improve search aswell, such as the song title). Sounds good.
One issue I have noticed in recent patches is that in some cases both .rb-blist *and* .jpg files are created.
Created attachment 64790 [details] [review] fixes typo, improve search, include fading patch, small fixes (In reply to comment #40) > This patch doesn't address the removal of single quotes (') which will cause > some album searches to fail because it removes them if used as a possesive > (e.g. Let's != Lets). Yes this remains alongside with things like (Solomé vs. Solome). I'll have a look into it. Did not want to change it as I do not know what it breaks vs. what it solves. ;) Perfect example for the testcase to solve such things. > > One issue I have noticed in recent patches is that in some cases both .rb-blist > > *and* .jpg files are created. Right, it's useless. Fixed. --- Changes: * Fix some covers being re-downloaded although album art cache file exists * Fix creating both image file and "rb-blist" in some search cases * Fixed typo "rb-list" -> "rb-blist" file extension * Improved search heuristics to increase success locating cover art * Include fading between cover images
With this patch, it appears that neither a .jpg nor .rb-blist file are being created for some album searches.
I notice that in albums with multiple artists (i.e. The Office Space Soundtrack) it downloads CD cover multiple times to ~/.gnome2/rhythmbox/covers. This can become an issue when a person listens to one song while connected to the Internet. Then latter on that person listens to anther song on the same album but different artist without being connected the Internet. This would cause the album not able to be displayed even though it's already downloaded.
(In reply to comment #44) > I notice that in albums with multiple artists (i.e. The Office Space > Soundtrack) it downloads CD cover multiple times to ~/.gnome2/rhythmbox/covers. > This can become an issue when a person listens to one song while connected to > the Internet. Then latter on that person listens to anther song on the same > album but different artist without being connected the Internet. This would > cause the album not able to be displayed even though it's already downloaded. Yep, it's a known problem. CD cover art is downloaded by matching on "Artist - Album", but for compilation albums there can obviously be more than one artist. Currently it's difficult to see a better way of doing this. It ultimately requires that rhythmbox have better support for detecting compilations and treating them as one album, currently everything is done in a per-track way. Then the album cover art plugin could use that information to only download and store one image per album. Compilation support is a tricky problem in general, see bug #318579 and this ML thread: http://mail.gnome.org/archives/rhythmbox-devel/2006-April/thread.html#00074
I don't know how your plugin retrieves and stores the album covers, buy my widget (I really have to rewrite it) you can find in bug #309228 first try searching using Amazon ASIN code (musicbrainz has it) in local cache and then, if it fails, asking Amazon. If ASIN code isn't available, another search is used (Artist/Title), but only to retrieve the ASIN code, and then fall back to ASIN regular search to download and store che jpeg. Since ASIN code is unique for every album, this should solve your problem (if I understood it well :). As a side note, if the local stored cover is older than 3 months it must be deleted and downloaded again, as requested by Amazon. btw, it would be nice to have the image covers stored in a common place, shared among all the apps that need them.
(In reply to comment #46) > I don't know how your plugin retrieves and stores the album covers, buy my > widget (I really have to rewrite it) you can find in bug #309228 first try > searching using Amazon ASIN code (musicbrainz has it) in local cache and then, > if it fails, asking Amazon. If ASIN code isn't available, another search is > used (Artist/Title), but only to retrieve the ASIN code, and then fall back to > ASIN regular search to download and store che jpeg. Since ASIN code is unique > for every album, this should solve your problem (if I understood it well :). I think rhythmbox would have to support musicbrainz in the rhythmdb.xml database and be able to associate a particular track with an album and that album with an ASIN before that could be done, see bug #104203
Adding support for MusicBrainz id tags (track, artist, album) probably wouldn't be too hard, and would be good to do anyway. The latest release of gst-plugins-bad includes the 0.10 port of the TRM-generation plugin, so we can now generic MB ids too. Probably what would be better is to make the cache map MB id to art, rather than ASIN to art. This would allow us to find the art without doing a MB id to ASIN conversion (which would require a 'net connection), and also allows for other non-Amazon art grabbing. The big issue for Rhythmbox would be dealing with tracks we don't have MB ids for. I'm not sure what would be best here.
(In reply to comment #43) > With this patch, it appears that neither a .jpg nor .rb-blist file are being > created for some album searches. > More a feature than an issue. Sometimes there are basicly no successful search results for the various searches. "rb-blist" is only generated if the search was successful, but downloading the cover failed (due to corrupt/bad image) and no better/working alternative cover was found. "rb-blist" is ment to mark bad/corrupt images or rather searches with no results? (I assumed the first from the initial version) --- Regarding the "multiple-covers-due-to-compilation" issue, I think the best would be to add musicbrainz support so you additionally have an ID to work with. I assume that would also increase the success rate for cover search. Grouping compilations support should use "manual-grouping" (but added in some distant future) as it is very hard to implement something that does it automatically. In the best case the artdisplay plugin should be able to: - Download covers from internet sources (amazon, bestbuy.com, google/images...) - Re-download covers every 3 months (if amazon is used at least) - Use cover images within directory of the playing track - Use cover art embbeded within ID3v2 tags - Intelligent caching of downloaded covers (album vs. per-track) - Cache album covers in a common place I kind of start to believe this this functionality might be good for a new external library project. ;) --- BTW: At least the typo fix ("rb-list" -> "rb-blist" file extension) should be commited just in case, so it won't accidently go into release. :)
(In reply to comment #49) > > With this patch, it appears that neither a .jpg nor .rb-blist file are being > > created for some album searches. > > More a feature than an issue. Sometimes there are basicly no successful search > results for the various searches. "rb-blist" is only generated if the search > was successful, but downloading the cover failed (due to corrupt/bad image) and > no better/working alternative cover was found. > > "rb-blist" is ment to mark bad/corrupt images or rather searches with no > results? (I assumed the first from the initial version) That's not how I read it. I think the original version written by Gavin always generated one or the other if the network was connected (Gavin?) This was to prevent lookups being done repeatedly on albums that have no matches, otherwise how does it know not to re-lookup the album?
Created attachment 64905 [details] [review] other bits I've committed the typo fix and cover fading. This patch is the other bits from the last patch. I wasn't sure what bit(s) were good and what needed fixing, so I've left it for now.
Created attachment 64922 [details] [review] fix re-use issues This fixes various issues with the image loader and search engines being re-used. It makes the gnomevfs-loader reusable, and creates a new instance of the amazon-searcher instead of reusing the old one. Without these fixes, some nasty things can happen if you change tracks while lookup is in progress, and a new lookup starts before the old one finishes.
Created attachment 64986 [details] [review] above + split This includes my re-use patch, and also splits the classes into their own .py files. If this looks good to people, I'd like to commit it and then work on making the Loader class sharable between plugins.
(In reply to comment #53) > Created an attachment (id=64986) [edit] > above + split > > This includes my re-use patch, and also splits the classes into their own .py > files. > > If this looks good to people, I'd like to commit it and then work on making the > Loader class sharable between plugins. Have a problem with on_get_pixbuf_completed() call: Traceback (most recent call last):
+ Trace 68111
self.set_entry(sp.get_playing_entry ())
self.art_db.get_pixbuf(db, entry, self.on_get_pixbuf_completed)
callback (entry, pixbuf)
Looks like the callbacks are missing the db parameter in CoverArtDatabase.get_pixbuf(), adding it makes it work for existing art. There is also another problem when trying to parse results from Amazon when no art is already downloaded: Traceback (most recent call last):
+ Trace 68112
best_match = search_engine.get_best_match (best_match)
best_match = search_results[0]
Created attachment 65077 [details] [review] Hopefully fixes split patch and incorporates some bits from "other bits" patch
(In reply to comment #56) > and incorporates some bits from "other bits" patch in particular it adds back some of the heuristics from: http://bugzilla.gnome.org/attachment.cgi?id=64905&action=diff#rhythmbox/plugins/artdisplay/artdisplay.py_sec7
Created attachment 65079 [details] [review] Was missing configure.ac
Created attachment 65080 [details] [review] One more fix, remove a local change not intended for CVS
Created attachment 65084 [details] [review] One more to get it right
Created attachment 65085 [details] [review] Really get it right this time
Committed to cvs.
CVS doesn't seem to clear the previous art when it doesn't find a match for the current song. If we don't want to have the area hide then maybe we could display "Artwork not found" or something.
Created attachment 65237 [details] [review] Various fixes Check to see if the search actually found something in AmazonCoverArtSearch.py, make sure that we don't try to iterate through a non-list. Also check that best_match actually returned something before querying it's ImageURL (which could be non-existent). This should clear the art when there is no match and address comment #63. Also don't strip single quotes (') before search because that's important punctuation, e.g "Let's Get Lost" will be stripped to "Lets Get Lost" which won't match.
Created attachment 65239 [details] [review] Update to previous patch
Created attachment 65263 [details] [review] updated patch This should: * correct fallback to medium size * convert & to "and" when doing comparisons * only call the first artist the best match when album==Unknown * don't perform each query twice if album doesn't include "Vol N" * Fix searching for compilations
Looks good to me.
(In reply to comment #66) > Created an attachment (id=65263) [edit] > updated patch > > This should: > > * correct fallback to medium size > * convert & to "and" when doing comparisons > * only call the first artist the best match when album==Unknown > * don't perform each query twice if album doesn't include "Vol N" > * Fix searching for compilations This patch seems to reintroduce an issue I found previously: it creates both a .jpg and a .rb-blist file for some searches, e.g. for: artist: Black Dog Productions album: Bytes Both .jpg and .rb-blist are created. The semantics of when .rb-blist is created should also be clarified. My understanding based on Gareth's original patch and discussion with him on IRC is that if no matches are found, the .rb-blist is created, unless the network is not present and therefore the search could not be performed.
Right. There were a number of things that are not so good. The code had callbacks starting from all over the place and blacklisting only done from the image data callback. So blacklists would only be created if there were search results with images. And as you pointed out it was blacklisting too aggressively. Also it wasn't possible to check the result of search_next to see if there were any more searches in the list. So, I've added a return value to search_next and created a private method to do the blacklist and callback. I've commited this. Please give it a try.
I love this plugin, but I would like to make a point. I sometimes to listen to albums that aren't widely distributed and are not on amazon. So I get the wrong album art when I play the music. I would like to be able to change the art for an album easily. Perhaps DnD from another source would set it. Also this should be very self evident. Thanks
Created attachment 65975 [details] [review] allow dropping of art on widget This patch allows you to drop files (or uris) onto the art display widget to set the art for the current album. In theory you should also be able to drag the art to Nautilus et al to copy, but I've done something silly so it doesn't work. Current issues: * you can't set the art if there isn't some already, because the widget isn't shown. Should we have a rb logo/"drag art here" placeholder? * we load the image and then save it as a jpeg, which allows us to import any (gdk supported) image format. This recompresses things that are already im jpeg format, which isn't ideal.
How about the ability to right click on an image and click on something like "wrong one". I have some CDs that are not sold on amazon, and don't get the right covers. Wonderful work so far. I love this. Thanks!
Created attachment 67611 [details] [review] cover reload patch This dirty patch adds to new menu items in menu Edit: one allow user to overwrite cover arts (needs more error checking), another one to re-fetch cover arts from Amazon. It may use some code from patch #65975
See comment 9 and the following "sub bugs": Bug 345688 – Show and album covers from local files Bug 345975 – Show album covers embedded in files e.g. mp3 ID3 tags Finally, this current bug is very long and covers a lot of ground: Could it be closed now that the basic functionality has been checked in, and separate bugs be files for things like editing (removing, replace) and drag-and-dropping the image?
Placeholder image: bug 346736
(In reply to comment #74) > Finally, this current bug is very long and covers a lot of ground: > > Could it be closed now that the basic functionality has been checked in, and > separate bugs be files for things like editing (removing, replace) and > drag-and-dropping the image? I agree, now that it's more or less working, we should have new separate bugs for the various enhancements/fixes.
I agree with the previous comments, this can be closed and have separate bugs for additional functionality. The two partially worked on patches will need substantial reworking with to apply to current cvs, so marking obsolete.