I think I'd add this to the right side bar.

As said in a mail, this should be optional (using a --enable-video-effect) and disabled by default at first.

We should think how this would better fit in the new UI.

Nick, do you have any idea on how this would best look?

Camera related functions are currently on the i button, so that's a good place to keep camera manipulation, although it does tend to suggest that we need to go a bit further in moving some settings there.

Aside from that, I like the design and I'll whip up a quick page in the overall video chat wireframes to show it.

How many effects do we expect to have?

Cheese has 9, so I'd say something like that for now; perhaps some more as time goes on and gstreamer has more transformation elements. Raluca, please correct me if I'm wrong :)

Nick, you can open cheese and click on the "Effects" button on the bottom to see how it looks there.

Yeah, so my concern is the size of the window in cheese is somewhat different to the size of the window in video chat so we may have to have a more dynamic grid, and possibly scrollbars or somesuch.

I've added design for this to what will be the next iteration of the new ui wireframes. It'll be just like cheese.

Nick, where can I see the updated wireframes?

I have implemented effects in Empathy:

There's a new button to select effect previews. Pressing that button will replace the two video sinks with a grid of 3x3 effect previews.

The floating toolbar is still displayed in the effect preview page.
- if you click on the "camera" button you can disable sending video over the network, but the effect previews are still playing.
- if you click on the "effects" button you go back to the main page without changing the current effect
- if you click on an effect from the grid the currently playing effect is discarded and the selected one is used

There's one problem to be solved: I still need to add Next/Prev buttons to move between pages of effects. I don't think you'll want these added to the floating toolbar because they would use up too much space. I was thinking about adding two big clutter arrows on each side of the preview page.


I broke things up to ease review, but I'm not very sure if I should add 14 patches as attachements.

Code is up on https://github.com/raluca-elena/empathy-cheese, branch cheese-video-effects. I may rebase this a bit while I make sure stuff works without HAVE_CHEESE.

Awesome! Could you please attach screenshot of all of this so it would be easier for Nick to give you input on the UI.

Yeap! You can find the screenshots here:

http://building-a-black-hole.blogspot.com/2011/08/cheesy-effects-attack-empathy.html

I will make an archive also if you want. I already made one but it was to big so I will have to resize the pictures.

Cool, thanks. Does it work fine when you change your input webcam on the fly? You may need to wait that bug #656579 is fixed.

I updated the patches (rebased on master) and they work with on-the-fly camera replacement.

Guillaume/Emilio if you want to test this you'll need libcheese from master (just pushed the necessary commit).

Again, I rewrote/added some patches. Now it compiles/runs well with and without HAVE_CHEESE.

I also added a patch with a 'proper' icon for effects copy-pasted from cheese.

Should I send this icon to gnome-icon-theme too so that both empathy and cheese can discard their own copies in the future?

I didn't manage to test your branch because of bug #657070.

I guess we should depend on the (future) cheese 3.1.6 rather than 3.1.5 as your changes haven't been released yet.

As said in my first comment, I'd prefer to have a --enable-video-effect switch disabled by default and depending on cheese so distros can easily turn off video effects while keeping cheese avatar support.

Thanks for this, it's a cool feature. It'd be best exposed on the info menu of the stream, which is where options that modify the video stream go rather than in the toolbar. 

http://nickr.org/linux/wireframes/empathy-video-chat-wireframes.pdf

video effects is on p8 of the 22nd August 2011 version.

@Guillaume: added --enable-video-effect switch and pushed to my github (rebased)

@Nick: will add the effects to the "i" button.
Regarding the design: you have a scrollbar for when there are two many video effects. The problem with a scrollbar is that we would have too many video effect previews playing at the same time and the interface would be very CPU intensive.

That's why now Cheese has 3x3 grids of previews but at one time only one grid's previews are playing. The rest are paused not using CPU.

In this case the scrollbar itself wouldn't work very much like a normal scrollbar. Only the next/prev icons on it would be useful. I was thinking of having two big transparent arrows '<' and '>' on the sides of the window. How does that look to you?

I rewrote the patches to move the "Video effects" button from the toolbar to the info-menu.

Code is on: https://github.com/raluca-elena/empathy-cheese branch cheese-video-effects-info-menu

Should work with or without video effects enabled. Use --enable-video-effect/--disable-video-effect to select.

I've updated the design (same URL) to use an inline pager for effects rather than a scroll bar.

I see that on the design page the bottom in-video-control toolbar is not shown while the video-effect-previews are displayed.

