[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

--- Comment #4 from Sebastian Dröge <slomo at circular-chaos.org> 2013-06-06 13:07:27 UTC ---
(In reply to comment #2)
> (In reply to comment #1)
> > Review of attachment 245821 [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

> > @@ +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 :)

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