GNOME Bugzilla – Bug 653449
[GSOC] empathy-video-widget: switch to GtkClutterEmbed
Last modified: 2011-07-12 15:08:30 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.
Created attachment 190714 [details] [review] empathy-video-widget: reimplemented with GtkClutterEmbed
Created attachment 190715 [details] [review] empathy-video-widget: remove gst-bin property and constructor arg
Please please please, check with Emilio first (pochu on IRC) who is doing some Clutter gst work as well.
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() ?
(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 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.
(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.
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
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.
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.
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
+ Trace 227650
(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 )
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.
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.
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 ***