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 653449 - [GSOC] empathy-video-widget: switch to GtkClutterEmbed
[GSOC] empathy-video-widget: switch to GtkClutterEmbed
Status: RESOLVED DUPLICATE of bug 582442
Product: empathy
Classification: Core
Component: VoIP
unspecified
Other All
: Normal enhancement
: ---
Assigned To: empathy-maint
Depends on:
Blocks:
 
 
Reported: 2011-06-26 20:57 UTC by Raluca-Elena Podiuc
Modified: 2011-07-12 15:08 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
empathy-video-widget: reimplemented with GtkClutterEmbed (19.97 KB, patch)
2011-06-26 20:58 UTC, Raluca-Elena Podiuc
none Details | Review
empathy-video-widget: remove gst-bin property and constructor arg (7.65 KB, patch)
2011-06-26 20:59 UTC, Raluca-Elena Podiuc
none Details | Review
empathy-video-widget: reimplemented with GtkClutterEmbed (20.52 KB, patch)
2011-06-29 01:24 UTC, Raluca-Elena Podiuc
none Details | Review
empathy-video-widget: videoscale not needed (scaling done in clutter) (2.11 KB, patch)
2011-06-29 01:26 UTC, Raluca-Elena Podiuc
none Details | Review

Description Raluca-Elena Podiuc 2011-06-26 20:57:49 UTC
These two patches greatly simplify the implementation of empathy-video-widget by switching from GtkDrawingArea to GtkClutterEmbed.

This is the first step in getting Empathy to use the video-source from Cheese.

More details in the patches themselves.
Comment 1 Raluca-Elena Podiuc 2011-06-26 20:58:43 UTC
Created attachment 190714 [details] [review]
empathy-video-widget: reimplemented with GtkClutterEmbed
Comment 2 Raluca-Elena Podiuc 2011-06-26 20:59:16 UTC
Created attachment 190715 [details] [review]
empathy-video-widget: remove gst-bin property and constructor arg
Comment 3 Guillaume Desmottes 2011-06-27 07:12:26 UTC
Please please please, check with Emilio first (pochu on IRC) who is doing some  Clutter gst work as well.
Comment 4 Emilio Pozuelo Monfort 2011-06-27 10:19:31 UTC
When I saw you on #clutter I thought you were working on some cheese integration, not on this :/

Your approach looks good, except for two things:

- It makes Empathy hard-depend on clutter, and Guillaume wanted to avoid that for the time being AFAIK (though that may not be the case anymore).
- I was working on bug #582442, which is about displaying the video preview on top of the remote video. To do that with your approach, we'd have to embed the two EmpathyVideoWidgets in GtkClutterActors (after we've put the video textures in GtkClutterEmbeds!), and then layout them in a ClutterBox as I was doing (except that I created the video textures directly in empathy-call-window and so I don't need to embed them in GtkClutterEmbeds).

An option is to follow your approach but make EmpathyVideoWidget a ClutterTexture instead of a GtkWidget. I'm not sure if that's a good idea.

A few more comments:
- You don't call gtk_clutter_init()
- You don't check for clutter-gst in configure.ac
- I don't think you need videoscale anymore, clutter will scale the video for you.
- Is the aspect ratio kept? I see you call clutter_texture_set_keep_aspect_ratio(), but that didn't work for me, that's why I wonder.

+  priv->sink = clutter_gst_video_sink_new (CLUTTER_TEXTURE (texture));
+  g_object_unref (G_OBJECT (texture));
+  g_assert (priv->sink != NULL);

+  /* keep a reference so we can set it's "sync" or "async" properties */
+  gst_object_ref (priv->sink);