In the code I wrote I made that toolbar visible on the effect preview (with the normal 3 second fade-out delay). I think the info on the toolbar is useful while looking on effect previews too (is my mike on?, am I sending video while I'm selecting these effects?, was the call disconnected?).

Also you can choose whether to enable/disable sending video on the network while choosing effects.


With this in mind, because the bottom toolbar is floating over the video stream (not separated and to the lower part of the window) the "v" (down) button would look weird.

I don't understand why you chose vertical "^" and "v" instead of lateral "<" and ">". Cheese's buttons are < and >, and so are iChat's/Photobooth's effects: http://b-l-a-c-k-o-p.com/CatEye.html

The toolbar thing is a design mistake and I'll rectify it. Under the normal behaviour of the toolbar it will be visible as the user will have moved their mouse within the last three seconds. It is likely to stay visible throughout the transaction as well since preview selection is an interactive process. I see no reason to override the standard toolbar behaviour in this screen unless there is confusion as to what is selectable in future if this toolbar is truly floating.

As for the position of the arrows, there isn't really a HIG guide or general pattern for 'paging' currently. Personally I believe it should be up and down since that's the way navigation on the rest of the system works but I have no particular investment in the decision and no data to 'prove' anything either way. So yeah, we can stick em on the side. I'll update the stuff in a bit.

Branch doesn't build. I have cheese 3.0.1 installed, yet the configure.ac check passes. That's because of this:

+   PKG_CHECK_MODULES(VIDEO_EFFECT,
+   [
+     gstreamer-0.10
+     cheese-gtk >= LIBCHEESE_GTK_REQUIRED
+   ], have_video_effect="yes", have_video_effect="no" )

That should be $LIBCHEESE_GTK_REQUIRED

Also, you require  cheese 3.1.5 but git only has 3.0.x. I guess you want git master, and we need that to bump the version and make a release.


There's a bug in cheese causing a build failure. I've reported that as bug #657825


+AC_ARG_ENABLE(video-effect,
+            AS_HELP_STRING([--enable-video-effect=@<:@no/yes/auto@:>@],
+            [build with Cheese video effects]),,
+            [enable_video_effect=auto])

That doesn't disable the effects by default. We want them to be disabled unless you pass --enable-video-effect to configure. That's probably fixed by doing enable_video_effect=no instead of auto, but I haven't tested it.


+  #ifdef HAVE_VIDEO_EFFECT
+  /* array of CheeseEffect objects, sorted by name. First element=identity */
+  CheeseEffect **cheese_effects;
+  gint         cheese_effects_count;
+  ClutterActor **effect_pages;
+  gint         effect_pages_count, curr_effect_page;
+  #endif /* HAVE_VIDEO_EFFECT */

#ifdefs shouldn't be wrapped (same in other places)

You need to include config.h (before any other include) in empathy-call-window.c to get HAVE_VIDEO_EFFECT. Also the cheese-effect include should be:

#include <cheese-effect.h>

(i.e. without the "cheese/".

And you want to add $(VIDEO_EFFECT_CFLAGS) to empathy_call_CFLAGS and $(VIDEO_EFFECT_LIBS) to empathy_call_LDFLAGS in src/Makefile.am



 if test x"$with_cheese" != x"no" ; then
-   PKG_CHECK_MODULES(CHEESE, gstreamer-0.10 cheese-gtk >= 2.91.91.1, [have_cheese=yes], [have_cheese=no])
+   PKG_CHECK_MODULES(CHEESE, gstreamer-0.10 cheese-gtk >= LIBCHEESE_GTK_REQUIRED, [have_cheese=yes], [have_cheese=no])

This change is unrelated and unnecessary, so it should be reverted. You check for cheese-gtk >= $LIBCHEESE_GTK_REQUIRED in the video-effects check, not here. (Also it's missing a `$')



+       Cheese video effects  ......:  ${have_video_effect}

The dots should start right after `effects' (without any spaces) like the rest of the summary



+static void
+container_remove_all_children (ClutterContainer *c)
+{
+  GList *chld, *l;
+  chld = clutter_container_get_children (c);
+  for (l = chld; l; l = g_list_next (l))
+    clutter_container_remove_actor (c, CLUTTER_ACTOR (l->data));
+  g_list_free (chld);
+}

There should be a new line after the variable declarations, and another one after the clutter_container_remove_actor() call. The `l' check should be `l != NULL'. Same in other functions (if in doubt, check what the rest of the code does).


+  if (eff_valve)  gst_object_unref (eff_valve);
+  if (eff_csp1)   gst_object_unref (eff_csp1);
+  if (eff_csp2)   gst_object_unref (eff_csp2);

There should be newlines there and the ifs should have != NULL.


I see no arrows to change pages?

You should try to stick to 80 chars width lines


+  /* keep a reference so it won't die when we'll remove it
+   * in later call of container_remove_all_children () */
+  g_object_ref (eff_page);

This looks wrong to me. If you go to page 2, then back to page 1, then back to page 2, we'll have two refs on page 2, no? And so on... So we'll potentially leak the pages? If so, you can take a ref when creating them, and release them on destruction, and don't do it when changing the page.


+  g_object_set (G_OBJECT (eff_page), "opacity", 0, NULL);

clutter_actor_set_opacity ()

The indentation on ifs and fors is wrong. The brackets should be indented two spaces. E.g. instead of


  if (!ok)
  {
    g_warning ("Error linking effect elements. Effect name=%s", eff_name);
    goto err_linking_gst_elems;
  }

It should be

  if (!ok)
    {
      g_warning (...);
      goto ...;
    }

In general, review all your coding style and make it fit the Telepathy/Empathy style.


+static void
+empathy_call_window_video_effects_cb (GtkAction *action,
+  EmpathyCallWindow *self) { }

Put the brackets in new lines.


+  cmp = strcmp (a, b);

You can do g_strcmp0() which is NULL-safe, just in case.


+  priv->effect_pages_count = \
+    (priv->cheese_effects_count + EFFECTS_PER_PAGE - 1) / EFFECTS_PER_PAGE;

No need to use '\' in C.


+    clutter_box_pack (box, CLUTTER_ACTOR (texture), NULL, NULL, NULL);

You can use clutter_container_add_actor () since you don't set any property. Plus you don't need 3 NULLs, there should just be one.


+    clutter_actor_set_opacity ((ClutterActor*) rect, (guint) 128);

That cast to guint shouldn't be necessary.


+    clutter_color_from_string (&rect_color, "black");
+    clutter_rectangle_set_color (rect, &rect_color);

You can just do

clutter_rectangle_set_color (rect, CLUTTER_COLOR_Black);


+    clutter_box_pack (box, CLUTTER_ACTOR (rect),
+                      "x-align", CLUTTER_BIN_ALIGNMENT_FILL,
+                      "y-align", CLUTTER_BIN_ALIGNMENT_END,
+                      NULL, NULL);

There's an extra NULL at the end that shouldn't be there.


+     Only show it if we have access to cheeese effects. */

yum yum, cheeeeese!


+  g_object_set (priv->video_effects_button, "visible", TRUE, NULL);

You can call gtk_widget_show ()


   constraint = clutter_bind_constraint_new (
       gtk_clutter_embed_get_stage (GTK_CLUTTER_EMBED (priv->video_container)),
       CLUTTER_BIND_SIZE, 0);
-  clutter_actor_add_constraint (priv->video_box, constraint);
+  clutter_actor_add_constraint (priv->root_box, constraint);

This change shouldn't be necessary AFAICS.


+  tp_clear_object (&priv->video_preview_tee);
+  //TODO

Either fix that or remove the comment if there's nothing to fix.


+  /* drop frames so that we don't run the filter uselesly */

uselessly


+#else /* HAVE_VIDEO_EFFECT */
+
+static void
+empathy_call_window_init_video_effects (EmpathyCallWindow *self) { }
+
+#endif /* HAVE_VIDEO_EFFECT */

Instead of doing that, you could call empathy_call_window_init_video_effects() conditionally. Same with empathy_call_window_video_effects_cb()


I'm not sure about commit 9fbda9fbcbdc8350be7161d3bc472f2e95dd15be ("empathy-call: don't stop the camera while camera preview disabled"). Sounds like it's working around a bug, but I may be wrong.


 empathy_call_window_remove_video_input (EmpathyCallWindow *self)
 {
...

   gtk_widget_set_sensitive (priv->camera_button, FALSE);
+  gtk_widget_set_sensitive (priv->video_effects_button, FALSE);
 }

You don't need to do that. The preview button won't be shown when the video preview is not there (and thus when the video input is removed.

The branch works for me with the mentioned build fixes. The main issue is the coding style, and I'd like some GStreamer savvy person to look at the pipeline stuff.

I've redone the patches to incorporate the comments from above.

https://github.com/raluca-elena/empathy-cheese cheese-video-effects-v2 (default branch)


In this bug: https://bugzilla.gnome.org/show_bug.cgi?id=665244
Nicolas Dufresne suggested using GstFilters to implement video effects.
http://gstconf.ubicast.tv/videos/gstreamer-and-farsight/

As far as I've seen gst-filter hasn't been merged into gstreamer:
https://bugzilla.gnome.org/show_bug.cgi?id=631574

What do you advise?

I don't think we should block this branch on GstFilters as we have no clear idea when they'll be merged. I'll re-review your branch, thanks!

Trying to build your branch I get:

empathy-call-window.c: In function ‘empathy_call_window_load_video_effects’:
empathy-call-window.c:1784:3: erreur: too many arguments to function ‘cheese_effect_new’
/usr/include/cheese/cheese-effect.h:56:15: note: declared here


I guess that's because of a libcheese API change. configure should check for the right version.

Updating to cheese master fixed the build so yeah, that's just a matter of checking for the right version.

I get those warnings when displaying the effect menu:

(empathy-call:32364): empathy-WARNING **: Error parsing effect name=Cartoon err=no element "frei0r-filter-cartoon"
(empathy-call:32364): empathy-WARNING **: Error parsing effect name=Che Guevara err=no element "frei0r-filter-twolay0r"
(empathy-call:32364): empathy-WARNING **: Error parsing effect name=Chrome err=no element "frei0r-filter-color-distance"
[New Thread 0x7fff96ffd700 (LWP 32415)]
(empathy-call:32364): empathy-WARNING **: Error parsing effect name=Distortion err=no element "frei0r-filter-distort0r"
(empathy-call:32364): empathy-WARNING **: Error parsing effect name=Invertion err=no element "frei0r-filter-invert0r"
(empathy-call:32364): empathy-WARNING **: Error parsing effect name=Sobel err=no element "frei0r-filter-sobel"
(empathy-call:32364): empathy-WARNING **: Error parsing effect name=Time delay err=no element "frei0r-filter-delay0r"


and indeed those effects don't have a preview in the menu. I guess I miss some gst elements to make them work? Couldn't we detect that and disable those effects?

Also the effects page take a LOT of time to load (between 5 and 10 seconds) is that normal? If we can't make it faster when should display a spinner or something if not user will expect that it failed.

I didn't look at the code yet.

Created attachment 202801 [details] [review]
empathy-call: remove redundant events for GtkClutterEmbed

GtkClutterEmbed seems to have the necessary widget events set.
We don't need to add them explicitly.

Created attachment 202802 [details] [review]
configure: add experimental --enable-video-effect switch

Empathy uses cheese in two places enabled with two separate switches:
  --with-cheese:         for avatar selection (req. cheese 2.91.79)
  --enable-video-effect: for video effects    (req. cheese 3.2.0)

This experimental feature is not enabled by default.

Created attachment 202803 [details] [review]
empathy-call: add a video-effects button

This ads a new button, but it's icon is (at the moment) not good. We
should either add a new icon to the gnome-theme and use that in both
Empathy and Cheese or copy Cheese' icon here.

The button doesn't do anything at the moment.

It is enabled (sensitive) only when we know we have a camera (as
detected by the camera monitor).

Created attachment 202804 [details] [review]
empathy-call: don't show video effects button when libcheese is unavailable

Created attachment 202805 [details] [review]
empathy-call: hide video output on video-effect menu action

Created attachment 202806 [details] [review]
empathy-call: add a wrapper box to separate video and effects

We only have the video box now (video_box), but we'll soon be adding
the effects_box.

Separating video and effects in two different boxes makes it easy to
switch from displaying video to displaying effect previews (and
reverse): we only have to hide one of the two boxes to have everything
inside them will be hidden too.

Created attachment 202807 [details] [review]
empathy-call: add empty effects_box

Created attachment 202808 [details] [review]
empathy-call: initialize cheese effects

Effects are not used in the UI.

ATM we only load the effects from known directories (implemented in
libcheese) and create the clutter-boxes with 3x3 effect previews.

Created attachment 202809 [details] [review]
empathy-call: show effect labels

Effects are not displayed; only labels.

Labels appear with a fade-in animation.

Created attachment 202810 [details] [review]
empathy-call: show effect previews

Effect previews are now displayed when selecting the effects menu.

A GstValve is used to stop effect previews from using CPU time when
they are not displayed.

ATM, clicking on an effect preview does nothing and there's no way to
scroll to the next page of effects.

Created attachment 202811 [details] [review]
empathy-call: prepare gst pipeline for sending effects on network

Added "ffmpegcolorspace ! identity ! ffmpegcolorspace" to
pipeline. Selecting an effect from the effect-preview page will
replace "identity" with that gstreamer effect.

Also added a "video_preview_valve" to be able to stop sending video on
the network and on the video_preview_sink, but at the same time keep
the video_input in PLAYING so that the effect previews work while we
don't send video on the network.

Created attachment 202812 [details] [review]
empathy-call: don't stop the camera while camera preview disabled

We no longer stop the camera while the camera preview is disabled
(when we're not sending video to the other party).

Before this patch there was a bug when calling echo@collabora.co.uk:
 - start a video call with echo@test.collabora.co.uk

 - wait until the camera video is echoed back

 - press the "camera" button to disable camera preview and sending
   video to echo@test.collabora.co.uk

 - wait until the echobot stops transmitting video back

 - press the "camera" button to enable camera preview and sending
   video to echo@test.collabora.co.uk

 - no matter how long you wait video doesn't come back any more.

With this patch, we don't have this problem any more: after pressing
"camera" the second time, we see video streaming back from the echobot
as expected.

Created attachment 202813 [details] [review]
empathy-call: set preview_valve drop=TRUE/FALSE when camera preview disabled/enabled

We'll soon add functionality to replace the identity video filter with
a user chosen effect filter. When an effect filter is selected and the
video preview is disabled it doesn't make sense to run video buffers
through the filter because they will not be used by anyone.

Created attachment 202814 [details] [review]
empathy-call: add video effect to preview and send it over network

Created attachment 202815 [details] [review]
empathy-call: fix pinkish video preview sink

Without this the small video preview for the users camera would have a
pinkish colour when using effects. The stream sent over the network
did not display these problems.

Resetting the clutter video sink seems to fix this.

Created attachment 202816 [details] [review]
empathy-call: make floating_toolbar visible in effect preview mode

The floating_toolbar was added in the video_layout which is hidden in
effect preview mode.

This patch adds it to the root-bin, making it visible in both
video-mode and effect-preview-mode.

New issue found: clicking inside the floating-toolbar even on buttons
will let the click fall-through to the effect-box bellow and select a
video-preview.

Created attachment 202817 [details] [review]
empathy-call: make the floating toolbar reactive

Make the floating_toolbar reactive to mouse events.

Without this, when the toolbar is displayed over the effect previews,
when you'd click inside the toolbar the mouse click would be sent to
the effect under it. This would even happen for "click" events for
buttons embedded in the toolbar!

Review of attachment 202801 [details] [review]:

++

Review of attachment 202802 [details] [review]:

::: configure.ac
@@ +56,3 @@
 GEOCLUE_REQUIRED=0.11
 ISO_CODES_REQUIRED=0.35
+LIBCHEESE_REQUIRED=3.2.0

As said above, this version isn't right any more.

Review of attachment 202803 [details] [review]:

Could you please file a gnome-icon-theme bug about the icon? (not a blocker).

::: src/empathy-call-window.c
@@ +625,3 @@
       g_list_length ((GList *) cameras) >= 2);
+  gtk_action_set_visible (self->priv->menu_video_effects,
+      g_list_length ((GList *) cameras) >= 1);

You could cache the result of g_list_length() and so save a O(n) call.

@@ +638,3 @@
       g_list_length ((GList *) cameras) >= 2);
+  gtk_action_set_visible (self->priv->menu_video_effects,
+      g_list_length ((GList *) cameras) >= 1);

same here.

@@ +1381,3 @@
+}
+
+empathy_call_window_video_effects_cb (GtkAction *action,

Would be easier to only connect the signal if needed.

Review of attachment 202804 [details] [review]:

++ Feel free to squash with previous commit.

Review of attachment 202805 [details] [review]:

Looks good but should probably be squashed into the patch implementing the rest of this callback.

Review of attachment 202806 [details] [review]:

++

Review of attachment 202807 [details] [review]:

++

Review of attachment 202808 [details] [review]:

::: src/empathy-call-window.c
@@ +204,3 @@
+#ifdef HAVE_VIDEO_EFFECT
+  /* array of CheeseEffect objects, sorted by name. First element=identity */
+  CheeseEffect **cheese_effects;

I'd use a GPtrArray, it's more convenient when dealing with dynamic arrays and you can use g_ptr_array_new_with_free_func() to make memory management of the array's content easier.

@@ +205,3 @@
+  /* array of CheeseEffect objects, sorted by name. First element=identity */
+  CheeseEffect **cheese_effects;
+  gint         cheese_effects_count;

guint. But could be dropped when using a GPtrArray; just use array->len.

@@ +206,3 @@
+  CheeseEffect **cheese_effects;
+  gint         cheese_effects_count;
+  ClutterActor **effect_pages;

Same here, use a GPtrArray.

@@ +1556,3 @@
+
+  g_object_get (G_OBJECT (pa), "name", &a, NULL);
+{

Could be nice to add : const gchar * cheese_effect_get_name (CheeseEffect *self); to save some memory allocation (not a blocker).

@@ +1570,3 @@
+  GList                 *list, *l;
+  int i;
+  g_free (a);

This function should be called only once. I'd ensure that with a g_return_if_fail (priv->cheese_effects == NULL);

@@ +1572,3 @@
+
+  list = cheese_effect_load_effects ();
+  g_free (b);

if (list == NULL)

@@ +1575,3 @@
+    return;
+
+}

this is redudant with the previous check. An empty list is NULL.

@@ +1591,3 @@
+
+  for (l = list, i = 1; l != NULL; l = g_list_next(l), i++)
+  int i;

When switching this code to GPtrArray please add a comment "pass ownership of the effect to the array" so we have an idea how the memory is managed.

@@ +1631,3 @@
+    }
+
+

What happen if we have more than 9 effects to display?

Review of attachment 202809 [details] [review]:

++

Review of attachment 202810 [details] [review]:

::: src/empathy-call-window.c
@@ +1429,3 @@
+  ClutterTexture        *texture;
+  GstElement            *eff_valve, *eff_csp1, *eff_filter, *eff_csp2,
+}

