GNOME Bugzilla – Bug 436319
iPod database async save
Last modified: 2007-07-27 07:24:47 UTC
This patch implements asynchronous iPod database save. iPod database saving can be quite slow (a few seconds on my iPod), doing it from the main thread blocks the UI, so it's better to do it asynchronously. This is what that patch does. It's a bit large (4 files changed, 809 insertions(+), 151 deletions(-)) because we must handle the fact that libgpod isn't thread-safe. Saving is implemented by marking the libgpod database as read-only, creating a thread to do the actual save, and queueing an idle callback to mark the database as read-write. There's a more detailed explanation of how it works at the beginning of the (new) rb-ipod-db.c file.
Created attachment 87646 [details] [review] Async iPod database save
I'll need to update that patch to integrate the changes made in bug #436744
Did you ever get anywhere with updating this Christophe? ... or are you waiting until the recent changes in libgpod CVS (specifically the 2007-05-06 background-copy with thread-safeness) appear in a stable release?
Nope, I haven't updated it yet, the only thing I'm waiting for to update it is free time ;) Rhythmbox already handles copying to the iPod on its own without libgpod, so the libgpod changes should be unnecessary.
Created attachment 89209 [details] [review] Update I finally updated it ;) Same patch as the previous one, except that RbIpodDB now supports adding/removing songs to/from playlists.
Created attachment 89212 [details] [review] A tested patch is better ;)
Using the latest patch (either with my iPod plugged in or not) I receive an error saying the iPod plugin could not be started. (Compiled against HEAD (5147) with libgpod 0.4.2)
There is probably a warning about a missing symbol in the console when rb tries to activate the ipod plug-in, could you paste the message ? Thanks for the testing ;)
Spot on :) (rhythmbox:2591): Rhythmbox-WARNING **: /usr/lib/rhythmbox/plugins/ipod/libipod.so: undefined symbol: itdb_schedule_save (rhythmbox:2591): Rhythmbox-WARNING **: Could not load plugin ipod (rhythmbox:2591): Rhythmbox-WARNING **: Error, impossible to activate plugin 'Portable Players - iPod'
Hmm, there is no such function call after applying that patch: $ grep schedule *.[hc] rb-ipod-db.c: rb_debug ("Database save already scheduled"); Could you try again after a make distclean and double-check that the patch applied properly?
OK, Tried it again against 5155 -- it applies with fuzz 2 to rb-ipod-source.c -- but otherwise now compiles and runs. Seems to read the DB OK and writes the play-count back for music i've played, but i've not had a chance to actually copy any music to/from it yet -- i'll update this bug when i've had an opportunity to try it. In summary, it doesn't seem to break anything yet :) -- is there any additional diagnostics / debug information I should look out for Christophe?
Thanks for the feedback, I'll commit that patch soon since moch and doctau said it looked fine on IRC. The thing to look out for when testing this patch is small inconsistencies between what rhythmbox shows and what is on the iPod, since the database saving is no longer synchronous with rhythmbox UI updates, this could happen even though I have tried to make sure it's not possible (except it there are IO errors when writing to the iPod). Crashes are also possible. In short, there is nothing special to test, just use rhythmbox and your iPod normally, and if you notice unusual/unexpected behaviour, just report it ;) Thanks!
I was going to suggest you committed it :) I'll try it out on a low-speed USB connection (I have a sacrificial iPod) too then, because RB frequently crashes with a synchronous DB when writing large amounts of data (say, 20-30 tracks at a time)
While I think of it, any chance we can get the patch in #411634 committed to trunk too? The patch works fine here, and does stop my 1GB Nano from filling up with duplicated tracks.
Committed to SVN, marking as fixed, I wouldn't be surprised if it got reopened though ;=)
*** Bug 460646 has been marked as a duplicate of this bug. ***