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 357400 - rhythmbox, album art, ipod
rhythmbox, album art, ipod
Status: RESOLVED FIXED
Product: rhythmbox
Classification: Other
Component: Plugins (other)
0.9.5
Other All
: Normal enhancement
: ---
Assigned To: RhythmBox Maintainers
RhythmBox Maintainers
Depends on:
Blocks: 338564 412853
 
 
Reported: 2006-09-24 01:12 UTC by Scott Harmon
Modified: 2008-03-03 12:39 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
51ipod-artwork.patch (2.45 KB, patch)
2007-02-21 23:08 UTC, Ed Catmur
none Details | Review
51ipod-artwork.patch (3.19 KB, patch)
2007-02-21 23:26 UTC, Ed Catmur
none Details | Review
51ipod-artwork.patch (3.20 KB, patch)
2007-02-22 20:59 UTC, Ed Catmur
none Details | Review
add itdb_track_set_thumbnails_from_pixbuf to libgpod (6.45 KB, patch)
2007-03-01 13:44 UTC, Christophe Fergeau
none Details | Review
51ipod-artwork.patch (3.29 KB, patch)
2007-03-04 23:37 UTC, Ed Catmur
none Details | Review
updated (3.91 KB, patch)
2007-03-26 08:09 UTC, James "Doc" Livingston
committed Details | Review
52ipod-artwork-new-gpod-api.patch (1.81 KB, patch)
2007-03-26 22:45 UTC, Ed Catmur
committed Details | Review

Description Scott Harmon 2006-09-24 01:12:46 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.
Comment 1 Alex Lancaster 2006-09-24 06:59:41 UTC
Add to iPod tracker bug.
Comment 2 Ed Catmur 2007-02-21 23:08:52 UTC
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).
Comment 3 Ed Catmur 2007-02-21 23:26:44 UTC
Created attachment 83076 [details] [review]
51ipod-artwork.patch

Updated to dispose stuff
Comment 4 Ed Catmur 2007-02-22 20:59:16 UTC
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!
Comment 5 James "Doc" Livingston 2007-02-23 07:37:48 UTC
I don't have an ipod that supports art to test with, but it looks sane to me.
Comment 6 Christophe Fergeau 2007-02-28 08:43:34 UTC
The latest patch works fine for me without any additional patch. And it looks nice :p
Comment 7 Christophe Fergeau 2007-03-01 10:44:37 UTC
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 ;)
Comment 8 Christophe Fergeau 2007-03-01 13:44:19 UTC
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 ;)
Comment 9 James "Doc" Livingston 2007-03-03 05:15:48 UTC
(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.
Comment 10 Ed Catmur 2007-03-03 18:08:52 UTC
(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.
Comment 11 Christophe Fergeau 2007-03-04 20:56:56 UTC
> 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.
Comment 12 Ed Catmur 2007-03-04 23:37:56 UTC
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?
Comment 13 Christophe Fergeau 2007-03-05 08:39:06 UTC
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.
Comment 14 James "Doc" Livingston 2007-03-21 07:19:29 UTC
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.
Comment 15 Christophe Fergeau 2007-03-21 08:38:59 UTC
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 ;)
Comment 16 James "Doc" Livingston 2007-03-26 08:09:06 UTC
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.
Comment 17 Christophe Fergeau 2007-03-26 12:36:22 UTC
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.
Comment 18 Ed Catmur 2007-03-26 22:45:42 UTC
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.
Comment 19 Christophe Fergeau 2007-03-27 07:46:27 UTC
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 ;)
Comment 20 Christophe Fergeau 2007-03-27 08:26:04 UTC
(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 ;)
Comment 21 James "Doc" Livingston 2007-03-27 09:57:00 UTC
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.
Comment 22 Christophe Fergeau 2007-03-27 12:37:05 UTC
(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. 

Comment 23 Christophe Fergeau 2007-03-29 18:44:11 UTC
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?
Comment 24 James "Doc" Livingston 2007-04-01 08:15:43 UTC
Looks fine to me, feel free to commit
Comment 25 Christophe Fergeau 2007-04-01 10:16:31 UTC
Ok, committed Ed's patch, keeping this bug open to remember we have something to do when libgpod get artwork support detection.
Comment 26 Christophe Fergeau 2007-05-06 11:06:44 UTC
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
Comment 27 Nicolò Chieffo 2008-03-03 12:19:12 UTC
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
Comment 28 Nicolò Chieffo 2008-03-03 12:33:32 UTC
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.
Comment 29 Christophe Fergeau 2008-03-03 12:39:21 UTC
This is bug #485535