GNOME Bugzilla – Bug 388162
Show embedded cover art
Last modified: 2008-10-01 06:28:07 UTC
I have cover art embedded in almost all of my mp3s. It would be nice if Banshee could pull the image out of the file and use it.
Created attachment 78727 [details] [review] Quick hack to get this working. This is just a quick hack to get this working. I plan to clean it up before submitting it for approval. I'm putting it here in case somebody gets to it before I do.
Created attachment 78973 [details] [review] Patch to retrieve embedded cover art from TagLib for display in Banshee. This is a revised version of the hack that I had posted earlier. I feel this patch is ready to be committed.
Last night I added a basic metadata services API with a Rhapsody web implementation. I'd like to see this patch wrapped in the metadata services instead. It would act as a high priority provider for local tracks, for example. See src/Banshee.Base/Banshee.Metadata and src/Banshee.Base/Banshee.Metadata.Rhapsody. If you follow the same shell implementation as the Rhapsody client, it should be pretty simple to do. No modification to TrackInfo should be necessary. Looking good!
Created attachment 79251 [details] [review] Updated patch to retrieve embedded cover art. This uses the new MetadataProvider framework. Aaron, I modified both TrackInfo and StreamTagger to try all possible image extensions when using the new artist_album_id. Previously it would only work with ".jpg".
i also have cover art embedded in almost all of my mp3s. This is a better idea than use cover.jpg in the album folder I think. So is this feature going to be avaiable in Banshee > 0.11.3? I't a quite used feature... I think the correct order to get artwork of a song would be: 1.embedded cover if fails then: 2.look for cover.jpg in album folder if fails then: 3.banshee database and online covers.
Okay, this is now committed, with a few changes of my own: 1. Updated to work with the changes in the metadata services API 2. Instead of writing the raw image data to a file, instead use MemoryStream on the byte data and create a Gdk.Pixbuf from it. This will ensure that the image will in fact be able to be displayed later on, and also normalizes the image data. This pixbuf is then written out as a JPEG. 3. Some small performance changes in changes to TrackInfo 4. Got rid of ASIN support altogether as it is deprecated and the old metadata search plugin is about to get an overhaul to use the services APIs That said, I don't have any music right now with embedded cover art and I'm out of time today to embed some with TagLib, so this is completely untested from me, but looks good. However, please confirm that my Pixbuf normalization change does in fact work :) Thanks Trey!
Does this still work in 1.0? Users have reported it not to be working on IRC and I also read a "report" about this on digg. Reopening for now.
*** Bug 328376 has been marked as a duplicate of this bug. ***
*** Bug 538228 has been marked as a duplicate of this bug. ***
*** Bug 540914 has been marked as a duplicate of this bug. ***
Regarding Version 1.2.1 (from Ubuntu PPA) and probably others. As mentioned in this thread: http://mail.gnome.org/archives/banshee-list/2008-September/msg00069.html I noticed a couple issues: 1.) Even though you disabled the "Cover Art Fetching" plugin, album art is still retrieved online. I've verified this with packet captures of dns queries and http get requests. a.) If the mp3 has no embedded images in it's tag, Banshee first checks rhapsody.com, then musicbrainz.org, and finally amazon.com. b.) If the mp3 has embedded images, Banshee again checks rhapsody.com, then musicbrainz.org, and finally extracts the artwork. In some cases, the images are also downloaded from images.listen.com. 2.) Embedded album is extracted from the tag once you play the album or track. This image is stored as ~/.cache/album-art/artist-album.cover, but this is not used. I verified this by disabling internet access and playing the album. It extracted the ".cover" file, but did not display the image in Banshee. I copied the image and named it using the album-artist.jpg naming convention, and it immediately displayed correctly. Apparently Banshee ONLY uses files with .jpg as cover art. .cover files seem to be ignored. 3.) Earlier in my thread, I mentioned that Various Artist albums that I had to manually group by modifying the album artist metadata would not display any album art. It turns out that the embedded artwork is extracted as "variousartists-album.cover", but since there's never a match on any of the above mentioned music services, it never downloads a .jpg version of the file. Again... manually copying and renaming the file as "variousartists-album.jpg" immediately resolves the issue.
Out of curiosity, I noticed that the status of this bug report is still NEEDINFO. My last message gave quite a bit of information, so is there more that is needed? Is this something we can expect in 1.4.0? I just wanted to see if .cover files will eventually be used instead of downloaded album art. BTW... my workaround for displaying .cover files is the following: 1. I double click on every album once, which extracts the album art from the tag in a "artist-album.cover" format. This also downloads the .jpg equivalents from the net, which is what Banshee uses for displaying in the browser. 2. After all covers have been extracted, I run the following script which deletes all .jpg files in the ~/.cache/album-art directory, and copies the .cover files over to .jpg files. Banshee immediately displays the new version of the files. #!/bin/bash # fix-banshee-album-art.sh ART="$HOME/.cache/album-art" find $ART -type f -iname "*.jpg" -exec rm -f '{}' \; for i in $ART/*.cover do cp $i $ART/`basename "$i" .cover`.jpg done
(In reply to comment #12) > Out of curiosity, I noticed that the status of this bug report is still > NEEDINFO. My last message gave quite a bit of information, so is there more > that is needed? You left the bug in that state; feel free to reopen when providing information.
I've been looking at this bug, and found a few things. First of all, the cover is extracted from the file (unless you run into Bug 533689, which I did), and is stored into a .cover file. The ArtworkManager is ready to handle this .cover file, changing it into a .jpg, but this code is never executed since it relies in CoverArtSpec returning a .cover ended string. CoverArtSpec only works with jpg files, so the extracted file is never used. The GetPath and GetPathForSize methods of CoverArtSpec are used for two different things. First, they are used when you're trying to find an existing cover, and so should return a valid cover, whether it's a .jpg, a .png, or a .cover. But it's also used when you're trying to find the proper place to store a downloaded file, so it must return a jpg name of file, regardless of its existence. I've split the functionality in two different methods, GetPreferredPath and GetValidPath (with their _ForSize helpers), and adapted the few places where it's used. I've been able to properly test an old mp3's embedded cover art that I had and didn't even know of, but when adding images through easytag, I always run into Bug 533689, so I've been unable to test embedding png's or gif's. Please test.
Created attachment 119526 [details] [review] Split CoverArtSpec.GetPath into two different methods
(In reply to comment #14) > I've been able to properly test an old mp3's embedded cover art that I had and > didn't even know of, but when adding images through easytag, I always run into > Bug 533689, so I've been unable to test embedding png's or gif's. Please test. > Bug number 533689 I believe is related to ID3 v2.4 tags and UTF-8. I've tested this thoroughly, and found that if you change your EasyTag settings to use v2.3 tagging, files are imported with zero issues. Also, regarding the rest of your post, not sure if you want us to test Banshee v1.3.1, or to apply your patch and try that. I'm not a programmer, and not quite sure if I follow what your patch does. Apologies. In v1.3.1, I just tested the following: All embedded image types of JPG, PNG and GIF are successfully extracted as .cover files when you execute a play function on the album or track. Banshee only looks for files with a .jpg extension, so I renamed the extracted images, and regardless of their actual image type, to use a .jpg extension. Banshee properly displayed them all, but only if the .jpg extension is used.
(In reply to comment #16) > Also, regarding the rest of your post, not sure if you want us to test Banshee > v1.3.1, or to apply your patch and try that. I'm not a programmer, and not > quite sure if I follow what your patch does. Apologies. > Ahh... after reading your message and patch more carefully, I see what you're doing. I'll have to work with SVN, patch, compile and test. Sorry for the confusion.
(In reply to comment #15) > Created an attachment (id=119526) [edit] > Split CoverArtSpec.GetPath into two different methods > I can successfully compile and run from SVN, but I'm having problems compiling with your patch applied. See below. Making all in Banshee.Dap.Ipod make[4]: Entering directory `/usr/src/banshee/src/Dap/Banshee.Dap.Ipod' Compiling Banshee.Dap.Ipod.dll... ./Banshee.Dap.Ipod/DatabaseRebuilder.cs(205,40): error CS0117: `Banshee.Base.CoverArtSpec' does not contain a definition for `GetPath' /usr/src/banshee/bin/Banshee.Core.dll (Location of the symbol related to previous error) Compilation failed: 1 error(s), 0 warnings make[4]: *** [../../../bin/Banshee.Dap.Ipod.dll] Error 1 make[4]: Leaving directory `/usr/src/banshee/src/Dap/Banshee.Dap.Ipod' make[3]: *** [all-recursive] Error 1 make[3]: Leaving directory `/usr/src/banshee/src/Dap' make[2]: *** [all-recursive] Error 1 make[2]: Leaving directory `/usr/src/banshee/src' make[1]: *** [all-recursive] Error 1 make[1]: Leaving directory `/usr/src/banshee' make: *** [all] Error 2 Steps used: apt-get build-dep banshee-1 cd /usr/src sudo svn co http://svn.gnome.org/svn/banshee/trunk/banshee cd banshee (downloaded patch to ./patch.txt) patch -p0 <patch.txt (successfully patched) ./autogen.sh ./configure --prefix=/usr make (see make error above)
Félix, I guess you missed some calls because you have the ipod stuff disabled. A quick grep found these : src/Dap/Banshee.Dap.Ipod/Banshee.Dap.Ipod/IpodTrackInfo.cs: SetIpodCoverArt (device, track, CoverArtSpec.GetPath (ArtworkId)); src/Dap/Banshee.Dap.Ipod/Banshee.Dap.Ipod/DatabaseRebuilder.cs: string path = CoverArtSpec.GetPath (aaid); I'm also not so sure about the new methods' names. Wouldn't "GetExistingPath" be more correct than "GetValidPath" ? And maybe "GetPath" could be left as is ?
Bertrand, I'm afraid that's the problem. I'm using a Ubuntu machine for testing, and I have to disable the Ipod support. I'm changing the patch right now. Regarding the names, I'm changing GetValidPath to GetExistingPath, I like it more too. I changed the original GetPath to be able to detect any trouble at compile time (completely forgot about the ipod stuff). Besides, I find the original GetPath misleading, since its use will always be getting a path where we'll be storing the file, not any real path itself. I prefer the new GetPreferredPath, but it's your call. Not changing it would make for a rather smaller path, to be honest.
Created attachment 119541 [details] [review] Updated to include ipod support, and method name changing
With Gilbert's workaround for Bug 533689 , I just tested embedding a png, and it works. Thanks for pointing it out, Gilbert!
(In reply to comment #22) > With Gilbert's workaround for Bug 533689 , I just tested embedding a png, and > it works. Thanks for pointing it out, Gilbert! > Cool! No problem.
(In reply to comment #21) > Created an attachment (id=119541) [edit] > Updated to include ipod support, and method name changing > Sorry, patch still has an issue. :-( Making all in Banshee.Dap.Ipod make[4]: Entering directory `/usr/src/banshee/src/Dap/Banshee.Dap.Ipod' Compiling Banshee.Dap.Ipod.dll... ./Banshee.Dap.Ipod/DatabaseRebuilder.cs(205,40): error CS0117: `Banshee.Base.CoverArtSpec' does not contain a definition for `GetExistingPath' /usr/src/banshee/bin/Banshee.Core.dll (Location of the symbol related to previous error) Compilation failed: 1 error(s), 0 warnings make[4]: *** [../../../bin/Banshee.Dap.Ipod.dll] Error 1 make[4]: Leaving directory `/usr/src/banshee/src/Dap/Banshee.Dap.Ipod' make[3]: *** [all-recursive] Error 1 make[3]: Leaving directory `/usr/src/banshee/src/Dap' make[2]: *** [all-recursive] Error 1 make[2]: Leaving directory `/usr/src/banshee/src' make[1]: *** [all-recursive] Error 1 make[1]: Leaving directory `/usr/src/banshee' make: *** [all] Error 2
Created attachment 119546 [details] [review] Include missing method GetExistingPath /me bangs his head against a wall...
(In reply to comment #25) > Created an attachment (id=119546) [edit] > Include missing method GetExistingPath > > /me bangs his head against a wall... > Hehe.. great job, mate! Looks like you've fixed the compilation issue, as well as the mission at hand. Here's my test results. Embedded images files are extracted successfully on scanning the library. Files with embedded images that are appropriately named no longer get album art downloaded from the net. w00t! I've tested this on a 5GB library, all of which were completely extracted. JPEG files are named with the .jpeg extension, which is somewhat atypical, but meh, no big deal. It works great, and that's what counts. The images are appropriately copied and scaled int their respective subfolders. e.g. 32, 42, 48. PNG files are extracted with the .png extension and work perfectly. The images are appropriately copied and scaled int their respective subfolders. e.g. 32, 42, 48. GIF files are extracted, but the file extension is missing. e.g. "artist-album." I haven't run into any mp3's that use GIF's, and EasyTag doesn't mention that it's supported, but I was able to attach the images anyway. That being said, when I look at the image type in EasyTag, it says "Unknown image", which may be the root of the problem. Again, very good job, Felix.
Would it be possible for you to find *any* legally distributable song (e.g. one with a suitable Creative Commons license attached to it) and embed a cover file in it? I'm looking to review this patch but I have no embedded art.
I added one for rhythmbox cover art bug. You could use that one. http://bugzilla.gnome.org/show_bug.cgi?id=345975#c32
The patch looks good and seems to work for me. I just have a question : in EmbeddedQueryJob.cs, you set the file extension by using the MIME type. Is that intentional ? I'm asking because I understood from your description that everything should work with the .cover extension.
Initially, I thought of using the .cover extension, but then I realized that would only work for embedded jpg's. This way, png's are also supported, for instance.
Looking into this. I don't think I like the idea of storing the non-jpeg files with their random extensions. The point of the .cover extension was to tell Banshee or whatever app that we had a non-jpeg file that needed to be converted - which allowed us to de-couple the grabbing of the image in whatever format from the conversion. The bug here, afaict, is that the bit of code that took the .cover images and converted them to jpegs is broken. I'm working on a patch to fix that.
OK, svn up if you're running from trunk and tell me how it works. Sorry this took so long! Re-open if it doesn't work at all (works for me w/ the sample mp3 somebody posted on that RB bug). Keep in mind that if Banshee already has cover art for a track in the ~/.cache/album-art/ cache it won't try to pull it from within the file.
Looks like it's working, only that it's recoding the original jpeg files. I first I thought it didn't work because my 110ks jpeg file had been converted into a 40ks one. And Banshee doesn't support cover art in folders unless it's a .jpg file, but then, neither did it supported before.