[Bug 629244] [opencv] Add motion detection element

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=629244
  GStreamer | gst-plugins-bad | git

Stefan Kost (gstreamer, gtkdoc dev) <ensonic> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #192268|none                        |needs-work
             status|                            |

--- Comment #146 from Stefan Kost (gstreamer, gtkdoc dev) <ensonic at sonicpulse.de> 2011-07-20 08:14:23 UTC ---
Review of attachment 192268:
 --> (https://bugzilla.gnome.org/review?bug=629244&attachment=192268)

A few more comments:
1) For MotionCells.cpp MotionCells.h analitics.h motioncells_wrapper.cpp
motioncells_wrapper.h we'd like to have the same comment block at the top as in
gstmotioncells.c (the license is important).
2) would it be possible to merge the analitics.h into one of the other headers?
I'd like to make sure that it is easy to see which files in the opencv plugin
belong to which element

These are cosmetic. The element itself builds fine and first experiments show
that it is working. Would be great if you could join IRC, then we can discuss
there too - I am there as ensonic.

::: configure.ac
@@ +57,1 @@


its good to update to git head before making the patch

@@ +117,2 @@
 AS_PROG_OBJC


why do you need to set the OPENCV_FLAGS here?

::: ext/opencv/gstmotioncells.c
@@ +42,3 @@
+ * Boston, MA 02111-1307, USA.
+ */
+

Please add a gtk-doc blob here. Have a look at another element. This would also
help to understand the properties. Please include some example launch lines
(those that you use for testing).

@@ +217,3 @@
+  g_object_class_install_property (gobject_class, PROP_GRID_X,
+      g_param_spec_int ("gridx", "GRID X", "Motion Grid X", GRID_MIN,
+          GRID_MAX, GRID_DEF, G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));

For the 1st arg of g_param_spec_xxx() we usualy use a '-' to separate words.
Don't make the 2nd arg of g_param_spec_xxx() all caps. The 2nd arg should be a
slighly more read able version of 1st arg (e.g. use a ' ' instead of a '-').
Here is an example:

  g_object_class_install_property (gobject_class, PROP_CHECK_PERFECT,
      g_param_spec_boolean ("check-perfect", "Check For Perfect Stream",
          "Verify that the stream is time- and data-contiguous. "
          "This only logs in the debug log.  This will be deprecated in favor "
          "of the check-imperfect-timestamp/offset properties.",
          DEFAULT_CHECK_PERFECT, G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));

@@ +232,3 @@
+  g_object_class_install_property (gobject_class, PROP_GAP,
+      g_param_spec_int ("gap", "GAP", "GAP between clips", GAP_MIN, GAP_MAX,
+          GAP_DEF, G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));

A better description needed. Also write "Gap" instead of "GAP" - it is not an
abbreviation.

@@ +259,3 @@
+      g_param_spec_long ("date", "Motion Cell Date",
+          "Current Date in milliseconds", DATE_MIN, DATE_MAX, DATE_DEF,
+          G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));

What is the date used for?

@@ +267,3 @@
+      g_param_spec_string ("datafileextension", "DataFile Extension",
+          "Extension of datafile", DEF_DATAFILEEXT,
+          G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));

What is the datafile used for. If it is output of detected motion, then leave
that to the application. If you want add a tests/examples/motioncells_logger.c
that write motion-bus-messages to a log file.

::: ext/opencv/motioncells_wrapper.h
@@ +6,3 @@
+#ifndef motioncells_WRAPPER_H
+#define motioncells_WRAPPER_H
+

this should be all caps (MOTIONCELLS_WRAPPER_H)

@@ +49,3 @@
+#endif
+
+#endif                          /* BGDETECT_WRAPPER_H */

wrong header name in comment

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.