[Bug 704760] opencv: disparity-map calculation element

[GStreamer (bugzilla.gnome.org)]

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

--- Comment #2 from Miguel (elmiguelao) Casas-Sanchez <miguelecasassanchez at gmail.com> 2013-07-26 17:07:44 UTC ---
(In reply to comment #1)
> Review of attachment 249926 [details]:
> 
> ::: ext/opencv/gstdisparity.cpp
> @@ +161,3 @@
> +      {METHOD_SGBM, "Semi-global block matching algorithm", "sgbm"},
> +      {METHOD_VAR, "Variational matching algorithm", "svar"},
> +      {METHOD_GC, "Graph=cut based matching algorithm", "sgc"},
> 
> Should this be Graph=cut or Graph-cut?
> 
Graph-cut :)

> @@ +262,3 @@
> +  gst_pad_set_chain_function (filter->sinkpad_right,
> +      GST_DEBUG_FUNCPTR (gst_disparity_chain_right));
> +  GST_PAD_SET_PROXY_CAPS (filter->sinkpad_right);
> 
> I don't think this makes negotiation happen correctly. At the moment when one
> of the pads has caps, the other one has to return exactly these caps as a
> result of the CAPS query and can't accept anything else anymore. Apart from
> that it is PROXY_CAPS, yes. So implement a query function on both pads that
> does this and does not call gst_pad_query_default() for the CAPS query.
> 

Hmm I don't fully understand, I thought at the moment the negotiation would 
happen via sink_event, so perhaps I should modify that function so on second
call
it returns the format accepted in the first call, and keep the caps proxied?


> @@ +318,3 @@
> +
> +  switch (GST_EVENT_TYPE (event)) {
> +    case GST_EVENT_CAPS:
> 
> All this needs some blocking, the CAPS events will arrive from two different
> threads and can happen at the same time
> 

Added a mutex on both sides to make this a critical section. I guess
gst_disparity_init() is called before any event handling (rtfm for me?) ;)

> @@ +324,3 @@
> +      gst_video_info_from_caps (&info, caps);
> +
> +      GST_WARNING (" Negotiating caps via event (%s) %s",
> 
> Not a warning, and should be GST_WARNING_OBJECT(pad, ...) if anything :) Same
> for everything else, GST_DEBUG_OBJECT(), ...
> 
Corrected/ cleaned up.

> @@ +330,3 @@
> +      fs->width = info.width;
> +      fs->height = info.height;
> +      fs->actualChannels = info.finfo->n_components;
> 
> Maybe just store the GstVideoInfo in the instance struct instead of copying
> some of its part there
> 

GstVideoInfo has loads of fields including pointers to other structs, and since
I just need 3 fields instead I thought better to get them copied.

> @@ +348,3 @@
> +
> +      GST_DEBUG (" Negotiated caps via event (%s) %s",
> +          gst_pad_get_name (pad), gst_caps_to_string (caps));
> 
> GST_DEBUG_OBJECT (pad, "Negotiated caps %" GST_PTR_FORMAT, caps)
> 
> Otherwise you leak the object name and the caps string (alternatively "%s:%s",
> GST_DEBUG_PAD_NAME(pad))
Done.

> 
> @@ +403,3 @@
> +  if (fs->buffer_left) {
> +    GST_INFO (" right is busy, wait and hold");
> +    g_cond_wait (fs->cond, fs->lock);
> 
> Also here GST_DEBUG_OBJECT(pad, ...) like everywhere else :)
> 
Done.

> 
> So for the GCond handling there are two things missing. This can easily
> deadlock right now. First of all you should always put the waiting in a loop,
> i.e. while (fs->buffer_left) g_cond_wait().

The cond should keep the chain_{left,right} fully blocked until the other chain
arrives at
the rendezvous. What would be the situation in which a chain function blocked
at the cond 
variable would wake up _not_ being signalled by the other chain function?

> And then you need to unblock the
> waiting in GstElement::change_state() when going from PAUSED to READY before
> chaining up to the parent class' change_state implementation. You would for
> that introduce a new instance variable "flushing" that is set to FALSE in
> READY->PAUSED and to TRUE in PAUSED->READY (and afterwards you signal the
> GCond). If any of the chain functions sees that flushing is TRUE, it should
> return GST_FLOW_FLUSHING immediately. If after g_cond_wait() it is TRUE, return
> GST_FLOW_FLUSHING immediately too.

Ok, done this part.

> 
> @@ +493,3 @@
> +    cvCvtColor (fs->cvRGB_right, fs->cvGray_right, CV_RGB2GRAY);
> +    run_svar_iteration (fs);
> +    //cvNormalize(fs->cvGray_depth_map2, fs->cvGray_depth_map2, 0, 255,
> CV_MINMAX, NULL);
> 
> C99 comment
> 
Done.

> @@ +516,3 @@
> +
> +  ret = gst_pad_push (fs->srcpad, buffer);
> +  //gst_object_unref (fs);
> 
> C99 comment, and the unref is not necessary
> 
Done. Is the forgotten remaining of some debugging.

> ::: ext/opencv/gstdisparity.h
> @@ +50,3 @@
> +#include <highgui.h>            // includes highGUI definitions
> +#endif
> +#ifdef HAVE_OPENCV2_HIGHGUI_HIGHGUI_C_H
> 
> Don't you want configure checks for these two? And #include config.h?

Actually they are not needed :) The highgui and their checks are in place from
gstmotioncells.{h,cpp} times, so they can be -and have been- removed.

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