GNOME Bugzilla – Bug 751185
Use clutter-gst-3.0
Last modified: 2016-12-21 16:56:34 UTC
empathy-call uses clutter-gst-2.0. clutter-gst-3.0 now exists so will need to be migrated to at some point.
See https://bugzilla.gnome.org/show_bug.cgi?id=743496 for similar porting for cheese. https://git.gnome.org/browse/totem/commit/?id=361f76ce9636d3123273b912398b77d29715cf94 is the commit for totem.
I'm taking some liberties and adding Lionel Landwerlin to cc. Is this something you could help out with? - You seem to have a very good grasp of the code :-)
We need cheese to land the clutter-gst 3.0 changes first.
Ah my mistake, it's done already. Just having an old checkout locally.
Created attachment 316527 [details] [review] call: port to clutter-gst 3.0
Unfortunately I have nobody to test that feature with, so this patch might break everything... Anybody feeling adventurous?
Hi and thanks a lot for the patch, but I can't get it to build. Did it build for you? [ 457s] empathy_call-empathy-call-window.o: In function `create_video_output_widget': [ 457s] empathy-call-window.c:(.text+0x3f51): undefined reference to `clutter_bind_constraint' [ 457s] empathy-call-window.c:(.text+0x3f5e): undefined reference to `clutter_constraint_add' [ 457s] collect2: error: ld returned 1 exit status [ 457s] Makefile:884: recipe for target 'empathy-call' failed [ 457s] make[3]: *** [empathy-call] Error 1 [ 457s] make[3]: Leaving directory '/home/abuild/rpmbuild/BUILD/empathy-3.12.11/src' [ 457s] Makefile:724: recipe for target 'all' failed [ 457s] make[2]: *** [all] Error 2 [ 457s] make[2]: Leaving directory '/home/abuild/rpmbuild/BUILD/empathy-3.12.11/src' [ 457s] Makefile:510: recipe for target 'all-recursive' failed [ 457s] make[1]: *** [all-recursive] Error 1 [ 457s] make[1]: Leaving directory '/home/abuild/rpmbuild/BUILD/empathy-3.12.11' [ 457s] Makefile:442: recipe for target 'all' failed [ 457s] make: *** [all] Error 2 [ 457s] error: Bad exit status from /var/tmp/rpm-tmp.x0kLiY (%build)
Same build error here. I also tried --without-cheese.
Created attachment 316559 [details] [review] call: port to clutter-gst 3.0
My mistake, I'm not sure why I didn't get a jhbuild error while building :(
It compiles fine now but telepathy/empathy can't seem to be able to call google hangouts contacts (it never worked in the past), so I cannot test.
dropping this bug. not gonna happen
Created attachment 329347 [details] Appears patch causes segfault It looks like Debian Empathy 3.12.12-2 has this patch applied and it doesn't work for me. Attachment is bt full (current thread only) log of empathy-call segfaulting. I get several console log messages (below), though I think they're showing at least 3 different problems. (Old theme, looking for a gstreamer 0.10 module, and problems with Clutter port) My test is to call empathy configured using telepath-rakia from an Android phone running ring, both connected to a reSIProcate SIP server. I'll see if I can figure out whats not right. Diane --- (empathy:1476): Gtk-WARNING **: Theme parsing error: empathy.css:2:30: The style property GtkButton:default-border is deprecated and shouldn't be used anymore. It will be removed in a future version (empathy:1476): Gtk-WARNING **: Theme parsing error: empathy.css:3:38: The style property GtkButton:default-outside-border is deprecated and shouldn't be used anymore. It will be removed in a future version (empathy:1476): Gtk-WARNING **: Theme parsing error: empathy.css:4:27: The style property GtkButton:inner-border is deprecated and shouldn't be used anymore. It will be removed in a future version (empathy:1476): Gtk-WARNING **: Theme parsing error: empathy.css:5:32: The style property GtkWidget:focus-line-width is deprecated and shouldn't be used anymore. It will be removed in a future version (empathy:1476): Gtk-WARNING **: Theme parsing error: empathy.css:6:29: The style property GtkWidget:focus-padding is deprecated and shouldn't be used anymore. It will be removed in a future version empathy-Message: Element factory "postproc_tmpnoise" not found. empathy-Message: Failed to add "postproc_tmpnoise" (gst-ffmpeg missing?) (empathy:1476): GLib-GObject-CRITICAL **: g_object_new_valist: invalid object type 'ClutterBox' for value type 'ClutterGstVideoSink' [New Thread 0x7fffd1314700 (LWP 1500)] [New Thread 0x7fffae14b700 (LWP 1501)] x264 [error]: baseline profile doesn't support 4:4:4 [New Thread 0x7fffa5e68700 (LWP 1506)] [New Thread 0x7fffa5667700 (LWP 1510)] [New Thread 0x7fffa4e66700 (LWP 1511)] [New Thread 0x7fff87ffb700 (LWP 1513)] [New Thread 0x7fff873b1700 (LWP 1514)] [New Thread 0x7fff8699d700 (LWP 1515)] [New Thread 0x7fff8619c700 (LWP 1516)] [New Thread 0x7fff8599b700 (LWP 1517)] [New Thread 0x7fff8519a700 (LWP 1520)] [New Thread 0x7fff84999700 (LWP 1521)] Program received signal SIGSEGV, Segmentation fault.
The problem I encountered is unrelated to this patch. The segfault is because the Wayland guard condition is preventing XInitThreads from being called in empathy-call's main. Removing the guard and forcing XInitThreads to be called I was able to get a call window to load and start previewing video. Unfortunately I couldn't see the other contact, but then the telepathy/farstream stack is sufficiently buggy (and I don't know how functional ring is) its hard to know what the cause of that issue is. So... this patch should probably should be shipped.
(In reply to Diane Trout from comment #14) > The problem I encountered is unrelated to this patch. > > The segfault is because the Wayland guard condition is preventing > XInitThreads from being called in empathy-call's main. See https://bugzilla.gnome.org/show_bug.cgi?id=767516 for that bug
(In reply to Diane Trout from comment #14) > So... this patch should probably should be shipped. OK. Nobody is maintaining Empathy nowadays, so I'll trust you on this. We want to migrate to clutter-gst-3.0 anyway.
Thank you, I'll keep slowly plodding away at issues that I find. (Though right now what little time I have is focused on gabble)
Harmless warnings: empathy-call-window.c:395:27: warning: assignment from incompatible pointer type [-Wincompatible-pointer-types] priv->video_output_sink = clutter_gst_video_sink_new (); ^ empathy-call-window.c: In function ‘create_video_preview’: empathy-call-window.c:1079:28: warning: assignment from incompatible pointer type [-Wincompatible-pointer-types] priv->video_preview_sink = clutter_gst_video_sink_new (); ^ empathy-call-window.c:1065:34: warning: unused variable ‘sublayout’ [-Wunused-variable] ClutterLayoutManager *layout, *sublayout; ^~~~~~~~~ CCLD empathy-call Will fix these when committing.
Attachment 316559 [details] pushed as f781ba2 - call: port to clutter-gst 3.0
Since this commit, we lost the video preview. Mainly because of this part of the patch, but not only: + clutter_actor_set_content (priv->video_preview, + g_object_new (CLUTTER_GST_TYPE_ASPECTRATIO, + "sink", priv->video_preview, <=== should be video_preview_sink + NULL)); I see another difference with the main video output actor: the "preview" ClutterActor is a special empathy object, an EmpathyRoundedTexture object, derived from a ClutterTexture object. It can be replaced by a regular ClutterActor object, but we loose the rounded borders in this case.
Hi, if you have a git-formatted patch I'm happy to commit it. Nobody is currently working on Empathy unfortunately.
Sure, I'll be happy to contribute. I tried to get the Cogl pipeline from the ClutterGstVideoSink, and to *tweak* it with the cogl primitives added in empathy_rounded_texture_paint(), without much success yet. The initial author of this part of code certainly borrowed the idea of a non rectangular texture from there : https://developer.gnome.org/clutter-cookbook/stable/actors-non-rectangular.html Could cogl/clutter experts confirm if this is the right direction to fix that ? I didn't find examples of these kind of modifications of the video sink element.
Created attachment 340129 [details] Screenshot of the rounded video preview window, with clutter-gst-2.0
It seems you could get this to work with minimal work : 1. Derive EmpathyRoundedTexture from ClutterActor instead of ClutterTexture 2. Create a ClutterGstContent with a sink property set to the video sink used in the video pipeline 3. Set the ClutterGstContent created in 2. as the content property of the EmpathyRoundedTexture
Thanks Lionel for the advice. I finally succeeded to make it work, with an EmpathyRoundedTexture derived _not_ from a ClutterActor, but from a ClutterEffect instead. For some obscure reason, the video sink aspect was totally insensitive by the clipping operations in the paint() method, when deriving from a ClutterActor. I also noticed that cogl_rectanle_with_texture_coords() was not working as expected too, because there was no texture available. This function with clutter-gst-2.0 was just needed to redraw the texture horizontally flipped. I replaced it with cogl matrix transforms. And that made me decrease the min version of cogl deprecation warnings. I also can rename the sub-class from *empathy_rounded_texture* to *empathy_rounded_effect* for example, to better reflect the parent class if needed.
Created attachment 340235 [details] [review] call: fix the video preview
Right, I might have missed something about the Cogl path API then. I think what you're doing in this patch is to render the video offscreen and then rerendering that onscreen with the cogl clipping. It should be possible to do it with a single pass, but that might be a bit more complicated (requires writing a couple of like of shader).
Review of attachment 340235 [details] [review]: ::: src/empathy-call-window.c @@ +1076,3 @@ clutter_actor_set_size (preview, SELF_VIDEO_SECTION_WIDTH, SELF_VIDEO_SECTION_HEIGHT); + clutter_actor_add_effect (preview, empathy_rounded_texture_new ()); I think you leak the EmpathyRoundedTexture, right? The documentation of clutter_actor_add_effect indicates it holds a reference.
Created attachment 340375 [details] [review] call: fix the video preview This is version 2 of the patch : * I renamed empathy-rounded-texture.[hc] to empathy-rounded-effect.[hc] to better reflect the parent class of the rounded effect object. * I verified that empathy_rounded_effect_new () doesn't leak a reference, because it is derived from GInitiallyUnowned. It is disposed in empathy_call_window_reset_pipeline(), when the priv->video_preview object is destroyed. * But the ClutterGstContent priv->preview_content object needs to be unref()ed. (You may see *leaked* objects in valgrind when closing empathy, because the way empathy works: upon disconnect, it recreates a new pipeline for video preview immediately after removing the previous one. So all these objects are recreated, _except_ if video transmission is shut off during the previous connection.)
Review of attachment 340375 [details] [review]: Thanks for fixing this. It looks OK to me (but I don't know anything :) so I'll commit it soon if Lionel doesn't complain.
Review of attachment 340375 [details] [review]: Looks fine to me :)
Attachment 340375 [details] pushed as 9cbcf1d - call: fix the video preview
Created attachment 342286 [details] [review] rounded-effect: make the rounded rectangle coarser While profiling empathy-call, I noticed that a very significant amount of time was spent in the cogl-path library : CPU: Intel Ivy Bridge microarchitecture, speed 3200 MHz (estimated) Counted CPU_CLK_UNHALTED events (Clock cycles when not halted) with a unit mask of 0x00 (No unit mask) count 100000 samples % image name symbol name 93283 7.6121 libc-2.23.so __memcpy_sse2_unaligned 78907 6.4390 libc-2.23.so malloc 60264 4.9177 libc-2.23.so _int_malloc 59493 4.8547 libcogl-path.so.20.4.1 __gl_meshCheckMesh 52060 4.2482 libc-2.23.so _int_free 29738 2.4267 libvpx.so.3.0.0 vpx_get16x16var_sse2 26180 2.1363 orcexec.yKVs6p (deleted) /run/user/1000/orcexec.yKVs6p (deleted) 24993 2.0395 libcogl-path.so.20.4.1 __gl_pqSortInit 23873 1.9481 libvpx.so.3.0.0 vp8_fast_quantize_b_ssse3 19998 1.6319 libvpx.so.3.0.0 vp8_mbloop_filter_vertical_edge_sse2 18884 1.5410 libopus.so.0.5.2 /usr/lib64/libopus.so.0.5.2 15614 1.2741 libc-2.23.so malloc_consolidate 13470 1.0992 libcogl-path.so.20.4.1 WalkDirtyRegions 13427 1.0957 libcogl-path.so.20.4.1 AddRightEdges 11947 0.9749 libm-2.23.so sincosf 10498 0.8567 libcogl-path.so.20.4.1 __gl_computeInterior 10478 0.8550 libcogl-path.so.20.4.1 __gl_edgeSign 10052 0.8203 libvpx.so.3.0.0 vp8_pack_tokens [...] The fine grained value of the arc_step parameter of cogl_path_round_rectangle() is the culprit for that. According to the typical size of the video preview (120x90), the radius of the rounded borders (90/16 ~= 5 pixels), it is safe to provide a much higher angle step (in degrees), without any visual effect. This patch does that.
Review of attachment 342286 [details] [review]: ::: src/empathy-rounded-effect.c @@ +43,2 @@ /* create and store a path describing a rounded rectangle */ + cogl_path_round_rectangle (0, 0, width, height, height / 16., 15); I'd leave a warning comment here, pointing to your result.
Created attachment 342315 [details] [review] rounded-effect: make the rounded rectangle coarser (v2) Patch with added comment.