GNOME Bugzilla – Bug 642759
faceoverlay plugin: displays an SVG image over a detected face on a video stream
Last modified: 2016-03-22 03:28:02 UTC
Display an SVG image over a face on a video stream.
Created attachment 181324 [details] [review] faceoverlay plugin header file
Created attachment 181325 [details] [review] faceoverlay plugin .c file
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.
Created attachment 181559 [details] [review] faceoverlay plugin
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()"
Created attachment 184918 [details] [review] patch containing fixes asked for in the last review
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.
Created attachment 324513 [details] [review] 0001-faceoverlay-port-to-Gstreamer-1.x.patch port to Gstreamer 1.x
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" ?
(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
Port to Gstreamer 1.x is is now in https://bugzilla.gnome.org/show_bug.cgi?id=764011