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 751185 - Use clutter-gst-3.0
Use clutter-gst-3.0
Status: RESOLVED FIXED
Product: empathy
Classification: Core
Component: General
unspecified
Other Linux
: Normal normal
: ---
Assigned To: empathy-maint
empathy-maint
Depends on: 743496
Blocks:
 
 
Reported: 2015-06-18 21:54 UTC by Robert Ancell
Modified: 2016-12-21 16:56 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
call: port to clutter-gst 3.0 (3.75 KB, patch)
2015-11-30 15:20 UTC, Lionel Landwerlin
none Details | Review
call: port to clutter-gst 3.0 (3.80 KB, patch)
2015-11-30 21:54 UTC, Lionel Landwerlin
committed Details | Review
Appears patch causes segfault (44.75 KB, text/plain)
2016-06-07 22:35 UTC, Diane Trout
  Details
Screenshot of the rounded video preview window, with clutter-gst-2.0 (20.22 KB, image/png)
2016-11-17 11:16 UTC, Fabrice Bellet
  Details
call: fix the video preview (5.70 KB, patch)
2016-11-18 14:33 UTC, Fabrice Bellet
none Details | Review
call: fix the video preview (10.58 KB, patch)
2016-11-20 20:49 UTC, Fabrice Bellet
committed Details | Review
rounded-effect: make the rounded rectangle coarser (1.12 KB, patch)
2016-12-20 20:19 UTC, Fabrice Bellet
none Details | Review
rounded-effect: make the rounded rectangle coarser (v2) (1.48 KB, patch)
2016-12-21 12:37 UTC, Fabrice Bellet
committed Details | Review

Description Robert Ancell 2015-06-18 21:54:19 UTC
empathy-call uses clutter-gst-2.0. clutter-gst-3.0 now exists so will need to be migrated to at some point.
Comment 1 Robert Ancell 2015-06-18 21:56:57 UTC
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.
Comment 2 Bjørn Lie 2015-11-29 14:29:34 UTC
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 :-)
Comment 3 Lionel Landwerlin 2015-11-30 15:09:37 UTC
We need cheese to land the clutter-gst 3.0 changes first.
Comment 4 Lionel Landwerlin 2015-11-30 15:13:31 UTC
Ah my mistake, it's done already. Just having an old checkout locally.
Comment 5 Lionel Landwerlin 2015-11-30 15:20:47 UTC
Created attachment 316527 [details] [review]
call: port to clutter-gst 3.0
Comment 6 Lionel Landwerlin 2015-11-30 15:21:48 UTC
Unfortunately I have nobody to test that feature with, so this patch might break everything... Anybody feeling adventurous?
Comment 7 Bjørn Lie 2015-11-30 20:58:18 UTC
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)
Comment 8 Hussam Al-Tayeb 2015-11-30 21:22:09 UTC
Same build error here. I also tried --without-cheese.
Comment 9 Lionel Landwerlin 2015-11-30 21:54:08 UTC
Created attachment 316559 [details] [review]
call: port to clutter-gst 3.0
Comment 10 Lionel Landwerlin 2015-11-30 21:54:59 UTC
My mistake, I'm not sure why I didn't get a jhbuild error while building :(
Comment 11 Hussam Al-Tayeb 2015-11-30 22:55:25 UTC
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.
Comment 12 Matthias Clasen 2016-03-01 22:53:19 UTC
dropping this bug. not gonna happen
Comment 13 Diane Trout 2016-06-07 22:35:47 UTC
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.
Comment 14 Diane Trout 2016-06-10 18:32:18 UTC
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.
Comment 15 Diane Trout 2016-06-24 05:38:10 UTC
(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
Comment 16 Michael Catanzaro 2016-07-21 22:42:26 UTC
(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.
Comment 17 Diane Trout 2016-07-21 22:46:57 UTC
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)
Comment 18 Michael Catanzaro 2016-07-22 01:38:18 UTC
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.
Comment 19 Michael Catanzaro 2016-07-22 01:46:27 UTC
Attachment 316559 [details] pushed as f781ba2 - call: port to clutter-gst 3.0
Comment 20 Fabrice Bellet 2016-11-15 15:55:26 UTC
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.
Comment 21 Michael Catanzaro 2016-11-15 17:00:17 UTC
Hi, if you have a git-formatted patch I'm happy to commit it. Nobody is currently working on Empathy unfortunately.
Comment 22 Fabrice Bellet 2016-11-17 11:14:47 UTC
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.
Comment 23 Fabrice Bellet 2016-11-17 11:16:23 UTC
Created attachment 340129 [details]
Screenshot of the rounded video preview window, with clutter-gst-2.0
Comment 24 Lionel Landwerlin 2016-11-17 11:38:52 UTC
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
Comment 25 Fabrice Bellet 2016-11-18 14:32:16 UTC
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.
Comment 26 Fabrice Bellet 2016-11-18 14:33:02 UTC
Created attachment 340235 [details] [review]
call: fix the video preview
Comment 27 Lionel Landwerlin 2016-11-18 14:36:24 UTC
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).
Comment 28 Michael Catanzaro 2016-11-18 16:28:29 UTC
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.
Comment 29 Fabrice Bellet 2016-11-20 20:49:25 UTC
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.)
Comment 30 Michael Catanzaro 2016-11-21 00:54:16 UTC
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.
Comment 31 Lionel Landwerlin 2016-11-21 10:59:16 UTC
Review of attachment 340375 [details] [review]:

Looks fine to me :)
Comment 32 Michael Catanzaro 2016-11-21 14:25:46 UTC
Attachment 340375 [details] pushed as 9cbcf1d - call: fix the video preview
Comment 33 Fabrice Bellet 2016-12-20 20:19:02 UTC
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.
Comment 34 Michael Catanzaro 2016-12-20 22:16:46 UTC
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.
Comment 35 Fabrice Bellet 2016-12-21 12:37:11 UTC
Created attachment 342315 [details] [review]
rounded-effect: make the rounded rectangle coarser (v2)

Patch with added comment.