GNOME Bugzilla – Bug 520121
MTP plugin fights with gvfs' gphoto2 backend
Last modified: 2010-04-09 21:57:53 UTC
Using rhythmbox 0.11.4 (and a recent libmtp/libgphoto2) 1. Plugin in an MTP device 2. Nautilus mounts the device as a gphoto2 gvfs mount, and launches Rhythmbox 3. Rhythmbox tries to access the device, but can't because the device is busy Rhythmbox should be using gvfs to access the underlying filesystem. The only thing missing would be for Rhythmbox to fetch the supported file types from HAL's "portable_audio_player.output_formats" property (which might need serious updating). It would also need to detect the music directory itself, probably using the same naive code libmtp does: /* Is this the Music Folder */ if (!strcasecmp(oi->Filename, "My Music") || !strcasecmp(oi->Filename, "Music")) { device->default_music_folder = params->handles.Handler[i]; } (Same thing for the default playlists folder)
Funny, I just filed bug 520026 about making it easier to claim the USB device because from an app because I thought one of the things libmtp had going for it was that it could find the folders. Anyway, my view is that it's more useful to use the gphoto2 backend as the user can the use the device from other apps (such as Nautilus and/or Totem) when RB is running. Also, hopefully gio is a slightly nicer API to work with than libmtp. We'd need to get write support for the gphoto2 backend committed so I'm making bug 519651 a dep of this one.
(In reply to comment #0) > The only thing missing would be for Rhythmbox to fetch the supported file types > from HAL's "portable_audio_player.output_formats" property (which might need > serious updating). Sure we can do this; it's easy to get this information in the backend from HAL. I wonder what's the best way to export this. Probably as filesystem attributes? e.g. we could have x-content::audio-player::music-folder x-content::audio-player::playlists-folder x-content::audio-player::photo-folder x-content::audio-player::video-folder .. x-content::audio-player::output-formats Something like that?
The problems are delicate since it has several components: * libmtp claims a subset of the devices that libgphoto2 can support, so there really is some competition as to who should capture it. libmtp does not attempt to access plain cameras doing just PTP, just media devices doing MTP, really. libgphoto2 claims all devices. * libmtp cannot do everything libgphoto2 does and vice versa... libgphoto2 will try to handle even non-PTP devices that libmtp can never handle even if it wish to. * PTP/MTP devices aren't really filesystems... I wrote about it in private conversation with Bastien & David.
libgphoto2 is declaring my MP3 player (Archos 605 WiFi) a photo-CD because the root directory contains a Microsoft Icon file and a PNG. This causes Nautilus to want to open F-Spot. This despite the fact that HAL reports it as a USB mass storage device.
I have the same problem (rb doesn't see MTP device mounted by gvfs/libgphoto2) with my Creative Zen Touch. I propose a patch to correct this. Unfortunately it is just some kind of proof of concept, because for the moment it is very very slow and in fact unusable for release (I evaluate to nearly one hour the time to read the content of my 20gb Zen Touch which contains about 2000 songs). Anyway I would like to know if I'm on the right direction and if I should continue my investigations. The basic idea of the patch is : gvfs/libgphoto2 has already mounted the MTP device, so instead of using raw libmtp, just use generic-player plugin as for UMS devices. At implementation level : - mpid-device has been modified to rely on GIO mount and device objects (see device->mount) instead of unix mount (gvfs mounts are not "unix" mount) - rb-removable-media-manager has a hash table for all kind of removable stuff (volume, mount or device). This prevents rb to crash in libmtp plugin. See all_mounts in private section. Beside this patch, I've also found something strange in PSP detection routine but maybe it is to prevent execution of non working code ? Anyway I attach another patch to adress this detail. I've also fixed a problem in /lib/udev/rules.d/90-usb-media-players.rules : line 2, SUBSYSTEM!="block", GOTO="media_player_end" must be commented out because MTP devices are not block devices.
Created attachment 147231 [details] [review] MTP devices mounted by gvs/libgphoto2 are recognized by RB
Created attachment 147232 [details] [review] PSP detection fix
I'm really not interested in this approach. Besides being much slower, it prevents us providing several features (such as playlists and album art) and means we don't get useful information on the formats supported by the device.
(In reply to comment #8) > I'm really not interested in this approach. Besides being much slower, it > prevents us providing several features (such as playlists and album art) and > means we don't get useful information on the formats supported by the device. I understand your point but how do you think this bug should be solved ? - unmount MTP devices when rhythmbox is launched ? - ask people of gvfs/libgphoto2 to get their hands off such devices ? I think about newcomers to GNU/linux that can't see their MTP device in RB, while they have been invited to launch it when they plugged it in ?
I don't know. Using the gphoto2 gvfs mount just isn't a viable option.
Jonathan, Well, I consider this problem a real stopper for newcomers and in complete contradiction to GNOME philosophy of simplicity and ease of use. This issue has been raised for about 1.5 year and has been already discussed (c.f. http://mail.gnome.org/archives/gvfs-list/2009-February/msg00008.html, http://mail.gnome.org/archives/gvfs-list/2009-May/msg00001.html). I understand that it is not your top prioriy problem, but it is to me and I really want to help because I like RB and I am not very happy with the project to replace it with Banshee in Ubuntu (c.f. https://wiki.ubuntu.com/DesktopTeam/Specs/Karmic/DefaultMediaPlayerChoice) so I propose the following : 1/ short time fix : automatically unmount MTP devices when libmtp fails to grab them. 2/ study in depth the missing features of gphoto2 gvfs-backend in order to provide a good layer for MTP devices. (btw, I can already say that speed is not a problem : gvfs-ls lists 2000 songs in less than 14 seconds.)
Using gphoto2 is not a viable option because libgphoto2 only speaks basic PTP, not the MTP extensions, which means it doesn't provide access to song metadata, so we have to read it from the files directly, which is extremely slow. It also can't do album art or playlists, and it can't tell us what audio formats the device supports. I doubt the gphoto2 developers are interested in turning it into a combined PTP and MTP library. The options then seem to be that either we get the gphoto2 gvfs mount out of the way so we can talk to the device, or we use complicated magic to share the device. Maybe we can add a backend to libmtp that sends and receives PTP packets via the gphoto2 gvfs mount daemon? I don't really know enough about PTP, gphoto2, MTP, or libmtp to know if this is at all possible. If we're going to remove the gphoto2 gvfs mount, we need to make sure we can correctly identify the individual gphoto2 gvfs mount, and we should only do this when a new rhythmbox process was started to deal with the device. Otherwise I think we run too much risk of unmounting a device that is in use by some other process. This seems a bit messy, but it seems to be the only option that doesn't involve substantially redesigning multiple projects that don't necessarily care what we want.
(In reply to comment #12) > Using gphoto2 is not a viable option because libgphoto2 only speaks basic PTP, > not the MTP extensions, which means it doesn't provide access to song metadata, > so we have to read it from the files directly, which is extremely slow. It > also can't do album art or playlists, and it can't tell us what audio formats > the device supports. > > I doubt the gphoto2 developers are interested in turning it into a combined PTP > and MTP library. But davidz was interested in making the gvfs backend be able to handle both MTP and PTP. I'm not sure what's come of it in his head, as I'm still waiting on feedback of a discussion we had with Linus Wallej (libmtp developer)... So, David, what's the plan?
libgphoto2, despite its name, aims at adressing also MTP devices. In libmtp web site you can read that the objective is to merge libmtp into gphoto2. Both projects already share the same MTP backend and, as far as I understand, the main difference is the API ; libmtp being more raw MTP oriented. I thought about making a gvfs-libmtp backend, but because of libmtp objective, it doesn't seem a durable solution. Extending gphoto2 backend seems more useful. I agree with Bastien, David's lights are needed there as well as Linus Wallej ones. For the moment, I'm working on the workaround of unmounting MTP devices in RB with lot of precautions for not unmounting anything.
Created attachment 148470 [details] [review] Patch to autom. unmount/remount MTP device at RB startup/shutdown
Created attachment 148639 [details] [review] fix a case where GMount comes without GVolume associated While testing in depth the previous patch, it happens that sometimes GMount has no GVolume associated. It doesn't result an unexpected result but yields a nasty error message from GObject layer. This patch should be added to the previous one.
Review of attachment 148470 [details] [review]: ::: plugins/mtpdevice/rb-mtp-plugin.c @@ +373,3 @@ + /* check device is a media player */ + if (g_udev_device_get_property(G_UDEV_DEVICE (device), "ID_MEDIA_PLAYER") == NULL) { Why are you adding this check here? I don't think it's correct. ID_MEDIA_PLAYER won't necessarily be set for MTP devices. @@ +408,3 @@ + rb_debug ("device matched, checking if device is not mounted"); + /* Check if the device is already mounted */ + GMount* mount = rb_mtp_plugin_get_mount(G_UDEV_DEVICE (device)); variables go at the start of the block @@ +416,3 @@ + return NULL; + } + else { 'else' goes on the same line as the closing brace @@ +420,3 @@ + rb_debug("device is not mounted, creating source"); + LIBMTP_raw_device_t *raw_dev = g_malloc(sizeof (LIBMTP_raw_device_t)); + *raw_dev = raw_devices[i]; The source should take a copy of the device instead @@ +563,3 @@ + +/* Global notification of unmounted device (system wide) */ +static void rb_mtp_unmount_notification_cb (GVolumeMonitor *monitor, missing newline after the return type @@ +573,3 @@ + if (g_list_find(plugin->unmounted_volumes, volume)) { + rb_debug("unmounted volume found, force a new scan"); + RBRemovableMediaManager *mgr; variables go at the start of the block @@ +574,3 @@ + rb_debug("unmounted volume found, force a new scan"); + RBRemovableMediaManager *mgr; + g_object_get (G_OBJECT (plugin->shell), don't need the G_OBJECT cast here @@ +583,3 @@ + +/* Local notification of the unmount operation */ +static void rb_mtp_unmount_done_cb(GObject *object, GAsyncResult *result, gpointer pplugin) missing newline after the return type @@ +593,3 @@ + /* Remember the volume to remount it when quitting */ + GVolume *volume = g_mount_get_volume(mount); + if (volume != NULL) this is hard to read without braces @@ +600,3 @@ + * 'mount_removed_cb' callback in re_removable-media-manager */ + } + else { 'else' goes on the previous line @@ +601,3 @@ + } + else { + rb_debug("fail to unmount %s", mountpoint); what's going to happen in this error case? anything we should report to the user? @@ +622,3 @@ + GList *mounts = g_volume_monitor_get_mounts(volmon); + GList *el; + for (el = mounts; el != NULL && mount == NULL; el = el->next) { I really don't like the loop condition here. break out of the loop when you find a mount instead. @@ +625,3 @@ + GMount *elm = G_MOUNT(el->data); + GVolume *elv = g_mount_get_volume(elm); + if (elv == NULL) continue; 'continue' goes on the next line @@ +654,3 @@ + char *volpath = g_volume_get_identifier(volume, G_VOLUME_IDENTIFIER_KIND_UNIX_DEVICE); + rb_debug("remounting %s", volpath); + g_volume_mount(volume, G_MOUNT_MOUNT_NONE, NULL, NULL, NULL, NULL); what happens if this fails? ::: plugins/mtpdevice/rb-mtp-thread.c @@ +113,3 @@ g_free (task->filename); g_free (task->name); + g_free (task->raw_device); the device thread doesn't own the raw device, so it shouldn't be freeing it. the source owns it, so it should clean it up.
Review of attachment 148639 [details] [review]: Instead of adding patches on top of patches, please just attach a new version of the full patch.
Oops, I impatiently clicked the publish button twice. Both times.
Created attachment 150355 [details] [review] patch v2 to manage unmount/remount of MTP devices (In reply to comment #18) > Review of attachment 148470 [details] [review]: I've adressed all the issues of the previous patch. Merry chrismas mighty RB developpers :) > > ::: plugins/mtpdevice/rb-mtp-plugin.c > @@ +373,3 @@ > > + /* check device is a media player */ > + if (g_udev_device_get_property(G_UDEV_DEVICE (device), "ID_MEDIA_PLAYER") > == NULL) { > > Why are you adding this check here? I don't think it's correct. > ID_MEDIA_PLAYER won't necessarily be set for MTP devices. No it isn't necessary. It was an attempt to avoid a useless scan of libmtp raw devices, but I agree that it may reduce the set of MTP devices. Removed. > > @@ +408,3 @@ > + rb_debug ("device matched, checking if device is not mounted"); > + /* Check if the device is already mounted */ > + GMount* mount = rb_mtp_plugin_get_mount(G_UDEV_DEVICE (device)); > > variables go at the start of the block I've corrected all the stylish issues. > @@ +420,3 @@ > + rb_debug("device is not mounted, creating source"); > + LIBMTP_raw_device_t *raw_dev = g_malloc(sizeof > (LIBMTP_raw_device_t)); > + *raw_dev = raw_devices[i]; > > The source should take a copy of the device instead Corrected, though I've found some issues of the current thread design (I am working on another fix to address it) > @@ +574,3 @@ > + rb_debug("unmounted volume found, force a new scan"); > + RBRemovableMediaManager *mgr; > + g_object_get (G_OBJECT (plugin->shell), > > don't need the G_OBJECT cast here Fixed as well of all unneeded G_OBJECT cast ;) > @@ +601,3 @@ > + } > + else { > + rb_debug("fail to unmount %s", mountpoint); > > what's going to happen in this error case? anything we should report to the > user? No, I don't thing user report is useful (see comment in patch) > > @@ +625,3 @@ > + GMount *elm = G_MOUNT(el->data); > + GVolume *elv = g_mount_get_volume(elm); > + if (elv == NULL) continue; > > 'continue' goes on the next line 'continue' removed because a continue and a break in the same loop give me headache. btw there was a potential memory leak. > > @@ +654,3 @@ > + char *volpath = g_volume_get_identifier(volume, > G_VOLUME_IDENTIFIER_KIND_UNIX_DEVICE); > + rb_debug("remounting %s", volpath); > + g_volume_mount(volume, G_MOUNT_MOUNT_NONE, NULL, NULL, NULL, NULL); > > what happens if this fails? Nothing RB is quitting so we can't do much. >
Review of attachment 150355 [details] [review]: The changes in rb-mtp-source.c are no longer needed after a patch I committed a few hours ago. ::: plugins/mtpdevice/rb-mtp-plugin.c @@ +106,3 @@ #if defined(HAVE_GUDEV) static RBSource* create_source_device_cb (RBRemovableMediaManager *rmm, GObject *device, RBMtpPlugin *plugin); +static GMount* rb_mtp_plugin_get_mount(GUdevDevice *device); more code style nits: there needs to be a space between the function name and its arguments, pretty much everywhere @@ +278,3 @@ #if defined(HAVE_GUDEV) + g_list_foreach (plugin->unmounted_volumes, (GFunc)rb_mtp_volume_remount, NULL); + g_list_free (plugin->unmounted_volumes); should unref the volumes here, using rb_list_destroy_free (plugin->unmounted_volumes, (GDestroyNotify)g_object_unref); @@ +392,3 @@ + rb_debug ("device matched, checking if device is not mounted"); + /* Check if the device is already mounted */ + mount = rb_mtp_plugin_get_mount(G_UDEV_DEVICE (device)); 'device' in this function is now a GUdevDevice, so the cast isn't needed any more @@ +402,3 @@ + /* Device is free, create a source */ + rb_debug("device is not mounted, creating source"); + RBSource *source = rb_mtp_source_new (plugin->shell, RB_PLUGIN (plugin), &raw_devices[i]); variable declarations go at the start of the block @@ +587,3 @@ + * 'rb_mtp_unmount_notification_cb' callback */ + } + else { 'else' goes on the previous line @@ +615,3 @@ + GMount *elm = G_MOUNT(el->data); + GVolume *elv = g_mount_get_volume(elm); + char* devname = NULL; could move the declaration of devname inside the 'if' block @@ +622,3 @@ + mount = elm; + g_free(devname); + g_object_unref(elv); could unref 'elv' after the g_volume_get_identifier call, so it doesn't need to be done in both blocks @@ +629,3 @@ + } + } + g_object_unref(elm); remove this, it doesn't clean up the list properly when the loop exits early @@ +631,3 @@ + g_object_unref(elm); + } + g_list_free(mounts); can clean up the list properly with 'rb_list_destroy_free (mounts, (GDestroyNotify) g_object_unref);' @@ +639,3 @@ +rb_mtp_plugin_unmount(RBMtpPlugin *plugin, GMount *mount) +{ + g_mount_unmount_with_operation(mount, G_MOUNT_UNMOUNT_NONE, NULL, NULL, rb_mtp_unmount_done_cb, plugin); need to check the glib version before calling this as it was added in 2.22, while we only require 2.16. #if GLIB_CHECK_VERSION(2,22,0) g_mount_unmount_with_operation(...); #else g_mount_unmount(...); #endif ::: plugins/mtpdevice/rb-mtp-thread.c @@ +115,3 @@ g_free (task->filename); g_free (task->name); + task->raw_device = NULL; there isn't much of a need to do this - the task is being freed anyway
I've committed a distant relative of this patch - I moved the gvfs unmount/remount stuff inside RBMtpSource, so the plugin doesn't have to track multiple volumes. It appears to work for me. If some kind of device sharing scheme comes along in the future, we can work on that in another bug.
Created attachment 156675 [details] [review] Improve the opening of a just unmounted MTP device My apologies for not having answer to your last comments sooner. I'm glad to see that you finally tackled this awkward problem between gvfs and RB. I tried your modification to unmount/remount MTP devices but I encountered the following problem : despite RB is notified of the unmount, the device is not available and libmtp fails to open it. I propose a patch to correct this, that basically retries up to 5 times to open the device waiting 1 sec. between each try. It now works well with 2 different MTP devices on my computer after 1 or 2 retries. Btw, there is an issue with unmount_with_operation not available for glib version lower than 2.22. RB requires only 2.18. François Jaouen
(In reply to comment #25) > Created an attachment (id=156675) [details] [review] > I propose a patch to correct this, that basically retries up to 5 times to open > the device waiting 1 sec. between each try. > > It now works well with 2 different MTP devices on my computer after 1 or 2 > retries. Thanks for trying this out. I committed a different patch that has the same effect. > Btw, there is an issue with unmount_with_operation not available for glib > version lower than 2.22. RB requires only 2.18. That's what the #defines at the top of rb-mtp-source.c are for.
a simple gp_camera_exit() line in gvfs would btw help. see: https://bugzilla.gnome.org/show_bug.cgi?id=610261