we use 4 spaces align in such case.

@@ +1457,3 @@
+
+  eff_valve = gst_element_factory_make ("valve", NULL);
+  gboolean              ok;

Why are you adding 2 colorspace elements?

@@ +1469,3 @@
+
+  gst_bin_add_many (GST_BIN (priv->pipeline), eff_valve, eff_csp1,
+  src_pad = g_object_get_data (G_OBJECT (effect), "src-pad");

Wouldn't be cleaner to add all of this as a separated bin?

@@ +1604,3 @@
+  priv->effects_valve = gst_element_factory_make ("valve", "effects_valve");
+  g_object_set (G_OBJECT (priv->effects_valve), "drop", TRUE, NULL);
+  priv->effects_tee = gst_element_factory_make ("tee", "effects_tee");

These elements are added to the pipeline even when built without effects support. I'd really prefer to not touch any existing code when building without effects.

Review of attachment 202811 [details] [review]:

::: src/empathy-call-window.c
@@ +1314,3 @@
+  gst_element_set_state (priv->video_preview_csp2, state);
+  gst_element_set_state (priv->video_preview_tee, state);
+  gst_element_set_state (priv->video_preview_sink, state);

Packing all of of this into a bin would make this code simpler.

@@ +2967,3 @@
+      if (priv->video_input_tee != NULL)
+        g_object_unref (priv->video_input_tee);
+      priv->video_input_tee = NULL;

use g_clear_object().

@@ +2971,3 @@
+      if (priv->video_preview_tee != NULL)
+        g_object_unref (priv->video_preview_tee);
+      priv->video_preview_tee = NULL;

same here.

@@ +4010,3 @@
   priv->video_input = NULL;
+  g_object_unref (priv->video_preview_tee);
+  priv->video_preview_tee = NULL;

g_object_clear().

Review of attachment 202812 [details] [review]:

Humm I'm not sure this is the right way to fix this. Olivier?

Review of attachment 202813 [details] [review]:

::: src/empathy-call-window.c
@@ +4443,3 @@
+  /* drop frames so that we don't run the filter uselessly */
+  g_object_set (G_OBJECT (priv->video_preview_valve),
+                "drop", !priv->sending_video, NULL);

add a \n after this call.

Review of attachment 202814 [details] [review]:

::: src/empathy-call-window.c
@@ +1842,3 @@
+    }
+  ok = gst_element_link_many (priv->video_preview_csp1,
+  g_object_set (G_OBJECT (priv->video_preview_valve), "drop", TRUE, NULL);

add a \n

Review of attachment 202815 [details] [review]:

Is that a gst or cheese bug or...?

Review of attachment 202816 [details] [review]:

::: src/empathy-call-window.c
@@ +2185,3 @@
       TRUE, TRUE, 0);
 
   /* root box: contains the avatar/video box and the effects box */

