[Bug 701421] opencv: add foreground/background segmentation element

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=701421
  GStreamer | gst-plugins-bad | 1.x

Miguel (elmiguelao) Casas-Sanchez <miguelecasassanchez> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |miguelecasassanchez at gmail.c
                   |                            |om

--- Comment #5 from Miguel (elmiguelao) Casas-Sanchez <miguelecasassanchez at gmail.com> 2013-06-09 15:27:01 UTC ---
(In reply to comment #4)
> (In reply to comment #2)
> > (In reply to comment #1)
> > > Review of attachment 245821 [details] [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.

-- 
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.