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 520121 - MTP plugin fights with gvfs' gphoto2 backend
MTP plugin fights with gvfs' gphoto2 backend
Status: RESOLVED FIXED
Product: rhythmbox
Classification: Other
Component: Removable Media
unspecified
Other Linux
: Normal normal
: ---
Assigned To: RhythmBox Maintainers
RhythmBox Maintainers
Depends on: 519651 520120
Blocks:
 
 
Reported: 2008-03-03 16:03 UTC by Bastien Nocera
Modified: 2010-04-09 21:57 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
MTP devices mounted by gvs/libgphoto2 are recognized by RB (11.83 KB, patch)
2009-11-08 23:37 UTC, François Jaouen
rejected Details | Review
PSP detection fix (1013 bytes, patch)
2009-11-08 23:38 UTC, François Jaouen
committed Details | Review
Patch to autom. unmount/remount MTP device at RB startup/shutdown (8.50 KB, patch)
2009-11-25 18:43 UTC, François Jaouen
needs-work Details | Review
fix a case where GMount comes without GVolume associated (1.46 KB, patch)
2009-11-28 09:53 UTC, François Jaouen
needs-work Details | Review
patch v2 to manage unmount/remount of MTP devices (11.04 KB, patch)
2009-12-24 18:02 UTC, François Jaouen
needs-work Details | Review
Improve the opening of a just unmounted MTP device (1.87 KB, patch)
2010-03-21 15:39 UTC, François Jaouen
none Details | Review

Description Bastien Nocera 2008-03-03 16:03:10 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)
Comment 1 David Zeuthen (not reading bugmail) 2008-03-03 16:24:19 UTC
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.
Comment 2 David Zeuthen (not reading bugmail) 2008-03-03 16:42:12 UTC
(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?

Comment 3 Linus Walleij 2008-10-29 17:41:40 UTC
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.
Comment 4 Alan Clements 2009-01-10 17:24:47 UTC
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. 
Comment 5 François Jaouen 2009-11-08 23:35:52 UTC
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.
Comment 6 François Jaouen 2009-11-08 23:37:13 UTC
Created attachment 147231 [details] [review]
MTP devices mounted by gvs/libgphoto2 are recognized by RB
Comment 7 François Jaouen 2009-11-08 23:38:19 UTC
Created attachment 147232 [details] [review]
PSP detection fix
Comment 8 Jonathan Matthew 2009-11-09 05:37:21 UTC
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.
Comment 9 François Jaouen 2009-11-09 07:24:54 UTC
(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 ?
Comment 10 Jonathan Matthew 2009-11-09 09:41:29 UTC
I don't know. Using the gphoto2 gvfs mount just isn't a viable option.
Comment 11 François Jaouen 2009-11-10 07:14:15 UTC
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.)
Comment 12 Jonathan Matthew 2009-11-11 01:37:52 UTC
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.
Comment 13 Bastien Nocera 2009-11-11 01:50:18 UTC
(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?
Comment 14 François Jaouen 2009-11-11 12:01:57 UTC
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.
Comment 15 François Jaouen 2009-11-25 18:43:29 UTC
Created attachment 148470 [details] [review]
Patch to autom. unmount/remount MTP device at RB startup/shutdown
Comment 16 François Jaouen 2009-11-28 09:53:37 UTC
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.
Comment 17 Jonathan Matthew 2009-12-20 07:45:28 UTC
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.
Comment 18 Jonathan Matthew 2009-12-20 07:45:33 UTC
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.
Comment 19 Jonathan Matthew 2009-12-20 07:46:40 UTC
Review of attachment 148639 [details] [review]:

Instead of adding patches on top of patches, please just attach a new version of the full patch.
Comment 20 Jonathan Matthew 2009-12-20 07:46:42 UTC
Review of attachment 148639 [details] [review]:

Instead of adding patches on top of patches, please just attach a new version of the full patch.
Comment 21 Jonathan Matthew 2009-12-20 07:47:46 UTC
Oops, I impatiently clicked the publish button twice.  Both times.
Comment 22 François Jaouen 2009-12-24 18:02:01 UTC
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.
>
Comment 23 Jonathan Matthew 2009-12-29 11:23:05 UTC
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
Comment 24 Jonathan Matthew 2010-03-20 09:54:51 UTC
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.
Comment 25 François Jaouen 2010-03-21 15:39:54 UTC
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
Comment 26 Jonathan Matthew 2010-03-27 10:36:51 UTC
(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.
Comment 27 Marcus Meissner 2010-04-09 21:57:53 UTC
a simple gp_camera_exit() line in gvfs would btw help.

see:
https://bugzilla.gnome.org/show_bug.cgi?id=610261