GNOME Bugzilla – Bug 357400
rhythmbox, album art, ipod
Last modified: 2008-03-03 12:39:21 UTC
It would be nice if rhythmbox also transfered the album art (could be a setting, on by default) along with songs to the ipod. libgpod has this functionality AFAIK since gtkpod supports it.
Add to iPod tracker bug.
Created attachment 83075 [details] [review] 51ipod-artwork.patch Demo patch, currently untested. May require recent album art plugin improvements to supply the required artwork (see bug 363694).
Created attachment 83076 [details] [review] 51ipod-artwork.patch Updated to dispose stuff
Created attachment 83125 [details] [review] 51ipod-artwork.patch So that's why it wasn't working. I'm such a tool. This version really really works!
I don't have an ipod that supports art to test with, but it looks sane to me.
The latest patch works fine for me without any additional patch. And it looks nice :p
On second thought, this part of the patch: + g_return_if_fail (G_VALUE_HOLDS (metadata, GDK_TYPE_PIXBUF)); + g_return_if_fail ((pixbuf = GDK_PIXBUF (g_value_get_object (metadata)))); + g_return_if_fail (gdk_pixbuf_save_to_buffer (pixbuf, &image_data, &image_data_len, "jpeg", NULL, "quality", "100", NULL)); + g_hash_table_remove (priv->artwork_request_map, entry); + itdb_track_set_thumbnails_from_data (song, (guchar *) image_data, image_data_len); worries me. First, g_return_if_fail are macros which can go away if the appropriate flags are passed to the preprocessor, so it's risky to put real code in there (if other parts of rb already does that, you probably can ignore that comment). It would probably be less expensive CPU wise (but more memory hungry) to use "tiff" or "bmp" in the call to gdk_pixbuf_save_to_buffer. Though the right thing to do here is to add an itdb_track_set_thumbnails_from_pixbuf in libgpod ;)
Created attachment 83637 [details] [review] add itdb_track_set_thumbnails_from_pixbuf to libgpod For reference, here is such an untested patch for libgpod (totally off-topic here, I know ;)
(In reply to comment #7) > On second thought, this part of the patch: > > + g_return_if_fail (G_VALUE_HOLDS (metadata, GDK_TYPE_PIXBUF)); > + g_return_if_fail ((pixbuf = GDK_PIXBUF (g_value_get_object > (metadata)))); > + g_return_if_fail (gdk_pixbuf_save_to_buffer (pixbuf, &image_data, > &image_data_len, "jpeg", NULL, "quality", "100", NULL)); > + g_hash_table_remove (priv->artwork_request_map, entry); > + itdb_track_set_thumbnails_from_data (song, (guchar *) image_data, > image_data_len); > > worries me. First, g_return_if_fail are macros which can go away if the > appropriate flags are passed to the preprocessor, so it's risky to put real > code in there (if other parts of rb already does that, you probably can ignore > that comment). I agree, g_return_if_fail should only be used to check function pre-conditions - if any other code in RB has other stuff there, pleas file bugs. So the first of these is fine, but the other two should be done using if tests. > It would probably be less expensive CPU wise (but more memory > hungry) to use "tiff" or "bmp" in the call to gdk_pixbuf_save_to_buffer. Though > the right thing to do here is to add an itdb_track_set_thumbnails_from_pixbuf > in libgpod ;) It would get rid of potential JPEG conversion artifacts.
(In reply to comment #8) > Created an attachment (id=83637) [edit] > add itdb_track_set_thumbnails_from_pixbuf to libgpod > > For reference, here is such an untested patch for libgpod (totally off-topic > here, I know ;) Submit it to the gtkpod tracker on sourceforge and post the URL.
> Submit it to the gtkpod tracker on sourceforge and post the URL. > I already sent some mails about it on gtkpod-devel mailing list, see http://news.gmane.org/find-root.php?group=gmane.comp.ipod.gtkpod&article=2083 , I mentioned the patch here as well to make sure you knew about it.
Created attachment 83935 [details] [review] 51ipod-artwork.patch Use g_return_if_fail etc. appropriately. I think saving in jpeg has some benefits - disk space might not be that important, but transfer time matters. I can't see JPEG artifacts mattering much on an iPod display. Any idea what iTunes uses?
Ah, I think there's a misunderstanding about what happens in libgpod when generating artwork for the ipod. The jpeg encoded image doesn't end up on the ipod, it's just an intermediary format that libgpod will then decode to get a GdkPixbuf, and then convert to a format suitable for the iPod (it only supports unpacked RGB565 and a few other dumb formats like that one). So this intermediary jpeg conversion will only lead to increased cpu consumption, some RAM savings (maybe they'll be significant, I dunno) but that's about it.
The patch looks okay to me, but I can't test it as I don't have a art-capable iPod. I could commit it and leave the bug open for using the new libgpod pixbuf API if you want.
Sounds fine. I should write an additionnal patch to use the new libgpod api, this would be a good occasion to test the libgpod patch ;)
Created attachment 85300 [details] [review] updated Updated to SVN and had a few minor things fixed. Assuming this still works (I can't test), I'll commit it to svn. Also, can libgpod tell us whether art is supported on the iPod? If it can, we could avoid doing the work for iPods that don't support it.
I quickly tested this patch, and it's still working (I haven't tested cover writing though). I couldn't find an easy way to tell if art is supported on the ipod or not, I'll try to add a function to determine that to libgpod.
Created attachment 85356 [details] [review] 52ipod-artwork-new-gpod-api.patch Patch for the new libgpod api Christophe added. Hope I got the autofoo right.
I think in both cases (the old code path and the new code path using the new API), a ref to the pixbuf is leaked (I assume g_value_get_object raises the gobject reference count by one). This made me realize libgpod should document what it does with GdkPixbuf ref counts in the new functions ;)
(In reply to comment #16) > Also, can libgpod tell us whether art is supported on the iPod? If it can, we > could avoid doing the work for iPods that don't support it. > For what it's worth, I just sent http://article.gmane.org/gmane.comp.ipod.gtkpod/2131 I guess I'll have to convince the libgpod maintainer that he should make a release soon ;)
g_value_get_object() doesn't raise the refcount by one, g_value_dup_object() does that. I've committed the art-syncing patch to svn, I'll commit the one to sue the new libgpod API if it looks fine.
(In reply to comment #18) > Hope I got the autofoo right. With libgpod HEAD installed, it correctly uses the new _pixbuf function, I haven't tested with an older libgpod.
And it works fine with an older libgpod installed, is that ok to commit, or should we wait until we have the function to check if the iPod supports artwork in libgpod?
Looks fine to me, feel free to commit
Ok, committed Ed's patch, keeping this bug open to remember we have something to do when libgpod get artwork support detection.
The function itdb_device_supports_artwork has been added to libgpod CVS HEAD, and the following commit made rhythmbox optionnally use it: 2007-04-23 Christophe Fergeau <teuf@gnome.org> * plugins/ipod/rb-ipod-source.c: use the new itdb_device_supports_artwork method if using libgpod CVS. This allows us to detect art capable iPods
I'm using 0.11.4-0ubuntu6 and the artwork is copied only if it is under ~/.gnome2/rhythmbox/covers, and not if it is under the same directory as the music file, as cover.jpg
I'm sorry I made a mistake: the artwork (both cover.jpg and the downloaded one) is copied ONLY if the current copied song was playing during the transfer.
This is bug #485535