also reported at: https://bugs.launchpad.net/ubuntu/+source/empathy/+bug/585151

It seems like it should also be possible to select the "input", e.g. "Composite0", "Composite1", "S-Video".  On my webcam, I must use "Composite1".

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

agreed!

http://cgit.collabora.com/git/user/pochu/empathy.git/log/?h=multiple-cameras

+GList * empathy_camera_monitor_get_cameras (EmpathyCameraMonitor *self);

Not const GList*?

+typedef struct _EmpathyCamera
+{
+ gchar *id;
+ gchar *device;
+ gchar *name;
+} EmpathyCamera;

You can just do:

typedef struct { ... } Foo;

+ l = g_queue_find_custom (&self->priv->cameras, id, empathy_camera_find);
+
+ camera = l->data;

This segfault if it's not found.

Does the CheeseCameraMonitor signal camera-added for each camera when you instantiate it? As in, if you plug in a camera, then create a CheeseCameraMonitor, will it signal for that camera?

+ GQueue cameras;

You don't free the GList in here.

+ signals[CAMERA_ADDED] =
+ g_signal_new ("added", G_OBJECT_CLASS_TYPE (klass),
+ G_SIGNAL_RUN_LAST | G_SIGNAL_ACTION,
+ 0, NULL, NULL,
+ g_cclosure_marshal_VOID__POINTER,
+ G_TYPE_NONE, 1, G_TYPE_POINTER);

It'd probably be nicer to make EmpathyCamera a boxed type, but whatever.

The two commits "VideoSrc: add API to change the input device" and "VideoSrc: add API to get the input device" are mixed together by accident.

+ const gchar *device;
+
+ g_object_get (priv->src, "device", &device, NULL);

Surely this'll be a gchar* not a const gchar*? g_object_get returns new references/copies.

This EmpathyCameraMenu looks familiar!

+ for (; cameras != NULL; cameras = g_list_next (cameras))

g_list_next makes me sad, but oh well.

+ <_description>Default camera device to use in video calls, e.g. /dev/video0.</_description>

Wooaaahh, doesn't this mean that the default camera depends on the order in which the cameras are added?!

+ g_signal_handlers_block_by_func (settings,
+     empathy_camera_menu_prefs_camera_changed_cb, self);
+ gtk_toggle_action_set_active (GTK_TOGGLE_ACTION (action), TRUE);
+ g_signal_handlers_unblock_by_func (settings,
+     empathy_camera_menu_prefs_camera_changed_cb, self);

I actually found the g_signal_handlers_block_by_func stuff didn't work for the mic menu so added that priv->in_update gboolean. You should probably use one or the other way to prevent calling that callback, not both.

+ /* Do as if the gsettings key had changed, so we select the key that
+  * was last set. */
+ empathy_camera_menu_prefs_camera_changed_cb (self->priv->settings,
+     EMPATHY_PREFS_CALL_CAMERA_DEVICE, self);

I don't really like this. What if your preferred camera isn't connected? Your call will just stop as you're setting device=/dev/video101 but there is no fallback when it fails? You should at least check to see whether this device exists.

Also, what happens visually when you change the video device?