Don't you already have a ref from clutter_gst_video_sink_new() ?
Comment 5 Emilio Pozuelo Monfort 2011-06-27 10:22:27 UTC
(In reply to comment #0)
> This is the first step in getting Empathy to use the video-source from Cheese.

What does this mean? Use video-source from cheese where, and why?
Comment 6 Raluca-Elena Podiuc 2011-06-27 10:57:58 UTC
(In reply to comment #4)
> - It makes Empathy hard-depend on clutter, and Guillaume wanted to avoid that
> for the time being AFAIK (though that may not be the case anymore).

From what I read in https://bugzilla.gnome.org/show_bug.cgi?id=582442#c12 Guillaume it won't be a problem.

    "Plan is now to implement this using Clutter."


> - I was working on bug #582442, which is about displaying the video preview on
> top of the remote video. To do that with your approach, we'd have to embed the
> two EmpathyVideoWidgets in GtkClutterActors (after we've put the video textures
> in GtkClutterEmbeds!), and then layout them in a ClutterBox as I was doing
> (except that I created the video textures directly in empathy-call-window and
> so I don't need to embed them in GtkClutterEmbeds).
> 
> An option is to follow your approach but make EmpathyVideoWidget a
> ClutterTexture instead of a GtkWidget. I'm not sure if that's a good idea.


I'm not sure how hard an approach using my code will be, do you have a working git tree somewhere?


> A few more comments:
> - You don't call gtk_clutter_init()
> - You don't check for clutter-gst in configure.ac


Will post fix.


> - I don't think you need videoscale anymore, clutter will scale the video for
> you.

I didn't check the GstBin elements, I'll add a third cleanup patch.


> - Is the aspect ratio kept? I see you call
> clutter_texture_set_keep_aspect_ratio(), but that didn't work for me, that's
> why I wonder.


Yep. I used the box and layout manager to do this.

> +  priv->sink = clutter_gst_video_sink_new (CLUTTER_TEXTURE (texture));
> +  g_object_unref (G_OBJECT (texture));
> +  g_assert (priv->sink != NULL);
> 
> +  /* keep a reference so we can set it's "sync" or "async" properties */
> +  gst_object_ref (priv->sink);
> 
> Don't you already have a ref from clutter_gst_video_sink_new() ?


I'm putting this sink in the GstBin. I thought it will take the floating reference for itself (I saw this is what happened in clutter). I wanted to make sure I have my own. I'll check.
Comment 7 Raluca-Elena Podiuc 2011-06-27 11:08:15 UTC
(In reply to comment #5)
> (In reply to comment #0)
> > This is the first step in getting Empathy to use the video-source from Cheese.
> 
> What does this mean? Use video-source from cheese where, and why?


In the current implementation we use "gconfvideosrc" to create the video source. This will work if "gconfvideosrc" is correctly set and available (not used by other applications).

If I have two cameras on my system depending on the order of registration with the kernel they will get either /dev/video0 or /dev/video1. "gconfvideosrc" has a value that was valid when I last ran gstreamer-properties, but may not be valid now. I had crashes from empathy-av or empathy-call (I don't remember which) because they were trying to open a camera that was already in use or used "gconfvideosrc" but that was missconfigured.


Cheese monitors the registration/unregistration of video cameras and will use one that is available. This is a more dynamic approach.

Also the cheese-camera source has some gstelements that make sure you can change balance for contrast/gamma/etc. Not all camera sources support these and that's why in empathy-video-src we have to check for this support. With cheese-camera we know these will work and won't have to code it in Empathy.


A last point is that we will be able to use Cheese effects very easily in Empathy by using libcheese. But to integrate with that we need cheese-camera.
Comment 8 Raluca-Elena Podiuc 2011-06-29 01:24:16 UTC
Created attachment 190899 [details] [review]
empathy-video-widget: reimplemented with GtkClutterEmbed

addressed issues raised by Emilio:
- call gtk_clutter_init()
- check for clutter-gst in configure.ac
Comment 9 Raluca-Elena Podiuc 2011-06-29 01:26:04 UTC
Created attachment 190900 [details] [review]
empathy-video-widget: videoscale not needed (scaling done in clutter)

Videoscale not needed, and soon (after getting video from cheese-camera) neither will gst-bin or colorspace.
Comment 10 Guillaume Desmottes 2011-07-04 12:40:25 UTC
I'd really prefer to not touch empathy-av. empathy-call is the future and I prefer to keep empathy-av as it so it can still be used as a fallback and we don't introduce a hard dep on clutter.

Just keep empathy-video-widget.c unchanged and create a new src/empathy-video-clutter-widget.c or something.
Comment 11 Guillaume Desmottes 2011-07-04 13:27:08 UTC
I hit this error when terminating a video call:


Clutter-CRITICAL **: clutter_actor_unmap: assertion `CLUTTER_IS_ACTOR (self)' failed

Program received signal SIGTRAP, Trace/breakpoint trap.
0x00007fffecfc3370 in g_logv (log_domain=0x7ffff3510ba6 "Clutter", log_level=G_LOG_LEVEL_CRITICAL, 
    format=0x7fffed04ac18 "%s: assertion `%s' failed", args1=0x7fffffffb0f0) at gmessages.c:552
552			G_BREAKPOINT ();
(gdb) bt
  • #0 g_logv
    at gmessages.c line 552
  • #1 g_log
    at gmessages.c line 573
  • #2 g_return_if_fail_warning
  • #3 clutter_actor_unmap
    at ./clutter-actor.c line 1206
  • #4 gtk_clutter_embed_unmap
    at ./gtk-clutter-embed.c line 372
  • #5 g_cclosure_marshal_VOID__VOID
    at gmarshal.c line 85
  • #6 g_type_class_meta_marshal
    at gclosure.c line 884
  • #7 g_closure_invoke
    at gclosure.c line 773
  • #8 signal_emit_unlocked_R
    at gsignal.c line 3186
  • #9 g_signal_emit_valist
    at gsignal.c line 2987
  • #10 g_signal_emit
    at gsignal.c line 3044
  • #11 gtk_widget_unmap
    at gtkwidget.c line 4186
  • #12 gtk_widget_unrealize
    at gtkwidget.c line 4432
  • #13 gtk_widget_unparent
    at gtkwidget.c line 3734
  • #14 gtk_box_remove
    at gtkbox.c line 1796
  • #15 g_cclosure_marshal_VOID__OBJECT
    at gmarshal.c line 644
  • #16 g_type_class_meta_marshal
    at gclosure.c line 884
  • #17 g_closure_invoke
    at gclosure.c line 773
  • #18 signal_emit_unlocked_R
    at gsignal.c line 3186
  • #19 g_signal_emit_valist
    at gsignal.c line 2987
  • #20 g_signal_emit
    at gsignal.c line 3044
  • #21 gtk_container_remove
    at gtkcontainer.c line 1535
  • #22 gtk_widget_dispose
    at gtkwidget.c line 10586
  • #23 gtk_clutter_embed_dispose
    at ./gtk-clutter-embed.c line 130
  • #24 empathy_video_widget_dispose
    at empathy-video-widget.c line 269
  • #25 g_object_run_dispose
    at gobject.c line 945
  • #26 gtk_widget_destroy
    at gtkwidget.c line 3816
  • #27 empathy_streamed_media_window_reset_pipeline
    at empathy-streamed-media-window.c line 1846
  • #28 empathy_streamed_media_window_disconnected
    at empathy-streamed-media-window.c line 1899
  • #29 empathy_streamed_media_window_hangup_cb
    at empathy-streamed-media-window.c line 3044
  • #30 g_cclosure_marshal_VOID__VOID
    at gmarshal.c line 85
  • #31 g_closure_invoke
    at gclosure.c line 773
  • #32 signal_emit_unlocked_R
    at gsignal.c line 3256
  • #33 g_signal_emit_valist
    at gsignal.c line 2987
  • #34 g_signal_emit_by_name
    at gsignal.c line 3081
  • #35 button_clicked
    at gtktoolbutton.c line 814
  • #36 g_cclosure_marshal_VOID__VOID
    at gmarshal.c line 85
  • #37 g_closure_invoke
    at gclosure.c line 773
  • #38 signal_emit_unlocked_R
    at gsignal.c line 3256
  • #39 g_signal_emit_valist
    at gsignal.c line 2987
  • #40 g_signal_emit
    at gsignal.c line 3044
  • #41 gtk_button_clicked
    at gtkbutton.c line 1194
  • #42 gtk_real_button_released
    at gtkbutton.c line 1820
  • #43 g_cclosure_marshal_VOID__VOID
    at gmarshal.c line 85
  • #44 g_type_class_meta_marshal
  • #45 g_closure_invoke
    at gclosure.c line 773
  • #46 signal_emit_unlocked_R
    at gsignal.c line 3186
  • #47 g_signal_emit_valist
    at gsignal.c line 2987
  • #48 g_signal_emit
    at gsignal.c line 3044
  • #49 gtk_button_released
    at gtkbutton.c line 1180
  • #50 gtk_button_button_release
    at gtkbutton.c line 1712
  • #51 _gtk_marshal_BOOLEAN__BOXED
    at gtkmarshalers.c line 85
  • #52 g_type_class_meta_marshal
    at gclosure.c line 884
  • #53 g_closure_invoke
    at gclosure.c line 773
  • #54 signal_emit_unlocked_R
    at gsignal.c line 3294
  • #55 g_signal_emit_valist
    at gsignal.c line 2997
  • #56 g_signal_emit
    at gsignal.c line 3044
  • #57 gtk_widget_event_internal
    at gtkwidget.c line 6105
  • #58 gtk_widget_event
    at gtkwidget.c line 5821
  • #59 gtk_propagate_event
    at gtkmain.c line 2599
  • #60 gtk_main_do_event
    at gtkmain.c line 1874
  • #61 _gdk_event_emit
    at gdkevents.c line 71
  • #62 gdk_event_source_dispatch
    at gdkeventsource.c line 360
  • #63 g_main_dispatch
    at gmain.c line 2500
  • #64 g_main_context_dispatch
    at gmain.c line 3083
  • #65 g_main_context_iterate
    at gmain.c line 3161
  • #66 g_main_loop_run
    at gmain.c line 3369
  • #67 gtk_main
    at gtkmain.c line 1357
  • #68 gtk_application_run_mainloop
    at gtkapplication.c line 112
  • #69 g_application_run
    at gapplication.c line 1325
  • #70 main
    at empathy-av.c line 168

Comment 12 Emilio Pozuelo Monfort 2011-07-04 14:42:18 UTC
(In reply to comment #10)
> I'd really prefer to not touch empathy-av. empathy-call is the future and I
> prefer to keep empathy-av as it so it can still be used as a fallback and we
> don't introduce a hard dep on clutter.
> 
> Just keep empathy-video-widget.c unchanged and create a new
> src/empathy-video-clutter-widget.c or something.

+1. And make it a ClutterTexture subclass, instead of a GtkClutterEmbed one. But then I'm not sure we need a separate object, since there's not much to do other than 

  video_preview_texture = clutter_texture_new ();
  clutter_actor_set_size (video_preview_texture,
      SELF_VIDEO_SECTION_WIDTH, SELF_VIDEO_SECTION_HEIGTH);

  priv->video_preview_sink = clutter_gst_video_sink_new (
      CLUTTER_TEXTURE (video_preview_texture));

(see my branch at http://cgit.collabora.com/git/user/pochu/empathy.git/log/?h=call-preview-582442 )
Comment 13 Emilio Pozuelo Monfort 2011-07-06 12:12:37 UTC
I have posted my branch for review at https://bugzilla.gnome.org/show_bug.cgi?id=582442#c13

I think we can go on with my branch, and create a new object if necessary later.
Comment 14 Emilio Pozuelo Monfort 2011-07-06 14:42:33 UTC
Raluca, can you create the new widget (assuming we still need/want it) without modifying empathy-call-window / empathy-streamed-media-window, and then we can modify only empathy-call-window to use it when I land my branch? I can do the empathy-call-window changes to use your new widget if you want.

Are you fine with that? If so I'll push my branch in bug #582442 when they're good to go (I ask in case that will be a big problem for you because of the conflicts). Or if you already have a branch I can try to port mine to it.
Comment 15 Guillaume Desmottes 2011-07-12 15:08:30 UTC
Closing as a dup of bug #582442 as we agreed that was the right approach.

*** This bug has been marked as a duplicate of bug 582442 ***