Review of attachment 245821 [details] [review]:

::: ext/opencv/gstsegmentation.c
@@ +195,3 @@
+  g_object_class_install_property (gobject_class, PROP_DISPLAY,
+      g_param_spec_boolean ("display", "Display",
+          "Display the foreground as white over background black ",

Maybe call that property black-white instead? display sounds like it enables/disables displaying of the effect

@@ +298,3 @@
+  /* Codebook method */
+  segmentation->TcodeBook = (codeBook *)
+      g_malloc (sizeof (codeBook) *

This must be released with release_all_images() too, otherwise you leak it

@@ +401,3 @@
+  }
+  /* Create the foreground and background masks using BackgroundSubtractorMOG [1], 
+   *  Gaussian Mixture-based Background/Foreground segmentation slgorithm. OpenCV 

Typo slgorithm

@@ +461,3 @@
+
+
+#ifdef CODE_FROM_OREILLY_BOOK

Why? :)

::: ext/opencv/opencv_wrapper.h
@@ +1,1 @@
+/*

You could also just put your element into a .cpp file instead of having this wrapper :)

(In reply to comment #1)
> Review of attachment 245821 [details] [review]:
> 
> ::: ext/opencv/gstsegmentation.c
> @@ +195,3 @@
> +  g_object_class_install_property (gobject_class, PROP_DISPLAY,
> +      g_param_spec_boolean ("display", "Display",
> +          "Display the foreground as white over background black ",
> 
> Maybe call that property black-white instead? display sounds like it
> enables/disables displaying of the effect

Well, that's what it does, instead of the "normal" output, which is FG/BG in the alpha channel, "display=true" applies the mask over the input, thus creating a black and white output. It could be applied as transparency over the colour image, but in my experience this is a debug parameter used to check if the movement mask creation is going all right.

> 
> @@ +298,3 @@
> +  /* Codebook method */
> +  segmentation->TcodeBook = (codeBook *)
> +      g_malloc (sizeof (codeBook) *
> 
> This must be released with release_all_images() too, otherwise you leak it

It is released in gst_segmentation_stop(), which in turn calls release_all_images().

Btw I forgot to delete the two pointers in opencv_wrapper.cpp, I added (finalise_mog) it now :)

> 
> @@ +401,3 @@
> +  }
> +  /* Create the foreground and background masks using BackgroundSubtractorMOG
> [1], 
> +   *  Gaussian Mixture-based Background/Foreground segmentation slgorithm.
> OpenCV 
> 
> Typo slgorithm

Done.

> 
> @@ +461,3 @@
> +
> +
> +#ifdef CODE_FROM_OREILLY_BOOK
> 
> Why? :)

Well, this code is not mine, so I wanted to make it absolutely clear that I copied it. It's also mentioned in the first lines of the file, copyright stuff:

 * Except: Parts of code inside the preprocessor define CODE_FROM_OREILLY_BOOK, 
 * which are downloaded from O'Reilly website and adapted.

> 
> ::: ext/opencv/opencv_wrapper.h
> @@ +1,1 @@
> +/*
> 
> You could also just put your element into a .cpp file instead of having this
> wrapper :)

Ah, I didn't know. However, more and more OpenCV functionality comes just with C++ wrappers, and I have a couple of other elements/ideas in the pipeline that would also need to have (a bit larger) C++ wrapper. I find it clearer to have both separated, C for GStreamer and C++ for OpenCV :) but I can merge them if you think it's best.


I'll upload another patch with the mentioned changes.

Created attachment 246024 [details] [review]
2nd iteration segmentation plugin and associated opencv c++ wrapper

Addressed the comments from Sebastian Droege, added finalise_mog() function to free the static pointers in opencv_wrapper.cpp, called from gst_segmentation_stop(), at the same time that Images and Tcodebook are free'd.

(In reply to comment #2)
> (In reply to comment #1)
> > Review of attachment 245821 [details] [review] [details]:
> > 
> > ::: ext/opencv/gstsegmentation.c
> > @@ +195,3 @@
> > +  g_object_class_install_property (gobject_class, PROP_DISPLAY,
> > +      g_param_spec_boolean ("display", "Display",
> > +          "Display the foreground as white over background black ",
> > 
> > Maybe call that property black-white instead? display sounds like it
> > enables/disables displaying of the effect
> 
> Well, that's what it does, instead of the "normal" output, which is FG/BG in
> the alpha channel, "display=true" applies the mask over the input, thus
> creating a black and white output. It could be applied as transparency over the
> colour image, but in my experience this is a debug parameter used to check if
> the movement mask creation is going all right.

Ok, let's keep it then :) display just sounds so extremely generic
 
> > @@ +298,3 @@
> > +  /* Codebook method */
> > +  segmentation->TcodeBook = (codeBook *)
> > +      g_malloc (sizeof (codeBook) *
> > 
> > This must be released with release_all_images() too, otherwise you leak it
> 
> It is released in gst_segmentation_stop(), which in turn calls
> release_all_images().

Yes, but if set_format() is called multiple times you will reallocate that without freeing it first

> > @@ +461,3 @@
> > +
> > +
> > +#ifdef CODE_FROM_OREILLY_BOOK
> > 
> > Why? :)
> 
> Well, this code is not mine, so I wanted to make it absolutely clear that I
> copied it. It's also mentioned in the first lines of the file, copyright stuff:
> 
>  * Except: Parts of code inside the preprocessor define CODE_FROM_OREILLY_BOOK, 
>  * which are downloaded from O'Reilly website and adapted.

What's the license of that then? I would just add a comment on top of that code block with the license and every important information

> > ::: ext/opencv/opencv_wrapper.h
> > @@ +1,1 @@
> > +/*
> > 
> > You could also just put your element into a .cpp file instead of having this
> > wrapper :)
> 
> Ah, I didn't know. However, more and more OpenCV functionality comes just with
> C++ wrappers, and I have a couple of other elements/ideas in the pipeline that
> would also need to have (a bit larger) C++ wrapper. I find it clearer to have
> both separated, C for GStreamer and C++ for OpenCV :) but I can merge them if
> you think it's best.

I think I prefer to just put the element in a c++ file instead (see the taglib plugin for an example), there's no real need for such C wrappers and they only blow up the source size :)

(In reply to comment #4)
> (In reply to comment #2)
> > (In reply to comment #1)
> > > Review of attachment 245821 [details] [review] [details] [details]:
> > > 
> > > ::: ext/opencv/gstsegmentation.c
> > > @@ +195,3 @@
> > > +  g_object_class_install_property (gobject_class, PROP_DISPLAY,
> > > +      g_param_spec_boolean ("display", "Display",
> > > +          "Display the foreground as white over background black ",
> > > 
> > > Maybe call that property black-white instead? display sounds like it
> > > enables/disables displaying of the effect
> > 
> > Well, that's what it does, instead of the "normal" output, which is FG/BG in
> > the alpha channel, "display=true" applies the mask over the input, thus
> > creating a black and white output. It could be applied as transparency over the
> > colour image, but in my experience this is a debug parameter used to check if
> > the movement mask creation is going all right.
> 
> Ok, let's keep it then :) display just sounds so extremely generic
> 
> > > @@ +298,3 @@
> > > +  /* Codebook method */
> > > +  segmentation->TcodeBook = (codeBook *)
> > > +      g_malloc (sizeof (codeBook) *
> > > 
> > > This must be released with release_all_images() too, otherwise you leak it
> > 
> > It is released in gst_segmentation_stop(), which in turn calls
> > release_all_images().
> 
> Yes, but if set_format() is called multiple times you will reallocate that
> without freeing it first
> 
I renamed the function gst_segmentation_release_all_images() to ..._release_all_pointers() and now it is free'ing all allocated pointers, including a bunch of void* for MOG methods, see below.

> > > @@ +461,3 @@
> > > +
> > > +
> > > +#ifdef CODE_FROM_OREILLY_BOOK
> > > 
> > > Why? :)
> > 
> > Well, this code is not mine, so I wanted to make it absolutely clear that I
> > copied it. It's also mentioned in the first lines of the file, copyright stuff:
> > 
> >  * Except: Parts of code inside the preprocessor define CODE_FROM_OREILLY_BOOK, 
> >  * which are downloaded from O'Reilly website and adapted.
> 
> What's the license of that then? I would just add a comment on top of that code
> block with the license and every important information
> 

I added some more info on it right after the Copyright, it reads now:

 * Except: Parts of code inside the preprocessor define CODE_FROM_OREILLY_BOOK, 
 *  which are downloaded from O'Reilly website 
 *  [http://examples.oreilly.com/9780596516130/]
 *  and adapted. Its license reads:
 *  "Oct. 3, 2008
 *   Right to use this code in any way you want without warrenty, support or 
 *   any guarentee of it working. "

The code does not have a particular license such as BSD, GPL or Mozilla. (Typos are also theirs ;) )


> > > ::: ext/opencv/opencv_wrapper.h
> > > @@ +1,1 @@
> > > +/*
> > > 
> > > You could also just put your element into a .cpp file instead of having this
> > > wrapper :)
> > 
> > Ah, I didn't know. However, more and more OpenCV functionality comes just with
> > C++ wrappers, and I have a couple of other elements/ideas in the pipeline that
> > would also need to have (a bit larger) C++ wrapper. I find it clearer to have
> > both separated, C for GStreamer and C++ for OpenCV :) but I can merge them if
> > you think it's best.
> 
> I think I prefer to just put the element in a c++ file instead (see the taglib
> plugin for an example), there's no real need for such C wrappers and they only
> blow up the source size :)

I put the C++ code on the C file and renamed it to .cpp. I wanted to avoid rewriting much code so the GstSegmentation struct holds void* to the c++ classes, they are allocated and destroyed in initialise_mog() and finalise_mog(), and casted appropriately in run_mog_iteration and run_mog2_iteration. (void*) is a hack to avoid tossing c++ classes into the header, hence into gstopencv.c etc.


I also added a new parameter, float PROP_LEARNING_RATE.

New patch following shortly.

Created attachment 246359 [details] [review]
 3rd iteration segmentation element

- All c and c++ code into gstsegmentation.cpp, MOG and MOG2 classes manipulated as void* from c.
- Clarified copyright on O'Reilly code
- added parameter for fg/bg learning_rate.

Review of attachment 246359 [details] [review]:

::: ext/opencv/gstsegmentation.cpp
@@ +206,3 @@
+  g_object_class_install_property (gobject_class, PROP_DISPLAY,
+      g_param_spec_boolean ("display", "Display",
+          "Overwrite the output with foreground as white over background black",

The description could be more detailed to tell when one would want to enable this:
"Overwrite the output with foreground as white over background black to check that ..."
The property name could probably be changed to "apply-mask" or "test-mode". I also first though that "display" would be there to switch the effect to pass-through mode.

Review of attachment 246359 [details] [review]:

::: ext/opencv/gstsegmentation.cpp
@@ +135,3 @@
+      {0, NULL, NULL},
+    };
+    etype = g_enum_register_static ("GstSegmentationMethod", values);

The type name should probably contain the element name, otherwise there is risk of conflict

@@ +350,3 @@
+gst_segmentation_release_all_pointers (GstSegmentation * filter)
+{
+  cvReleaseImage (&filter->cvRGBA);

You should set at least this one here to NULL, otherwise there's risk for double free. Or is cvReleaseImage() doing that already?

@@ +363,3 @@
+
+static GstFlowReturn
+gst_segmentation_transform_ip (GstBaseTransform * btrans, GstBuffer * buf)

Note that GstVideoFilter has its own vfunc for this:
  GstFlowReturn (*transform_frame)    (GstVideoFilter *filter,
                                       GstVideoFrame *inframe, GstVideoFrame *outframe);
  GstFlowReturn (*transform_frame_ip) (GstVideoFilter *trans, GstVideoFrame *frame);

You might want to use that, or use GstOpenCVVideoFilter

(In reply to comment #8)
> Review of attachment 246359 [details] [review]:
> 
> ::: ext/opencv/gstsegmentation.cpp
> @@ +135,3 @@
> +      {0, NULL, NULL},
> +    };
> +    etype = g_enum_register_static ("GstSegmentationMethod", values);
> 
> The type name should probably contain the element name, otherwise there is risk
> of conflict
> 
> @@ +350,3 @@
> +gst_segmentation_release_all_pointers (GstSegmentation * filter)
> +{
> +  cvReleaseImage (&filter->cvRGBA);
> 
> You should set at least this one here to NULL, otherwise there's risk for
> double free. Or is cvReleaseImage() doing that already?

It should do it [http://opencv.willowgarage.com/documentation/c/core_utility_and_system_functions_and_macros.html#cvFree]; at core, is just (a couple of) free() calls.

> 
> @@ +363,3 @@
> +
> +static GstFlowReturn
> +gst_segmentation_transform_ip (GstBaseTransform * btrans, GstBuffer * buf)
> 
> Note that GstVideoFilter has its own vfunc for this:
>   GstFlowReturn (*transform_frame)    (GstVideoFilter *filter,
>                                        GstVideoFrame *inframe, GstVideoFrame
> *outframe);
>   GstFlowReturn (*transform_frame_ip) (GstVideoFilter *trans, GstVideoFrame
> *frame);
> 
> You might want to use that, or use GstOpenCVVideoFilter

I did so and also I changed set_caps() with set_info(), I think in the line of the GstVideoFilter.

(In reply to comment #7)
> Review of attachment 246359 [details] [review]:
> 
> ::: ext/opencv/gstsegmentation.cpp
> @@ +206,3 @@
> +  g_object_class_install_property (gobject_class, PROP_DISPLAY,
> +      g_param_spec_boolean ("display", "Display",
> +          "Overwrite the output with foreground as white over background
> black",
> 
> The description could be more detailed to tell when one would want to enable
> this:
> "Overwrite the output with foreground as white over background black to check
> that ..."
> The property name could probably be changed to "apply-mask" or "test-mode". I
> also first though that "display" would be there to switch the effect to
> pass-through mode.

I changed it to "test-mode", the description reads:

  test-mode           : If true, the output RGB is overwritten with the calculated foreground (white color)
                        flags: readable, writable
                        Boolean. Default: false

Created attachment 246400 [details] [review]
4th iteration segmentation element

- changed parameter name "display"->"test-mode"
- changed transform_ip from base to VideoFilter "type"
- more VideoFilter "methods": set_info() used in place of set_caps()

Review of attachment 246400 [details] [review]:

::: ext/opencv/gstsegmentation.cpp
@@ +295,3 @@
+static gboolean
+//gst_segmentation_set_caps (GstBaseTransform * btrans, GstCaps * incaps,
+//    GstCaps * outcaps)

Don't use C99/C++ comments, also can just go away, no?

@@ +303,3 @@
+  GstVideoInfo info;
+  CvSize size;
+  gst_video_info_from_caps (&info, incaps);

You already get the info passed as parameter

@@ +375,3 @@
+
+  /*  get image data from the input, which is RGBA */
+  filter->cvRGBA->imageData = (char *) GST_VIDEO_FRAME_COMP_DATA (frame, 0);

You might want to do something to handle the different possible strides here too... if OpenCV supports that.

(In reply to comment #12)
> Review of attachment 246400 [details] [review]:
> 
> ::: ext/opencv/gstsegmentation.cpp
> @@ +295,3 @@
> +static gboolean
> +//gst_segmentation_set_caps (GstBaseTransform * btrans, GstCaps * incaps,
> +//    GstCaps * outcaps)
> 
> Don't use C99/C++ comments, also can just go away, no?

Yes, I just commented it out for a moment, when substituting set_caps with set_info() function and they stood. Now removed.

> 
> @@ +303,3 @@
> +  GstVideoInfo info;
> +  CvSize size;
> +  gst_video_info_from_caps (&info, incaps);
> 
> You already get the info passed as parameter

True as well. Taken into.
> 
> @@ +375,3 @@
> +
> +  /*  get image data from the input, which is RGBA */
> +  filter->cvRGBA->imageData = (char *) GST_VIDEO_FRAME_COMP_DATA (frame, 0);
> 
> You might want to do something to handle the different possible strides here
> too... if OpenCV supports that.

Well it might well do, I'm not sure because I've never used openCV with funky-aligned data. It relies on IplImage for that. I'm setting it now from the video input frame:

  filter->cvRGBA->widthStep = (char *) GST_VIDEO_FRAME_COMP_PSTRIDE (frame, 0);

New patch coming.

Sorry, nonsense last line, should read:

  filter->cvRGBA->widthStep = GST_VIDEO_FRAME_COMP_STRIDE (frame, 0);

Created attachment 246503 [details] [review]
5th iteration segmentation element 

- addressed Sebastian's comments
- added "stride" setting in the incoming RGBA frame in transform_ip() function.

Great, thanks :) Please attach in "git format-patch" style in the future, that makes merging easier :)

commit c313e1d3b848a9450225525923122d3b16f5d474
Author: Miguel Casas-Sanchez <miguelecasassanchez@gmail.com>
Date:   Tue Jun 11 14:32:43 2013 +0200

    opencv: add foreground/background segmentation element
    
    Add an element to the opencv plugin for foregroung/background image
    sequence segmentation, using one out of 3 algorithms.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=701421