this comment should be updated

Review of attachment 202817 [details] [review]:

++

Review of attachment 202812 [details] [review]:

Humm I'm not sure this is the right way to fix this. Olivier?

This looks pretty good. You did a great job splitting patches, it makes review much easier, thanks!

In general, I'd like to have this code a no-op when building without video effects.

I'm not a GStreamer expert, so it would be good if someone could take a look on this part. Olivier ? :D

Created attachment 202893 [details] [review]
configure: add experimental --enable-video-effect

Updated libcheese required version to 3.3.2.

Review of attachment 202803 [details] [review]:

The patch comment is misleading, before Nick updated the document design, I put an "video effect" button on the floating toolbar.

Nick told me to move it to the "i" info menu (I don't understand why swapping video cameras and enabling video effects have to do with "Info", but that's just me :).

So, the comment was wrong, I've updated the patch accordingly.

Created attachment 202895 [details] [review]
empathy-call: add a video-effects menu entry in the  "i" info menu

Updated the patch comment (it used to talk about an older implementation).

Created attachment 202896 [details] [review]
empathy-call: initialize cheese effects

Review of attachment 202808 [details] [review]:

::: src/empathy-call-window.c
@@ +204,3 @@
+#ifdef HAVE_VIDEO_EFFECT
+  /* array of CheeseEffect objects, sorted by name. First element=identity */
+  CheeseEffect **cheese_effects;

