Review of attachment 324517 [details] [review]:

Getting there, but not quite OK yet, please read on GstCaps and negotiation, it's a fairly essential part of what gstreamer does, arguably the most important one, can't hurt to learn about it :)

::: gst/faceoverlay/gstfaceoverlay.c
@@ +82,3 @@
     GST_PAD_SINK,
     GST_PAD_ALWAYS,
+    GST_STATIC_CAPS (GST_VIDEO_CAPS_MAKE ("{RGB}")));

You should not just blindly copy paste stuff from other plugins, and instead take this as an occasion to learn what GstCaps are, and how to define them, for example here in new caps lingo we were previously looking at video/x-raw, format=[RGB, YUV], this reduces the plugin to only accepting video/x-raw, format=[RGB], which is incorrect. You can experiment by defining caps with capsfilters on the command line, something like this should work:

 gst-launch-1.0 autovideosrc ! videoconvert ! video/x-raw, format=yuv ! faceoverlay location=/path/to/gnome-video-effects/pixmaps/bow.svg x=0.5 y=0.5 w=0.7 h=0.7 ! videoconvert ! autovideosink

and it will be refused with your patch, even though the code in this plugin can handle both rgb and yuv. This is important to specify these right, because it's what is used by gstreamer to let videoconvert convert to an acceptable format for the plugin, while not doing unnecessary conversion, as this is a costly operation.

@@ +87,3 @@
     GST_PAD_SRC,
     GST_PAD_ALWAYS,
+    GST_STATIC_CAPS (GST_VIDEO_CAPS_MAKE ("{BGRA}")));

Same comment applies here

The faceoverlay plugin was removed in git master, but it can be resurrected if anyone wants to work further on porting it to 1.x.

commit 9b5de053995488d5ddc78c1bf4df651101271d70
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Wed Dec 21 11:00:47 2016 +0200

    Remove various unported plugins
    
    If they were not ported after 4+ years it seems unlikely that anybody is
    ever going to need them again. They're still in the GIT history if
    needed.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=774530


In the mean time let's close this bug until that happens.

(In reply to Mathieu Duponchelle from comment #1)
> Review of attachment 324517 [details] [review] [review]:
> 
> Getting there, but not quite OK yet, please read on GstCaps and negotiation,
> it's a fairly essential part of what gstreamer does, arguably the most
> important one, can't hurt to learn about it :)
> 
> ::: gst/faceoverlay/gstfaceoverlay.c
> @@ +82,3 @@
>      GST_PAD_SINK,
>      GST_PAD_ALWAYS,
> +    GST_STATIC_CAPS (GST_VIDEO_CAPS_MAKE ("{RGB}")));
> 
> You should not just blindly copy paste stuff from other plugins, and instead
> take this as an occasion to learn what GstCaps are, and how to define them,
> for example here in new caps lingo we were previously looking at
> video/x-raw, format=[RGB, YUV], this reduces the plugin to only accepting
> video/x-raw, format=[RGB], which is incorrect. You can experiment by
> defining caps with capsfilters on the command line, something like this
> should work:
> 
>  gst-launch-1.0 autovideosrc ! videoconvert ! video/x-raw, format=yuv !
> faceoverlay location=/path/to/gnome-video-effects/pixmaps/bow.svg x=0.5
> y=0.5 w=0.7 h=0.7 ! videoconvert ! autovideosink
> 
> and it will be refused with your patch, even though the code in this plugin
> can handle both rgb and yuv. This is important to specify these right,
> because it's what is used by gstreamer to let videoconvert convert to an
> acceptable format for the plugin, while not doing unnecessary conversion, as
> this is a costly operation.


Do you mean that by setting only {RGB} on caps I am restricting it to only accept RGB format as input? Imagine I am developing the element from scratch. How can I know what should the input formats should be?

Created attachment 342673 [details] [review]
faceoverlay: Revert deletion and port to Gstreamer 1.x

To ease reviewing, can you provide this in two patches? One for reverting the deletion, another for the porting to 1.x?

Created attachment 349508 [details] [review]
faceoverlay: Revert deletion

Created attachment 349509 [details] [review]
faceoverlay: Port to GStreamer 1.x

Review of attachment 349509 [details] [review]:

The review comments from comment 1 still apply here. Basically the caps should be just "video/x-raw", as this is a bin that is just wrapping around other elements that already specify their caps correctly (and there's a videoconvert!).

Attachment 349508 [details] pushed as d4797e4 - faceoverlay: Revert deletion
Attachment 349509 [details] pushed as 456153c - faceoverlay: Port to GStreamer 1.x