Created attachment 169523 [details] [review]
patch v2

I use dummy plugin identity as place holder for situation if plugin is not found

Review of attachment 169523 [details] [review]:

But how much more CPU does that use? It seems to be a bit self defeating vs #628789

::: libempathy-gtk/empathy-video-src.c
@@ +74,3 @@
+    g_warning ("postproc_tmpnoise filter not found, probably gst-ffmpeg"
+               " is not installed. We use place holder instead.");
+    noisefilter = gst_element_factory_make ("identity", NULL);

Don't put identity, just leave is NULL and don't add it if its not there

@@ +90,3 @@
+    capsfilter, NULL);
+  gst_element_link_many (priv->src, scale, colorspace, noisefilter,
+    capsfilter, NULL);

You should do

if (noisefilter)
{
  gst_bin_add(bin, noisefilter);
  gst_element_link_many (priv->src, scale, colorspace, noisefilter,
	capsfilter, NULL);
}
else
{
  gst_element_link_many (priv->src, scale, colorspace, capsfilter, NULL);
}

Also, please avoid the *_many GStreamer functions, they don't tell you if something has gone wrong. You want to run gst_bin_add() and gst_element_link() one element at a time and check the return value (although I do realize that this was already like that)

you should see the code with bug 628789 together. We have two optional plugins this will make more variants with and without noisefilter and with/without maxvideorate. Espesialy if this two can/will be linked together. Indepedet of what we use *_link_many or _bin_add, _elemnt_link.

About CPU load, it make minimal overhead but reduce load for encoder, becouse of reduced motion noise.

Any way, before i do my patch i need to convert gst_element_link_many to gst_bin_add

Created attachment 169613 [details] [review]
new funcion v1

Here is my init patch for new function to create factory, add and link it to bin and to report errors.
Please feetback .D

Created attachment 169649 [details] [review]
new function v2

Created attachment 169650 [details] [review]
new function v2

oops, to many changes was included in patch.

Review of attachment 169650 [details] [review]:

::: libempathy-gtk/empathy-video-src.c
@@ +79,3 @@
+  if (!gst_bin_add (bin, ret))
+  {
+    g_error ("Can't add \"%s\" do bin.", factoryname);

Do you really want to do g_error() if it fails ? (Maybe you actually do in this case.. hmm).
Also, you should do gst_object_unref(ret); if you dont abort() (g_error() calls abort())..

@@ +89,3 @@
+  if (!gst_element_link (src, ret))
+  {
+    g_warning ("Can't link \"%s\".", factoryname);

Also here if it fails, you probably want to do gst_bin_remove() to clean up the element.

Review of attachment 169650 [details] [review]:

::: libempathy-gtk/empathy-video-src.c
@@ +112,3 @@
+//  if(!priv->src)
+//    goto error;
+

Don't use C++ style comments, don't comment code just removed it if you don't need it anymore. But in this case when we have no source we should error out not try to continue on

@@ +128,1 @@
   g_object_set (G_OBJECT (capsfilter), "caps", caps, NULL);

If capsfilter creation might fail you chould check that, also we should fail the whole pipeline if any of those elements fail (they're all in the basic elements you need)

::: libempathy-gtk/empathy-video-src.h
@@ +86,1 @@
 

Why is this function exported ? also the name seems awkward

(In reply to comment #8)
> Review of attachment 169650 [details] [review]:
> 
> ::: libempathy-gtk/empathy-video-src.c
> @@ +79,3 @@
> +  if (!gst_bin_add (bin, ret))
> +  {
> +    g_error ("Can't add \"%s\" do bin.", factoryname);
> 
> Do you really want to do g_error() if it fails ? (Maybe you actually do in this
> case.. hmm).
> Also, you should do gst_object_unref(ret); if you dont abort() (g_error() calls
> abort())..

You right, it probably make sense to make this funktion more universal and use some thing lees critical. If for some reason we can't talk we still can lissen. With abort() user will not get what is broken.

(In reply to comment #9)
> Why is this function exported ? also the name seems awkward

This is initial patch, i think it will be better to use it later for audio/videi_src/sink too. So the funktion should be placed some were els.
I can't discuss about the name.. if have some thing better i'm open for other names.

Created attachment 169693 [details] [review]
new function v3

Chagelist:
- added gst_object_unref (ret)
- g_error was changed to g_warning , caller should decide haw important it is.
- more error exits added

todo:
- hot to check if caps was added?

Created attachment 169702 [details] [review]
new function v4

added check for caps.

It looks ok for me.
Comments?

Created attachment 169762 [details] [review]
new funcion v5

changes according review on irc.

- g_object_set (G_OBJECT (capsfilter), "caps", caps, NULL) is back
- gst_bin_remove (bin, ret) added after filed linking

Created attachment 169763 [details] [review]
new funcion v6

and updated g_error messages, or tester do not wont to talk to me any more :/

Created attachment 169769 [details] [review]
new funcion v7 (for git)

Created attachment 169806 [details] [review]
noisefilter + maxrate patch

This patch is actual target of my work. It was easier for me to add this elements together, so i didn't split it to two patches.

Optional elements add some complication, it is harder to read (at least for me it looks like bloody mess)

Review of attachment 169806 [details] [review]:

You can probably do like stream-engine and have

element = priv->src;
element = make...(element, "filter1");
element = make..(element, "filter2");
etc

And the make... function is the one that appends/links the element if possible otherwise does nothing.

::: libempathy-gtk/empathy-video-src.c
@@ +140,3 @@
+  }
+
+  g_debug ("Current video src caps are : %s", gst_caps_to_string(caps));

gst_caps_to_string() mallocs a string so you need to do
str = gst_caps_to_string(caps);
g_debug ("Current video src caps are : %s", str);
g_free(str);

Review of attachment 169769 [details] [review]:

Looks good

Created attachment 169894 [details] [review]
noisefilter + maxrate patch v2 (git)

I fixed some (trivial) coding style issues and merged the 2 patches. Thanks a lot for your work on this!

This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.

commit dac42b77124239862e31b2b5d23df362b4ce2f91
Author: Alexey Fisher <bug-track@fisher-privat.net>
Date:   Thu Sep 9 22:28:21 2010 +0200

    Add two optional filter to video src
    
    Add videomaxrate and postproc_tmpnoise as optional filters to
    empathy_video_src_init. Thay will used if thay are installed.
    
    videomaxrate will set maximal framerate to 15fps if some haw webcam
    will get more than 15fps it will not kill empathy-av with high CPU load.
    
    postproc_tmpnoise will reduce video noise produced by most (all) webcams.
    This will add smoll overhead for load but reduce CPU load of video encoder.
    Also this should reduce network load (for theoraenc x2)
    
    Signed-off-by: Alexey Fisher <bug-track@fisher-privat.net>

commit 4815a75a72a0ebacc7e2f29e7026b8493c6ea320
Author: Alexey Fisher <bug-track@fisher-privat.net>
Date:   Mon Sep 13 16:43:51 2010 +0200

    Add empathy_gst_make_to_bin

I opened bug #629532 about proper error report.