(In reply to comment #5)
> +GList * empathy_camera_monitor_get_cameras (EmpathyCameraMonitor *self);
> 
> Not const GList*?

Right, fixed.

> +typedef struct _EmpathyCamera
> +{
> + gchar *id;
> + gchar *device;
> + gchar *name;
> +} EmpathyCamera;
> 
> You can just do:
> 
> typedef struct { ... } Foo;

Aha, changed.

> + l = g_queue_find_custom (&self->priv->cameras, id, empathy_camera_find);
> +
> + camera = l->data;
> 
> This segfault if it's not found.

If it's not found we've already failed. I have added a g_return_if_fail (l != NULL) to fail gracefully though.

> Does the CheeseCameraMonitor signal camera-added for each camera when you
> instantiate it? As in, if you plug in a camera, then create a
> CheeseCameraMonitor, will it signal for that camera?

Good question! CheeseCameraMonitor does that when we call cheese_camera_device_monitor_coldplug() (which we do from EmpathyCameraMonitor's _constructed()). The problem is that EmpathyCameraMonitor is a singleton, so if somebody else already created an EmpathyCameraMonitor and then you create one, you're actually getting a ref to an existing EmpathyCameraMonitor, and so cheese_camera_device_monitor_coldplug() isn't called and you don't get the ::added signals for existing cameras. That's why I'm calling empathy_camera_monitor_get_cameras() when EmpathyCameraMenu is constructed, but we shouldn't need that, and it's currently error-prone (if we removed the camera-monitor from empathy-call-window, we would get the ::added signals and we would add the cameras twice).

I'm not sure how to best handle this. Perhaps by adding an empathy_camera_monitor_new() which doesn't return the singleton but always a new object, and keeping empathy_camera_monitor_dup_singleton() for those cases where we don't care about the signals?

> + GQueue cameras;
> 
> You don't free the GList in here.

Fixed. I wonder if I should dynamically allocate the GQueue, free it on _dispose and set it to NULL, and on ::added, check if it's != NULL before adding EmpathyCameras to it? So that we don't add cameras between _dispose() and _finalize().

> + signals[CAMERA_ADDED] =
> + g_signal_new ("added", G_OBJECT_CLASS_TYPE (klass),
> + G_SIGNAL_RUN_LAST | G_SIGNAL_ACTION,
> + 0, NULL, NULL,
> + g_cclosure_marshal_VOID__POINTER,
> + G_TYPE_NONE, 1, G_TYPE_POINTER);
> 
> It'd probably be nicer to make EmpathyCamera a boxed type, but whatever.

Done.

> The two commits "VideoSrc: add API to change the input device" and "VideoSrc:
> add API to get the input device" are mixed together by accident.

Oops, fixed.

> + const gchar *device;
> +
> + g_object_get (priv->src, "device", &device, NULL);
> 
> Surely this'll be a gchar* not a const gchar*? g_object_get returns new
> references/copies.

Fixed.

> This EmpathyCameraMenu looks familiar!

:D

> + for (; cameras != NULL; cameras = g_list_next (cameras))
> 
> g_list_next makes me sad, but oh well.

shrug. I don't really mind either way.

> + <_description>Default camera device to use in video calls, e.g.
> /dev/video0.</_description>
> 
> Wooaaahh, doesn't this mean that the default camera depends on the order in
> which the cameras are added?!

If you unplug and plug your cameras again in a different order, yes. I don't expect people from doing that all the time, and even if they do, at the moment they unplug the selected camera I think it's right for us to make another camera the preferred one. Anyway if you have a better idea on how to store this please speak up :)

> + g_signal_handlers_block_by_func (settings,
> +     empathy_camera_menu_prefs_camera_changed_cb, self);
> + gtk_toggle_action_set_active (GTK_TOGGLE_ACTION (action), TRUE);
> + g_signal_handlers_unblock_by_func (settings,
> +     empathy_camera_menu_prefs_camera_changed_cb, self);
> 
> I actually found the g_signal_handlers_block_by_func stuff didn't work for the
> mic menu so added that priv->in_update gboolean. You should probably use one or
> the other way to prevent calling that callback, not both.

For me it's this priv->in_update thing what doesn't work in this case! Because _activated_cb is called after this function returns, and not right when I call gtk_toggle_action_set_active(). Blocking the signal handler works fine in this case though. When _activated_cb calls g_settings_set_string, empathy_camera_menu_prefs_camera_changed_cb() won't be called again (if it did we'd hit an infinite loop).

> + /* Do as if the gsettings key had changed, so we select the key that
> +  * was last set. */
> + empathy_camera_menu_prefs_camera_changed_cb (self->priv->settings,
> +     EMPATHY_PREFS_CALL_CAMERA_DEVICE, self);
> 
> I don't really like this. What if your preferred camera isn't connected? Your
> call will just stop as you're setting device=/dev/video101 but there is no
> fallback when it fails? You should at least check to see whether this device
> exists.

If it isn't connected, it won't be in the menu and so we won't activate it. In that case we should probably activate another camera (e.g. the first one). Agreed?

> Also, what happens visually when you change the video device?