This array doesn't change in size/contents.

We get all the items from libcheese in one call, the size of the array will be set once, the array will be allocated once.

Except for coupling array+effects I don't see the benefits + it hides the types of the items (no generics in C).

Are you sure you want this?

@@ +1556,3 @@
+
+  g_object_get (G_OBJECT (pa), "name", &a, NULL);
+{

Added: http://git.gnome.org/browse/cheese/commit/?id=e40fcd5655cce9578a06be247c4826e7c20048d5

but I'm not sure what to put in the required version of libcheese because 3.3.2 was just released, and I don't know the policy of releasing another version. 

Could you advise?

@@ +1631,3 @@
+    }
+
+

Please read the code bellow: it creates pages of 3x3 previews.

@@ +1668,3 @@
+
+      text = CLUTTER_TEXT (clutter_text_new ());
+{

I simplified this too by using cheese_effec_get_name

Guillaume: I realised why I left some GstElements in the main pipeline without #ifdef-ing.

I constructed the pipeline with the required valve/tee/etc. and an 'identity' filter. That means that if you don't use video effects, you just don't get to change the filter element (there's no code compiled in to do that).

I haven't bench-marked the impact of these elements on performance, but given the identity filter, the whole set of elements added to the pipeline shouldn't have a significant footprint.

I did it this way, because the code gets very ugly with lots of #ifdefs. I'm now working on converting the later patches, and it doesn't look good at all.

Are you sure you want this?

Review of attachment 202893 [details] [review]:

++

Review of attachment 202895 [details] [review]:

Commit msg is clearer but you didn't fix the other comments ( https://bugzilla.gnome.org/show_bug.cgi?id=654448#c48 )

Review of attachment 202896 [details] [review]:

::: src/empathy-call-window.c
@@ +1553,3 @@
+{
+  return g_strcmp0 (cheese_effect_get_name ((CheeseEffect*) a),
+      cheese_effect_get_name ((CheeseEffect*) b));

That's cool, but means the version in configure.ac is out dated again. :)
But that's fine, just update it once 3.3.4 has been released.

Review of attachment 202808 [details] [review]:

::: src/empathy-call-window.c
@@ +204,3 @@
+#ifdef HAVE_VIDEO_EFFECT
+  /* array of CheeseEffect objects, sorted by name. First element=identity */
+  CheeseEffect **cheese_effects;

I generally avoid to use dynamically allocated arrays directly. It makes the code harder to read, less Glib-ish and memory management harder.
Furthermore, the overhead of using a GPtrArray is very small if you do the allocation using:
  priv->cheese_effects = g_ptr_array_new_full (g_list_length (list) + 1, g_object_unref);

Regarding type safety we kinda loose as soon as we wrote "priv->cheese_effects[i] = (CheeseEffect*) l->data;" so... :)

Btw, cheese_effect and is content is leaked. You should free its memory in dispose. By using a GPtrArray all you'll have to do is calling g_ptr_array_unref().

@@ +1556,3 @@
+
+  g_object_get (G_OBJECT (pa), "name", &a, NULL);
+{

We can wait for cheese 3.3.3 to be released in 2 weeks (see https://live.gnome.org/ThreePointThree) or kindly ask to cheese maintainers if they can make a 3.3.2.1 for us.

On the other hand, as this is an optional dep for an experimental feature, I wouldn't mind too much if we merge it while depending on a soon to be released version of Cheese.

You forgot to push the latest version of the branch btw.

(In reply to comment #70)
> Guillaume: I realised why I left some GstElements in the main pipeline without
> #ifdef-ing.
> 
> I constructed the pipeline with the required valve/tee/etc. and an 'identity'
> filter. That means that if you don't use video effects, you just don't get to
> change the filter element (there's no code compiled in to do that).
> 
> I haven't bench-marked the impact of these elements on performance, but given
> the identity filter, the whole set of elements added to the pipeline shouldn't
> have a significant footprint.
> 
> I did it this way, because the code gets very ugly with lots of #ifdefs. I'm
> now working on converting the later patches, and it doesn't look good at all.
> 
> Are you sure you want this?

Fair enough. I'll refer to Olivier's advice as he knows GStreamer way better than I do.

I've added two branches (one with and one without isolating GstElements with HAVE_VIDEO_EFFECT)


See branches:
  cheese-video-effects-v3--ifdef-HAVE_VIDEO_EFFECT-festival
  cheese-video-effects-v3-less-HAVE_VIDEO_EFFECTs

on my github repo: https://github.com/raluca-elena/empathy-cheese

I don't think it's *that* bad tbh. Wouldn't packing most of the elements into its own bin (as suggested in Comment 55) make the code even simpler?

Review of attachment 202811 [details] [review]:

The second ffmpegcolorspace shouldn't be needed. It should be just before the bit that needs it. You also probably don't need the identity, you can probably just add/remove the effects on demand.

Review of attachment 202811 [details] [review]:

The second ffmpegcolorspace shouldn't be needed. It should be just before the bit that needs it. You also probably don't need the identity, you can probably just add/remove the effects on demand.

Review of attachment 202812 [details] [review]:

Sounds like random hacking to me. We should try to figure out the underlying cause of the problem.

Review of attachment 202815 [details] [review]:

Sounds very bug like a bug in Cluttersink.. There is nothing in Gst that would add a pinkish tint.

Review of attachment 202814 [details] [review]:

Are the effects only on the preview? It's not clear to me what the actual pipeline inside empathy-call is?

As a general note, you probably want to possibly use Youness's GstFilters (soon to be renamed), as it does all the dynamic pipeline modifications correctly for you.

Refs:
http://gstconf.ubicast.tv/videos/gstreamer-and-farsight/
http://kakaroto.homelinux.net/2010/08/fsu-gstreamer-made-simple/
http://people.collabora.com/~kakaroto/gpb-docs/gstreamer-filters.html

(In reply to comment #84)
> As a general note, you probably want to possibly use Youness's GstFilters (soon
> to be renamed), as it does all the dynamic pipeline modifications correctly for
> you.

I thought the plan was to merge it in GStreamer which would take a while, so I suggested to Raluca to not block on this. But, if I understand things correctly, it may be released as a separated module after all?

Review of attachment 202812 [details] [review]:

Agreed; I opened bug #665786 about this.

Review of attachment 202815 [details] [review]:

Raluca: can you file a clutter bug for this please?

Changing status of reviewed patch as they won't make it this release.

Hopefully we'll finish this for 3.6 now that empathy-call finally landed.

Guillaume, not that I understand you workflow and such, but seeing many bugs with attached many small patches which actually touches the same file, does it make any sense? For example, if anyone would like to test your changes before you actually commit it, wouldn't it be much simpler to apply one patch, instead of 6-10 really tiny patches? Like this bug, here are attached 17 patches, does anybody except you understand what is for what? I call this just a real mess.

It's way better to record small atomic git commits rather than a huge one. It makes code review and viewing the history easier.
One can easily test the branch by applying all the patches using git-bz.
Furthermore Raluca linked her github branch so it's even easier to test.

So, no, it's not a mess.

I see, it's just a different workflow :) I prefer longer patches myself, to have all relevant changes in one place. No problem, I apologize for disturbing.

Patch status for a few patches here has been "accepted-commit_after_freeze" for more than 3 months now. So can this get committed, or should the patch status be updated, or which exact freeze does this refer to?

Most of these patches come as a block, so ideally we should merge them together (the full branch) rather than one by one. Ideally they should be be rebased on top of the current master as well.

I'm currently trying to clean up the empathy call window code, so please wait before doing any rebasing.

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/empathy/issues/400.