Created attachment 170102 [details] [review]
patch to fix memory corruption in libgpod-sharp

Step 1: Fix memory corruption issues when accessing any of the glists exposed in libgpod.

Can I get this pushed upstream pretty please? ;)

Created attachment 170103 [details] [review]
Patch to banshee to add playlist support

Aaand done. That was easy :)

(In reply to comment #1)
> Step 1: Fix memory corruption issues when accessing any of the glists exposed
> in libgpod.

Do I understand the patch correctly that there is an iteration over the whole GList everytime something like Itdb.get_tracks() is called? This can get really costly with 10000 tracks in the databas if called too often. Wouldn't it be possible to get rid of the bindings for itdb_track_remove and similar, and to silently call these when a native C# remove is called on the container you use for the tracks, or something in that spirit ?

*** Bug 629982 has been marked as a duplicate of this bug. ***

Has this been fixed in 1.7.6?  because it ain't working for me.  My ipod classic syncs and the playlists show up in the ipod submenu of banshee, but when I eject they're not on the device.

(In reply to comment #5)
> Has this been fixed in 1.7.6?

The bug doesn't have the status FIXED. Does that answer your question?

Only brings up another.... why not?  There are patches for it in git before 1.7.6 was released. This has been a real killer for me and I'm ready to drop banshee, even though I like everything else. But thanks for the cheeky response.

There are a few things which had to be verified and some other issues that came up with a higher priority. It's a lot worse when Banshee doesn't work at all than when just some things don't perfectly work.

Banshee 1.8 (which is a stable release, 1.7 is an unstable series so it is perfectly possible things break) is set to be released somewhere next week and this issue will be fixed without a doubt.

Thank you.  I appreciate all the work you do in making a really great piece of software.  I waited patiently for a couple of weeks and was only a bit disappointed when the patch wasn't included in the recent release.  I look forward to 1.8.

(In reply to comment #7)
> Only brings up another.... why not?  There are patches for it in git before

In git? I guess you mean bugzilla, not git. If they were in git they would be marked here as "committed".

> But thanks for the cheeky response.

I just try to teach people how the bug lifecycle works if they don't seem to know.

Review of attachment 170103 [details] [review]:

Fine with me -- obviously blocks on the libgpod changes.

Hey,

Just to answer the original question, "Is this in 1.7.6", the answer is no. The reason was there was a very difficult to debug memory corruption issue. It took quite a lot of my free time to track down and by the time I had it figured out there wasn't enough time left for me to get everything committed and tested before 1.7.6, so I held off. Better to have no playlist support than have banshee crashing every time you try and sync anything.

Secondly, if you're getting any kind of playlists showing up you must not be running the AppleDevice extension. It still has zero support for playlists. Could you check in the banshee extensions list that "Apple Device" is there and is enabled? It sounds like you have the ipod-sharp plugin enabled.

Anyway, all the patches are ready now, I'm just doing some more testing to ensure there's definitely no memory corruption issues and then I'm going to upstream all the work. This will require another libgpod-sharp release as well as another banshee release. If you want to to help test things in the meantime, please do! Just compile banshee and libgpod-sharp from their respective repositories and let us know if everything works as expected.

I'll update this bug again when everything is upstreamed.

Review of attachment 170102 [details] [review]:

::: bindings/mono/libgpod-sharp/GPodList.cs
@@ +29,2 @@
 		protected HandleRef handle;
+		protected abstract GLib.List List {

Christopher is right, creating new Lists each time is not good.  This is painful b/c the GLib.List binding isn't complete (really doesn't support lists that change very well).

I think we should store the IntPtr to the List, and implement/bind Count and this[int] manually by calling g_list_length(list_ptr) g_list_nth_data (list_ptr, int).  GetEnumerator can be implemented using Count and this[].

Alan, do we really need to make a copy of the list before enumerating it to avoid collection-modified type issues (albeit in native memory land)?  Can we either add a lock or throw an exception if the user calls Insert/Remove while iterating, or just ignore it and assume users will use it properly?

Ok, talked to Alan about this on IRC some and looked deeper at GLib.List/ListBase -- I think I'd thought that it iterated the collection each time you made a new List, but it doesn't -- it just wraps the native pointer -- so the overhead is just a few dozen bytes of memory.

I asked Alan if we could get rid of the Enumerator list copy, and he thinks we can.  Given that, I think the rest of it is OK for now -- it's not ideal to create a new (relatively cheap) GLib.List each time Count or this[] is called, but it does fix the currently very broken code.

Plus, with his patch to Banshee, the only things we're actually using are the Enumerator and Add functions.

Hi,

To follow up, I do have the "Apple device Support" extension enabled (I also have MTP and Mass Storage Media Player support enabled, no libgpod is listed in the version info).  I'm happy to test out the newest and greatest releases, I'll add the Banshee daily build ppa repository to my sources list.

BTW, sorry I was confused regarding the status.  I had seen the suggestion of the patch being included in 1.7.6 and thought it was in git because all emails have "git master" near the header.  It also wasn't clear how important and related the memory issue was from the comment thread, but as a layman end user I'm glad you are all working so hard on this.

sorry libgpod is listed in version info

libgpod patch committed upstream: http://gtkpod.git.sourceforge.net/git/gitweb.cgi?p=gtkpod/libgpod;a=commit;h=c5c64bd98989074523e83a395b8efe7358e8c05d

(In reply to comment #17)
> libgpod patch committed upstream:
> http://gtkpod.git.sourceforge.net/git/gitweb.cgi?p=gtkpod/libgpod;a=commit;h=c5c64bd98989074523e83a395b8efe7358e8c05d

I think there's a second patch needed for playlist support which adds a constructor for ItdbPlaylist.

Created attachment 170962 [details] [review]
libgpod patch, return null if Podcasts playlist is null

Created attachment 170963 [details] [review]
libgpod patch, add Playlists ctor

Comment on attachment 170102 [details] [review]
patch to fix memory corruption in libgpod-sharp

Nathaniel, please mark patches at committed when appropriate.

Review of attachment 170962 [details] [review]:

A similar patch is probably needed for the master playlist since it might not exist when creating a new empty database from scratch using itdb_new (though I'm not sure people will try to do this)

Created attachment 170967 [details] [review]
libgpod patch, return null if Master playlist null

Created attachment 170968 [details] [review]
libgpod patch, return null if Master playlist null, v2 (fix whitespace)

Review of attachment 170963 [details] [review]:

::: bindings/mono/libgpod-sharp/Playlist.cs
@@ +130,3 @@
 								  set { ((Itdb_Playlist *) Native)->type = (byte) (value ? 1 : 0); } }
 
+		public Playlist(string name, bool smartPlaylist) : base (Itdb_Playlist.itdb_playlist_new (name, smartPlaylist)) {}

This is your call, but for now I'd just add a Playlist(string name) constructor since this boolean is unlikely to be enough to get smart playlist handling working. Moreover, you might want to expose smart playlists as deriving from Playlist. Maybe it won't make sense, but until you do know for sure how you want to handle smart playlist, I'd recommend not adding this boolean.

Once again, this is your call, if you want this patch to go in as is, I'm fine with it.

Created attachment 171054 [details] [review]
libgpod patch, add Playlists ctor (v2)

I agree with your advice; updated patch attached.

I feel there are certain things not yet working as expected which is why I am reopening this issue.

What works:
* Add several songs to the iPod.
* Create a playlist on the iPod.
* Select the songs from the Music category on the iPod and add them to the playlist created in the previous step.

What doesn't work:
* Create a playlist in Banshee. (Just a regular one, I did not test a smart one.)
* Drag & drop the newly created playlist on the iPod.

The songs were transferred correctly but the playlist was not created on the device and thus the songs were not added to it.

Jensen, AFAICT what you describe has never worked with any DAP backend.  Please see if there's already a bug filed about it, and if not file one.  Re-closing since AppleDevice at least isn't regressing compared to other DAP support now.

Looks like the functionality described in Comment 27 is actually Bug 551604 -- a fairly popular request, but not a regression with AppleDevice.