If you are sending video and have the video preview shown, that'll change to show the new device's capture.

(In reply to comment #6)
> > Does the CheeseCameraMonitor signal camera-added for each camera when you
> > instantiate it? As in, if you plug in a camera, then create a
> > CheeseCameraMonitor, will it signal for that camera?
> 
> Good question! CheeseCameraMonitor does that when we call
> cheese_camera_device_monitor_coldplug() (which we do from
> EmpathyCameraMonitor's _constructed()). The problem is that
> EmpathyCameraMonitor is a singleton, so if somebody else already created an
> EmpathyCameraMonitor and then you create one, you're actually getting a ref to
> an existing EmpathyCameraMonitor, and so
> cheese_camera_device_monitor_coldplug() isn't called and you don't get the
> ::added signals for existing cameras. That's why I'm calling
> empathy_camera_monitor_get_cameras() when EmpathyCameraMenu is constructed, but
> we shouldn't need that, and it's currently error-prone (if we removed the
> camera-monitor from empathy-call-window, we would get the ::added signals and
> we would add the cameras twice).
> 
> I'm not sure how to best handle this. Perhaps by adding an
> empathy_camera_monitor_new() which doesn't return the singleton but always a
> new object, and keeping empathy_camera_monitor_dup_singleton() for those cases
> where we don't care about the signals?

This sounds fair, yeah.

> > + GQueue cameras;
> > 
> > You don't free the GList in here.
> 
> Fixed. I wonder if I should dynamically allocate the GQueue, free it on
> _dispose and set it to NULL, and on ::added, check if it's != NULL before
> adding EmpathyCameras to it? So that we don't add cameras between _dispose()
> and _finalize().

Sounds like a good idea.

> > + <_description>Default camera device to use in video calls, e.g.
> > /dev/video0.</_description>
> > 
> > Wooaaahh, doesn't this mean that the default camera depends on the order in
> > which the cameras are added?!
> 
> If you unplug and plug your cameras again in a different order, yes. I don't
> expect people from doing that all the time, and even if they do, at the moment
> they unplug the selected camera I think it's right for us to make another
> camera the preferred one. Anyway if you have a better idea on how to store this
> please speak up :)

As we discussed on IRC, we should use some kind of unique identifier for the camera. If you open another bug once this gets merged to stop storing device names in gsettings I'll let this go for now. :-P

> > + /* Do as if the gsettings key had changed, so we select the key that
> > +  * was last set. */
> > + empathy_camera_menu_prefs_camera_changed_cb (self->priv->settings,
> > +     EMPATHY_PREFS_CALL_CAMERA_DEVICE, self);
> > 
> > I don't really like this. What if your preferred camera isn't connected? Your
> > call will just stop as you're setting device=/dev/video101 but there is no
> > fallback when it fails? You should at least check to see whether this device
> > exists.
> 
> If it isn't connected, it won't be in the menu and so we won't activate it. In
> that case we should probably activate another camera (e.g. the first one).
> Agreed?

OK, agreed.

> > Also, what happens visually when you change the video device?
> 
> If you are sending video and have the video preview shown, that'll change to
> show the new device's capture.

Haha yes of course, but I was talking about visual things you don't expect before you get the new video; does the old video freeze or does it go black or what?

+GType
+empathy_camera_get_type (void)
+{
+ static GType type_id = 0;
+
+ if (!type_id)
+ {
+ type_id = g_boxed_type_register_static ("EmpathyCamera",
+ (GBoxedCopyFunc) empathy_camera_copy,
+ (GBoxedFreeFunc) empathy_camera_free);
+ }
+
+ return type_id;
+}

(silly indentation loss)

no G_DEFINE_BOXED_TYPE?

+gchar *
+empathy_video_src_get_device (EmpathyGstVideoSrc *self)

I'd prefer this was called dup_device to highlight the fact it's returning a copied string.

The rest seems fair enough.

(In reply to comment #7)
> (In reply to comment #6)
> > > Does the CheeseCameraMonitor signal camera-added for each camera when you
> > > instantiate it? As in, if you plug in a camera, then create a
> > > CheeseCameraMonitor, will it signal for that camera?
> > 
> > Good question! CheeseCameraMonitor does that when we call
> > cheese_camera_device_monitor_coldplug() (which we do from
> > EmpathyCameraMonitor's _constructed()). The problem is that
> > EmpathyCameraMonitor is a singleton, so if somebody else already created an
> > EmpathyCameraMonitor and then you create one, you're actually getting a ref to
> > an existing EmpathyCameraMonitor, and so
> > cheese_camera_device_monitor_coldplug() isn't called and you don't get the
> > ::added signals for existing cameras. That's why I'm calling
> > empathy_camera_monitor_get_cameras() when EmpathyCameraMenu is constructed, but
> > we shouldn't need that, and it's currently error-prone (if we removed the
> > camera-monitor from empathy-call-window, we would get the ::added signals and
> > we would add the cameras twice).
> > 
> > I'm not sure how to best handle this. Perhaps by adding an
> > empathy_camera_monitor_new() which doesn't return the singleton but always a
> > new object, and keeping empathy_camera_monitor_dup_singleton() for those cases
> > where we don't care about the signals?
> 
> This sounds fair, yeah.

Done (see new commits).

> 
> > > + GQueue cameras;
> > > 
> > > You don't free the GList in here.
> > 
> > Fixed. I wonder if I should dynamically allocate the GQueue, free it on
> > _dispose and set it to NULL, and on ::added, check if it's != NULL before
> > adding EmpathyCameras to it? So that we don't add cameras between _dispose()
> > and _finalize().
> 
> Sounds like a good idea.

Done (new commits)

> > > + <_description>Default camera device to use in video calls, e.g.
> > > /dev/video0.</_description>
> > > 
> > > Wooaaahh, doesn't this mean that the default camera depends on the order in
> > > which the cameras are added?!
> > 
> > If you unplug and plug your cameras again in a different order, yes. I don't
> > expect people from doing that all the time, and even if they do, at the moment
> > they unplug the selected camera I think it's right for us to make another
> > camera the preferred one. Anyway if you have a better idea on how to store this
> > please speak up :)
> 
> As we discussed on IRC, we should use some kind of unique identifier for the
> camera. If you open another bug once this gets merged to stop storing device
> names in gsettings I'll let this go for now. :-P

Let's do that later.

> > > + /* Do as if the gsettings key had changed, so we select the key that
> > > +  * was last set. */
> > > + empathy_camera_menu_prefs_camera_changed_cb (self->priv->settings,
> > > +     EMPATHY_PREFS_CALL_CAMERA_DEVICE, self);
> > > 
> > > I don't really like this. What if your preferred camera isn't connected? Your
> > > call will just stop as you're setting device=/dev/video101 but there is no
> > > fallback when it fails? You should at least check to see whether this device
> > > exists.
> > 
> > If it isn't connected, it won't be in the menu and so we won't activate it. In
> > that case we should probably activate another camera (e.g. the first one).
> > Agreed?
> 
> OK, agreed.

Done.

> > > Also, what happens visually when you change the video device?
> > 
> > If you are sending video and have the video preview shown, that'll change to
> > show the new device's capture.
> 
> Haha yes of course, but I was talking about visual things you don't expect
> before you get the new video; does the old video freeze or does it go black or
> what?

It should show the old video frozen, then the new video. But it's not working all the time here. I'll ask Sjoerd to have a look at it as it might be some Gst related issues.


(In reply to comment #8)
> +GType
> +empathy_camera_get_type (void)
> +{
> + static GType type_id = 0;
> +
> + if (!type_id)
> + {
> + type_id = g_boxed_type_register_static ("EmpathyCamera",
> + (GBoxedCopyFunc) empathy_camera_copy,
> + (GBoxedFreeFunc) empathy_camera_free);
> + }
> +
> + return type_id;
> +}
> 
> (silly indentation loss)
> 
> no G_DEFINE_BOXED_TYPE?

Ah nice. Amended.

> 
> +gchar *
> +empathy_video_src_get_device (EmpathyGstVideoSrc *self)
> 
> I'd prefer this was called dup_device to highlight the fact it's returning a
> copied string.

Amended.

OK to go?

This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.