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 642759 - faceoverlay plugin: displays an SVG image over a detected face on a video stream
faceoverlay plugin: displays an SVG image over a detected face on a video stream
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
unspecified
Other Linux
: Normal normal
: 0.10.23
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2011-02-19 15:29 UTC by Laura Lucas Alday
Modified: 2016-03-22 03:28 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
faceoverlay plugin header file (3.29 KB, patch)
2011-02-19 15:30 UTC, Laura Lucas Alday
none Details | Review
faceoverlay plugin .c file (16.11 KB, patch)
2011-02-19 15:31 UTC, Laura Lucas Alday
none Details | Review
faceoverlay plugin (22.42 KB, patch)
2011-02-22 07:21 UTC, Laura Lucas Alday
none Details | Review
patch containing fixes asked for in the last review (21.38 KB, patch)
2011-04-02 00:28 UTC, Laura Lucas Alday
committed Details | Review
0001-faceoverlay-port-to-Gstreamer-1.x.patch (4.28 KB, patch)
2016-03-22 02:01 UTC, César Fabián Orccón Chipana
rejected Details | Review

Description Laura Lucas Alday 2011-02-19 15:29:27 UTC
Display an SVG image over a face on a video stream.
Comment 1 Laura Lucas Alday 2011-02-19 15:30:41 UTC
Created attachment 181324 [details] [review]
faceoverlay plugin header file
Comment 2 Laura Lucas Alday 2011-02-19 15:31:34 UTC
Created attachment 181325 [details] [review]
faceoverlay plugin .c file
Comment 3 Stefan Sauer (gstreamer, gtkdoc dev) 2011-02-19 17:57:37 UTC
Cool! Can you make this as a patch against gst-plugins-bad (that would make the patch review tool work)?

Wonder why it has these in the .c and .h
 * Copyright (C) 2005 Thomas Vander Stichele <thomas@apestaart.org>
 * Copyright (C) 2005 Ronald S. Bultje <rbultje@ronald.bitfreak.net>

As a first comment I would use NULL as a 2nd arg of
gst_element_factory_make() to avoid clashes with existing element names.
Comment 4 Laura Lucas Alday 2011-02-22 07:21:36 UTC
Created attachment 181559 [details] [review]
faceoverlay plugin
Comment 5 Stefan Sauer (gstreamer, gtkdoc dev) 2011-02-28 13:45:12 UTC
Irks splinter is broken, manual copy'n'paste of the comment below:

Looks good. A bunch of comments though:
Please also run the c-file through gst-indent (the coding style differs from the other plugins right now).

gst/faceoverlay/gstfaceoverlay.c

201	 
202	static gboolean
203	gst_faceoverlay_create_childs (Gstfaceoverlay *filter)
	
-> create_children :)


325	g_object_set(filter->svg_overlay, "y", svg_y, NULL);
326	g_object_set(filter->svg_overlay, "width", svg_width, NULL);
327	g_object_set(filter->svg_overlay, "height", svg_height, NULL);
	
use one g_object_set() call, its faster.


331	g_object_set(filter->svg_overlay, "location", NULL, NULL);
332	}
333	 
	
I believe you will need to chainup to the GstBin::message_handler.


347	"Filter/Editor/Video",
348	"Overlays SVG graphics over a detected face in a video stream",
349	"Laura Lucas Alday <<lauralucas@gmail.com>>");
	
There is a double "<<".


gst/faceoverlay/gstfaceoverlay.h
63	typedef struct _Gstfaceoverlay Gstfaceoverlay;
64	typedef struct _GstfaceoverlayClass GstfaceoverlayClass;
65	 
	
please use CamelCase "GstFaceOverlay" and "GstFaceOverlayClass"


66	struct _Gstfaceoverlay
67	{
68	GstBin faceoverlaybin;
	
by convention we would just call this "GstBin parent;"


80	float y;
81	float w;
82	float h;
	
nitpick, use gfloat instead of float


89	 
90	GType gst_faceoverlay_get_type (void);
91	 
	
according to the naming change above, this would become "gst_face_overlay_get_type()"
Comment 6 Laura Lucas Alday 2011-04-02 00:28:27 UTC
Created attachment 184918 [details] [review]
patch containing fixes asked for in the last review
Comment 7 Sebastian Dröge (slomo) 2011-05-30 10:28:33 UTC
commit 8aebdeb35da3b49d02a1442a54af2e47360cae33
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Mon May 30 12:27:37 2011 +0200

    faceoverlay: Add gst-plugins-base CFLAGS and LIBS to fix compilation

commit d11178aac033f64060988de0e3f37867babe18f2
Author: Laura Lucas Alday <lauralucas@gmail.com>
Date:   Mon May 30 12:24:31 2011 +0200

    faceoverlay: Add element that displays an SVG image over a detected face
    
    Fixes bug #642759.
Comment 8 César Fabián Orccón Chipana 2016-03-22 02:01:52 UTC
Created attachment 324513 [details] [review]
0001-faceoverlay-port-to-Gstreamer-1.x.patch

port to Gstreamer 1.x
Comment 9 Mathieu Duponchelle 2016-03-22 02:30:48 UTC
Review of attachment 324513 [details] [review]:

You should ask for review in a separate, new bug, not in a RESOLVED FIXED one from 2011 ;) I haven't actually tried your patch, but I assume it works for your use case, however you have reduced the negotiation capabilities of this element to nothing on your end, please fix that :)

::: gst/faceoverlay/gstfaceoverlay.c
@@ +82,3 @@
     GST_PAD_SINK,
     GST_PAD_ALWAYS,
+    GST_STATIC_CAPS_ANY);

You are now saying that this plugin can get anything as an input, which means I should be able to push an encoded mp3 stream in its direction, is that the case?

@@ +87,3 @@
     GST_PAD_SRC,
     GST_PAD_ALWAYS,
+    GST_STATIC_CAPS_ANY);

Same thing here, can this plugin output anything other than video/x-raw, format=[YUV, RGB] ?

@@ -93,1 +91,1 @@
-GST_BOILERPLATE (GstFaceOverlay, gst_face_overlay, GstBin, GST_TYPE_BIN);
+#define gst_face_overlay_parent_class parent_class

Is this needed at all ? I honestly don't know whether this was something defined by GST_BOILERPLATE and used in this file, it does not seem to be used in this diff's context.

@@ -255,3 +254,3 @@
   }
 
-  GST_LOG_OBJECT (filter, "overlay dimensions: %d x %d @ %d,%d",
+  GST_DEBUG_OBJECT (filter, "overlay dimensions: %d x %d @ %d,%d",

Any good reason for promoting this to "DEBUG" ?
Comment 10 César Fabián Orccón Chipana 2016-03-22 03:27:07 UTC
(In reply to Mathieu Duponchelle from comment #9)
> Review of attachment 324513 [details] [review] [review]:
> 
> You should ask for review in a separate, new bug, not in a RESOLVED FIXED
> one from 2011 ;) I haven't actually tried your patch, but I assume it works
> for your use case, however you have reduced the negotiation capabilities of
> this element to nothing on your end, please fix that :)
> 
> ::: gst/faceoverlay/gstfaceoverlay.c
> @@ +82,3 @@
>      GST_PAD_SINK,
>      GST_PAD_ALWAYS,
> +    GST_STATIC_CAPS_ANY);
> 
> You are now saying that this plugin can get anything as an input, which
> means I should be able to push an encoded mp3 stream in its direction, is
> that the case?
> 
> @@ +87,3 @@
>      GST_PAD_SRC,
>      GST_PAD_ALWAYS,
> +    GST_STATIC_CAPS_ANY);
> 
> Same thing here, can this plugin output anything other than video/x-raw,
> format=[YUV, RGB] ?
I have decided to use sink template and src template as its children use.
> 
> @@ -93,1 +91,1 @@
> -GST_BOILERPLATE (GstFaceOverlay, gst_face_overlay, GstBin, GST_TYPE_BIN);
> +#define gst_face_overlay_parent_class parent_class
> 
> Is this needed at all ? I honestly don't know whether this was something
> defined by GST_BOILERPLATE and used in this file, it does not seem to be
> used in this diff's context.
> 
It defines parent_class that is used in original code.
> @@ -255,3 +254,3 @@
>    }
>  
> -  GST_LOG_OBJECT (filter, "overlay dimensions: %d x %d @ %d,%d",
> +  GST_DEBUG_OBJECT (filter, "overlay dimensions: %d x %d @ %d,%d",
> 
> Any good reason for promoting this to "DEBUG" ?
No. Changed as it was.

https://bugzilla.gnome.org/show_bug.cgi?id=764011
Comment 11 César Fabián Orccón Chipana 2016-03-22 03:28:02 UTC
Port to Gstreamer 1.x is is now in https://bugzilla.gnome.org/show_bug.cgi?id=764011