After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 704760 - opencv: disparity-map calculation element
opencv: disparity-map calculation element
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
1.x
Other All
: Normal enhancement
: 1.1.4
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-07-23 16:59 UTC by Miguel Casas
Modified: 2013-08-23 10:02 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
1st full patch GstDisparity (31.96 KB, patch)
2013-07-23 16:59 UTC, Miguel Casas
needs-work Details | Review
1st delta over 1st patch, Sebastian's comments addressed. (10.62 KB, patch)
2013-07-26 17:08 UTC, Miguel Casas
needs-work Details | Review
2nd full patch GstDisparity (105.87 KB, patch)
2013-07-29 16:19 UTC, Miguel Casas
rejected Details | Review
3rd full patch GstDisparity (34.54 KB, patch)
2013-07-30 09:57 UTC, Miguel Casas
needs-work Details | Review
Addressing comments over 3rd full patch. (8.66 KB, patch)
2013-08-19 14:47 UTC, Miguel Casas
needs-work Details | Review
More addressing comments over 3rd full patch (3.60 KB, patch)
2013-08-22 12:28 UTC, Miguel Casas
reviewed Details | Review
4th full patch GstDisparity (35.15 KB, patch)
2013-08-22 13:21 UTC, Miguel Casas
none Details | Review
Squashed all commits in git format-patch mode (35.69 KB, patch)
2013-08-22 13:46 UTC, Miguel Casas
needs-work Details | Review
Squashed all commits in git format-patch mode (35.71 KB, patch)
2013-08-22 14:28 UTC, Miguel Casas
none Details | Review
Added return false when alloc query on the left pad. (855 bytes, patch)
2013-08-22 14:28 UTC, Miguel Casas
none Details | Review
Opencv disparity element in single patch compiling in opencv 2.4.3 (35.77 KB, patch)
2013-08-23 09:41 UTC, Miguel Casas
committed Details | Review

Description Miguel Casas 2013-07-23 16:59:59 UTC
Created attachment 249926 [details] [review]
1st full patch GstDisparity

"This element computes a disparity map from two stereo images, meaning each one coming from a different camera, both looking at the same scene and relatively close to each other - more on this below. The disparity map is a proxy of the depth of a scene as seen from the camera."

This element has an interesting twist: same as videomeasure, it is a muxer that normally needs to wait until both frames are present on its input pads before carrying out the stereo mapping. This is, strongly inspired by videomeasure, achieved via a mutex + condition variable + a buffer being NULL or not.
Comment 1 Sebastian Dröge (slomo) 2013-07-24 07:11:32 UTC
Review of attachment 249926 [details] [review]:

::: 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?

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

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

@@ +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(), ...

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

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

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


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

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

@@ +516,3 @@
+
+  ret = gst_pad_push (fs->srcpad, buffer);
+  //gst_object_unref (fs);

C99 comment, and the unref is not necessary

::: 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?
Comment 2 Miguel Casas 2013-07-26 17:07:44 UTC
(In reply to comment #1)
> Review of attachment 249926 [details] [review]:
> 
> ::: 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.
Comment 3 Miguel Casas 2013-07-26 17:08:57 UTC
Created attachment 250212 [details] [review]
1st delta over 1st patch, Sebastian's comments addressed.

git patch-format over the first patch.
Comment 4 Sebastian Dröge (slomo) 2013-07-28 15:16:19 UTC
(In reply to comment #2)
> (In reply to comment #1)
> > Review of attachment 249926 [details] [review] [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?

Yes, that's what videomixer does for example. Proxy the CAPS query always, unless a CAPS event arrived already. In that case return the caps that were set previously.
 
> > @@ +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?) ;)

Yes, that's called before anybody even has a pointer to the instance :)
 
 
> > 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?

Flushes and the pipeline being shut down.
Comment 5 Sebastian Dröge (slomo) 2013-07-28 15:21:56 UTC
Review of attachment 250212 [details] [review]:

You'll have to implement a query function on the sinkpads, and there do the CAPS query handling as described above (proxy if no caps set, set caps if caps are set)

::: ext/opencv/gstdisparity.cpp
@@ +185,2 @@
 G_DEFINE_TYPE (GstDisparity, gst_disparity, GST_TYPE_ELEMENT);
+static GstElementClass *parent_class = NULL;

Not needed, G_DEFINE_TYPE() creates and initializes a gst_disparity_parent_class variable for you

@@ +231,3 @@
 
+  element_class->change_state = gst_disparity_change_state;
+  parent_class = (GstElementClass *) g_type_class_peek_parent (klass);

It also does the g_type_class_peek_parent() for you

@@ +321,3 @@
+    case GST_STATE_CHANGE_PAUSED_TO_READY:
+      fs->flushing = true;
+      g_cond_signal (fs->cond);

You have to lock the mutex when signalling a GCond. On some weird platforms this is racy otherwise. Also broadcast just in case :)

@@ +440,3 @@
   fs = GST_DISPARITY (parent);
+  GST_DEBUG_OBJECT (pad, "processing frame from left");
+  if (fs->flushing)

Protect all accesses to flushing with the mutex

@@ +448,3 @@
+    GST_DEBUG_OBJECT (pad, " right is free, continuing");
+    if (fs->flushing)
+      return GST_FLOW_FLUSHING;

while (fs->buffer_left) { g_cond_wait(); if (flushing) return GST_FLOW_FLUSHING; }

Also unlock before return

@@ +483,3 @@
+    GST_DEBUG_OBJECT (pad, " left has just provided a frame, continuing");
+    if (fs->flushing)
+      return GST_FLOW_FLUSHING;

Same as above here
Comment 6 Miguel Casas 2013-07-29 16:19:23 UTC
Created attachment 250380 [details] [review]
2nd full patch GstDisparity

Addressed all issues regarding locking and usage of g_cond_lock. Also removed the not needed parent_class.

BUT: Still don't see the caps negotiation clearly :(
 I tried forcing the caps on both src and the other sink when there is a sink_event, but then the negotiation doesn't work out. Help! :'(

Uploaded a new patch since the code has changed a bit.
Comment 7 Sebastian Dröge (slomo) 2013-07-30 08:53:47 UTC
Comment on attachment 250380 [details] [review]
2nd full patch GstDisparity

This doesn't look like the patch you wanted to attach :)
Comment 8 Sebastian Dröge (slomo) 2013-07-30 08:57:08 UTC
(In reply to comment #6)

> BUT: Still don't see the caps negotiation clearly :(
>  I tried forcing the caps on both src and the other sink when there is a
> sink_event, but then the negotiation doesn't work out. Help! :'(

What you would do is something like the following:

sink_query(CAPS):
 - If !gst_pad_has_current_caps(srcpad):
     return gst_pad_get_template_caps(pad);
 - else:
     return gst_pad_get_current_caps(srcpad);

sink_event(CAPS):
  - if !gst_pad_has_current_caps(srcpad):
      gst_pad_set_caps(srcpad, caps);
  - else if !gst_caps_is_equal(gst_pad_get_current_caps(srcpad), caps):
      fail();
  - else:
      succeed();
Comment 9 Miguel Casas 2013-07-30 09:57:22 UTC
Created attachment 250442 [details] [review]
3rd full patch GstDisparity

Addressed from before all the comments on adding critical section to manipulate state changes etc.

And also now adapted the caps negotiation following Sebastian's comments: added sink_query. Pls note that I moved the OpenCV-specific allocations into another function initialise_disparity, so the handle_sink_event and (new) handle_query functions are easy to spot.

(Sorry about the messed up previous patch :(   )
Comment 10 Miguel Casas 2013-08-12 13:57:07 UTC
ping :)
Comment 11 Miguel Casas 2013-08-19 09:06:26 UTC
ping2?
Comment 12 Sebastian Dröge (slomo) 2013-08-19 10:11:13 UTC
Review of attachment 250442 [details] [review]:

::: ext/opencv/gstdisparity.cpp
@@ +324,3 @@
+  GstStateChangeReturn ret = GST_STATE_CHANGE_SUCCESS;
+  GstDisparity *fs = GST_DISPARITY (element);
+  g_mutex_lock (fs->lock);

Better take the lock just around the lines that need it, someone might add something else to the switch statement later

@@ +342,3 @@
+      transition);
+
+  g_mutex_lock (fs->lock);

Same here

@@ +373,3 @@
+      /* Critical section since both pads handle event sinking simultaneously */
+      g_mutex_lock (fs->lock);
+      GstVideoInfo info;

Put all variable declarations at the beginning of a block

@@ +387,3 @@
+            "format", G_TYPE_STRING, "RGB",
+            "width", G_TYPE_INT, fs->width,
+            "height", G_TYPE_INT, fs->height, NULL);

Use gst_video_info_to_caps() to create raw video caps

@@ +396,3 @@
+
+      GST_INFO_OBJECT (pad, " Negotiated caps via event, %" GST_PTR_FORMAT,
+          caps);

Put ret in the debug output too

@@ +402,3 @@
+      break;
+  }
+  ret = gst_pad_event_default (pad, parent, event);

You probably don't want to forward the event if above the caps equality check has failed

@@ +418,3 @@
+      else
+        gst_query_set_caps_result (query,
+            gst_pad_get_current_caps (fs->srcpad));

This is leaking the return values of gst_pad_get_pad_template_caps() and gst_pad_get_current_caps(), also needs locking

@@ +448,3 @@
+  filter = GST_DISPARITY (object);
+  gst_disparity_release_all_pointers (filter);
+  filter->width = filter->height = filter->actualChannels = 0;

Not necessary to reset these to 0 in finalize

@@ +469,3 @@
+  GST_DEBUG_OBJECT (pad, "processing frame from left");
+  if (fs->flushing)
+    return GST_FLOW_FLUSHING;

Check flushing after taking the mutex

@@ +476,3 @@
+    GST_DEBUG_OBJECT (pad, " right is free, continuing");
+    if (fs->flushing)
+      return GST_FLOW_FLUSHING;

Returning without unlocking the mutex

@@ +481,3 @@
+
+  if (!gst_buffer_map (buffer, &info, (GstMapFlags) GST_MAP_READWRITE)) {
+    return GST_FLOW_ERROR;

Returning without unlocking the mutex

@@ +504,3 @@
+  GST_DEBUG_OBJECT (pad, "processing frame from right");
+  if (fs->flushing)
+    return GST_FLOW_FLUSHING;

Check flushing after taking the mutex

@@ +511,3 @@
+    GST_DEBUG_OBJECT (pad, " left has just provided a frame, continuing");
+    if (fs->flushing)
+      return GST_FLOW_FLUSHING;

Returning without unlocking the mutex

@@ +587,3 @@
+
+  GST_DEBUG_OBJECT (pad, " right has finished");
+  fs->buffer_left = NULL;

Unmap and unref the left buffer

@@ +591,3 @@
+  g_mutex_unlock (fs->lock);
+
+  ret = gst_pad_push (fs->srcpad, buffer);

As the right buffer is always the one that is forwarded it would make sense to proxy the allocation query from the right sinkpad to the srcpad
Comment 13 Miguel Casas 2013-08-19 14:46:01 UTC
(In reply to comment #12)
> Review of attachment 250442 [details] [review]:
> 
> ::: ext/opencv/gstdisparity.cpp
> @@ +324,3 @@
> +  GstStateChangeReturn ret = GST_STATE_CHANGE_SUCCESS;
> +  GstDisparity *fs = GST_DISPARITY (element);
> +  g_mutex_lock (fs->lock);
> 
> Better take the lock just around the lines that need it, someone might add
> something else to the switch statement later

Done.

> 
> @@ +342,3 @@
> +      transition);
> +
> +  g_mutex_lock (fs->lock);
> 
> Same here

Done.

> 
> @@ +373,3 @@
> +      /* Critical section since both pads handle event sinking simultaneously
> */
> +      g_mutex_lock (fs->lock);
> +      GstVideoInfo info;
> 
> Put all variable declarations at the beginning of a block

Done.

> 
> @@ +387,3 @@
> +            "format", G_TYPE_STRING, "RGB",
> +            "width", G_TYPE_INT, fs->width,
> +            "height", G_TYPE_INT, fs->height, NULL);
> 
> Use gst_video_info_to_caps() to create raw video caps
> 

Done.

> @@ +396,3 @@
> +
> +      GST_INFO_OBJECT (pad, " Negotiated caps via event, %" GST_PTR_FORMAT,
> +          caps);
> 
> Put ret in the debug output too
> 

Done.

> @@ +402,3 @@
> +      break;
> +  }
> +  ret = gst_pad_event_default (pad, parent, event);
> 
> You probably don't want to forward the event if above the caps equality check
> has failed
> 

Done.

> @@ +418,3 @@
> +      else
> +        gst_query_set_caps_result (query,
> +            gst_pad_get_current_caps (fs->srcpad));
> 
> This is leaking the return values of gst_pad_get_pad_template_caps() and
> gst_pad_get_current_caps(), also needs locking
> 

Hopefully done :)

> @@ +448,3 @@
> +  filter = GST_DISPARITY (object);
> +  gst_disparity_release_all_pointers (filter);
> +  filter->width = filter->height = filter->actualChannels = 0;
> 
> Not necessary to reset these to 0 in finalize

Done.

> 
> @@ +469,3 @@
> +  GST_DEBUG_OBJECT (pad, "processing frame from left");
> +  if (fs->flushing)
> +    return GST_FLOW_FLUSHING;
> 
> Check flushing after taking the mutex
> 

Done.

> @@ +476,3 @@
> +    GST_DEBUG_OBJECT (pad, " right is free, continuing");
> +    if (fs->flushing)
> +      return GST_FLOW_FLUSHING;
> 
> Returning without unlocking the mutex

Done.

> 
> @@ +481,3 @@
> +
> +  if (!gst_buffer_map (buffer, &info, (GstMapFlags) GST_MAP_READWRITE)) {
> +    return GST_FLOW_ERROR;
> 
> Returning without unlocking the mutex

Done.

> 
> @@ +504,3 @@
> +  GST_DEBUG_OBJECT (pad, "processing frame from right");
> +  if (fs->flushing)
> +    return GST_FLOW_FLUSHING;
> 
> Check flushing after taking the mutex

Done.

> 
> @@ +511,3 @@
> +    GST_DEBUG_OBJECT (pad, " left has just provided a frame, continuing");
> +    if (fs->flushing)
> +      return GST_FLOW_FLUSHING;
> 
> Returning without unlocking the mutex

Done.

> 
> @@ +587,3 @@
> +
> +  GST_DEBUG_OBJECT (pad, " right has finished");
> +  fs->buffer_left = NULL;
> 
> Unmap and unref the left buffer

Done unmapping but not unrefing: fs->buffer_left is just a mapping of an already unreferenced incoming buffer (in chain_left), after the unmap, both will get automatically unreferenced and destroyed. 

> 
> @@ +591,3 @@
> +  g_mutex_unlock (fs->lock);
> +
> +  ret = gst_pad_push (fs->srcpad, buffer);
> 
> As the right buffer is always the one that is forwarded it would make sense to
> proxy the allocation query from the right sinkpad to the srcpad

I don't really understand that :)
Comment 14 Miguel Casas 2013-08-19 14:47:30 UTC
Created attachment 252234 [details] [review]
Addressing comments over 3rd full patch.

Incremental patch addressing (most of) Sebastian's comments.
Comment 15 Sebastian Dröge (slomo) 2013-08-19 15:13:36 UTC
Review of attachment 252234 [details] [review]:

::: ext/opencv/gstdisparity.cpp
@@ +480,2 @@
   if (fs->flushing)
     return GST_FLOW_FLUSHING;

Return without unlocking the mutex

@@ +517,2 @@
   if (fs->flushing)
     return GST_FLOW_FLUSHING;

Return without unlocking the mutex

@@ +601,2 @@
   GST_DEBUG_OBJECT (pad, " right has finished");
+  gst_buffer_unmap (fs->buffer_left, &info);

You must not unref the buffer in chain_left, but unref it here after unmapping
Comment 16 Sebastian Dröge (slomo) 2013-08-19 15:15:24 UTC
(In reply to comment #13)

> > @@ +591,3 @@
> > +  g_mutex_unlock (fs->lock);
> > +
> > +  ret = gst_pad_push (fs->srcpad, buffer);
> > 
> > As the right buffer is always the one that is forwarded it would make sense to
> > proxy the allocation query from the right sinkpad to the srcpad
> 
> I don't really understand that :)

In the query function of the right sinkpad, when you receive an ALLOCATION query, you should forward this query to the srcpad. But only for the right sinkpad (as that is the one that gets the buffers which are forwarded downstream). That way upstream of the right sinkpad will ideally allocate buffers via a buffer pool that is provided by the sink.
Comment 17 Miguel Casas 2013-08-22 07:41:55 UTC
(In reply to comment #15)
> Review of attachment 252234 [details] [review]:
> 
> ::: ext/opencv/gstdisparity.cpp
> @@ +480,2 @@
>    if (fs->flushing)
>      return GST_FLOW_FLUSHING;
> 
> Return without unlocking the mutex
> 
> @@ +517,2 @@
>    if (fs->flushing)
>      return GST_FLOW_FLUSHING;
> 
> Return without unlocking the mutex
> 
> @@ +601,2 @@
>    GST_DEBUG_OBJECT (pad, " right has finished");
> +  gst_buffer_unmap (fs->buffer_left, &info);
> 
> You must not unref the buffer in chain_left, but unref it here after unmapping

Got it! - done those 3
Comment 18 Miguel Casas 2013-08-22 12:26:26 UTC
(In reply to comment #16)
> (In reply to comment #13)
> 
> > > @@ +591,3 @@
> > > +  g_mutex_unlock (fs->lock);
> > > +
> > > +  ret = gst_pad_push (fs->srcpad, buffer);
> > > 
> > > As the right buffer is always the one that is forwarded it would make sense to
> > > proxy the allocation query from the right sinkpad to the srcpad
> > 
> > I don't really understand that :)
> 
> In the query function of the right sinkpad, when you receive an ALLOCATION
> query, you should forward this query to the srcpad. But only for the right
> sinkpad (as that is the one that gets the buffers which are forwarded
> downstream). That way upstream of the right sinkpad will ideally allocate
> buffers via a buffer pool that is provided by the sink.

Hopefully done as well :)
Comment 19 Miguel Casas 2013-08-22 12:28:31 UTC
Created attachment 252725 [details] [review]
More addressing comments over 3rd full patch

Addressed some missing return-without-releasing mutex, corrected buffer unref position, and hopefully added GST_QUERY_ALLOCATION correctly.
Comment 20 Sebastian Dröge (slomo) 2013-08-22 13:15:16 UTC
Review of attachment 252725 [details] [review]:

::: ext/opencv/gstdisparity.cpp
@@ +433,3 @@
+    case GST_QUERY_ALLOCATION:
+      if (pad == fs->sinkpad_right)
+        ret = gst_pad_query_default (fs->srcpad, parent, query);

Almost, gst_pad_peer_query(srcpad, ...)
Comment 21 Miguel Casas 2013-08-22 13:21:47 UTC
Created attachment 252731 [details] [review]
4th full patch GstDisparity

Using gst_pad_peer_query(...)  (line 435)

Merged into one big patch since it seems that is converging.
Comment 22 Sebastian Dröge (slomo) 2013-08-22 13:27:22 UTC
Does not compile here after applying all patches (also can you make a combined patch with "git format-patch -1" after committing everything locally?):


  CXX      libgstopencv_la-MotionCells.lo
In file included from gstdisparity.cpp:125:0:
gstdisparity.h:96:3: error: 'CvStereoGCState' does not name a type
   CvStereoGCState *sgc;         /* This is a C implementation */
   ^
gstdisparity.cpp: In function 'void gst_disparity_init(GstDisparity*)':
gstdisparity.cpp:283:31: error: 'g_mutex_new' was not declared in this scope
   filter->lock = g_mutex_new ();
                               ^
gstdisparity.cpp:284:30: error: 'g_cond_new' was not declared in this scope
   filter->cond = g_cond_new ();
                              ^
gstdisparity.cpp: In function 'void gst_disparity_finalize(GObject*)':
gstdisparity.cpp:470:28: error: 'g_cond_free' was not declared in this scope
   g_cond_free (filter->cond);
                            ^
gstdisparity.cpp:471:29: error: 'g_mutex_free' was not declared in this scope
   g_mutex_free (filter->lock);
                             ^
gstdisparity.cpp: In function 'int initialise_sbm(GstDisparity*)':
gstdisparity.cpp:687:11: error: 'GstDisparity' has no member named 'sgc'
   filter->sgc = cvCreateStereoGCState (16, 2);
           ^
gstdisparity.cpp:687:45: error: 'cvCreateStereoGCState' was not declared in this scope
   filter->sgc = cvCreateStereoGCState (16, 2);
                                             ^
gstdisparity.cpp:730:11: error: 'GstDisparity' has no member named 'sgc'
   filter->sgc->Ithreshold = 5;
           ^
gstdisparity.cpp:731:11: error: 'GstDisparity' has no member named 'sgc'
   filter->sgc->interactionRadius = 1;
           ^
gstdisparity.cpp:732:11: error: 'GstDisparity' has no member named 'sgc'
   filter->sgc->occlusionCost = 10000;
           ^
gstdisparity.cpp:733:11: error: 'GstDisparity' has no member named 'sgc'
   filter->sgc->minDisparity = 0;
           ^
gstdisparity.cpp:734:11: error: 'GstDisparity' has no member named 'sgc'
   filter->sgc->numberOfDisparities = 16;        /* Coming from constructor too */
           ^
gstdisparity.cpp:735:11: error: 'GstDisparity' has no member named 'sgc'
   filter->sgc->maxIters = 1;    /* Coming from constructor too */
           ^
gstdisparity.cpp: In function 'int run_sgc_iteration(GstDisparity*)':
gstdisparity.cpp:775:44: error: 'GstDisparity' has no member named 'sgc'
       filter->cvGray_depth_map1_2, filter->sgc, 0);
                                            ^
gstdisparity.cpp:775:50: error: 'cvFindStereoCorrespondenceGC' was not declared in this scope
       filter->cvGray_depth_map1_2, filter->sgc, 0);
                                                  ^
Comment 23 Miguel Casas 2013-08-22 13:42:33 UTC
(In reply to comment #22)
> Does not compile here after applying all patches (also can you make a combined
> patch with "git format-patch -1" after committing everything locally?):
> 
> 
>   CXX      libgstopencv_la-MotionCells.lo
> In file included from gstdisparity.cpp:125:0:
> gstdisparity.h:96:3: error: 'CvStereoGCState' does not name a type
>    CvStereoGCState *sgc;         /* This is a C implementation */
>    ^
> gstdisparity.cpp: In function 'void gst_disparity_init(GstDisparity*)':
> gstdisparity.cpp:283:31: error: 'g_mutex_new' was not declared in this scope
>    filter->lock = g_mutex_new ();
>                                ^
> gstdisparity.cpp:284:30: error: 'g_cond_new' was not declared in this scope
>    filter->cond = g_cond_new ();
>                               ^
> gstdisparity.cpp: In function 'void gst_disparity_finalize(GObject*)':
> gstdisparity.cpp:470:28: error: 'g_cond_free' was not declared in this scope
>    g_cond_free (filter->cond);
>                             ^
> gstdisparity.cpp:471:29: error: 'g_mutex_free' was not declared in this scope
>    g_mutex_free (filter->lock);
>                              ^
> gstdisparity.cpp: In function 'int initialise_sbm(GstDisparity*)':
> gstdisparity.cpp:687:11: error: 'GstDisparity' has no member named 'sgc'
>    filter->sgc = cvCreateStereoGCState (16, 2);
>            ^
> gstdisparity.cpp:687:45: error: 'cvCreateStereoGCState' was not declared in
> this scope
>    filter->sgc = cvCreateStereoGCState (16, 2);
>                                              ^
> gstdisparity.cpp:730:11: error: 'GstDisparity' has no member named 'sgc'
>    filter->sgc->Ithreshold = 5;
>            ^
> gstdisparity.cpp:731:11: error: 'GstDisparity' has no member named 'sgc'
>    filter->sgc->interactionRadius = 1;
>            ^
> gstdisparity.cpp:732:11: error: 'GstDisparity' has no member named 'sgc'
>    filter->sgc->occlusionCost = 10000;
>            ^
> gstdisparity.cpp:733:11: error: 'GstDisparity' has no member named 'sgc'
>    filter->sgc->minDisparity = 0;
>            ^
> gstdisparity.cpp:734:11: error: 'GstDisparity' has no member named 'sgc'
>    filter->sgc->numberOfDisparities = 16;        /* Coming from constructor too
> */
>            ^
> gstdisparity.cpp:735:11: error: 'GstDisparity' has no member named 'sgc'
>    filter->sgc->maxIters = 1;    /* Coming from constructor too */
>            ^
> gstdisparity.cpp: In function 'int run_sgc_iteration(GstDisparity*)':
> gstdisparity.cpp:775:44: error: 'GstDisparity' has no member named 'sgc'
>        filter->cvGray_depth_map1_2, filter->sgc, 0);
>                                             ^
> gstdisparity.cpp:775:50: error: 'cvFindStereoCorrespondenceGC' was not declared
> in this scope
>        filter->cvGray_depth_map1_2, filter->sgc, 0);
>                                                   ^

Perhaps OpenCV has no legacy enabled and then it fails to find CvStereoGCState.

Puzzles me that it complains about g_mutex_new() etc ??
Comment 24 Miguel Casas 2013-08-22 13:46:19 UTC
Created attachment 252747 [details] [review]
Squashed all commits in git format-patch mode
Comment 25 Miguel Casas 2013-08-22 14:00:45 UTC
Perhaps package libopencv-legacy-dev should be enabled? 
The configure.ac asks for opencv "generic" but then checks for highgui.h which is part of another package (libopencv-highgui-dev). Perhaps I could add a legacy check to the global configure.ac?
Comment 26 Sebastian Dröge (slomo) 2013-08-22 14:17:23 UTC
I have libopencv-legacy-dev installed, the missing type is defined in opencv2/legacy/legacy.hpp
Comment 27 Sebastian Dröge (slomo) 2013-08-22 14:20:10 UTC
Review of attachment 252747 [details] [review]:

Please choose a better commit message :)

::: ext/opencv/gstdisparity.cpp
@@ +433,3 @@
+    case GST_QUERY_ALLOCATION:
+      if (pad == fs->sinkpad_right)
+        ret = gst_pad_peer_query (fs->srcpad, query);

else ret = FALSE;
Comment 28 Miguel Casas 2013-08-22 14:28:07 UTC
Created attachment 252758 [details] [review]
Squashed all commits in git format-patch mode

With changed comment ;)
Comment 29 Miguel Casas 2013-08-22 14:28:44 UTC
Created attachment 252759 [details] [review]
Added return false when alloc query on the left pad.
Comment 30 Miguel Casas 2013-08-22 14:30:15 UTC
(In reply to comment #26)
> I have libopencv-legacy-dev installed, the missing type is defined in
> opencv2/legacy/legacy.hpp

Yes and legacy.hpp is included via cvaux.h (or similar) which comes from cv.h. OpenCV packaging is cumbersome, it goes partially into /usr/include/opencv and part under /usr/include/opencv2 and not all files (specially .h ones) are packaged, hence the mistery inclusions...
Comment 31 Sebastian Dröge (slomo) 2013-08-22 14:36:54 UTC
Hmm, so how do we make this work properly now? :) Is there a non-legacy replacement for this type?
Comment 32 Miguel Casas 2013-08-22 14:46:09 UTC
I have a bit of a mess on my side, but perhaps the key would be to add to gstdisparity.h, after the include <cv.h> :

#include <legacy/legacy.hpp>

cv.h comes from /usr/include/opencv and legacy/legacy from /usr/include/opencv2 so who knows how the coexistence would be right now ...

I can/will try cleaning up my system to work from packages but will take me a while - just saying ;)
Comment 33 Miguel Casas 2013-08-22 14:47:00 UTC
Actually I meant

#include <opencv2/legacy/legacy.hpp>
Comment 34 Miguel Casas 2013-08-23 07:49:54 UTC
Hum, I removed my OpenCV from source and installed from precise packages. I had installed:

sudo aptitude install libopencv-dev libopencv-legacy-dev libcv-dev libcvaux-dev libhighgui-dev

and I compiled ok without any added legacy.hpp inclusions. 

CvStereoGCState is defined in opencv2/calib3d/calib3d.hpp which is pulled in directly from cv.h , so legacy is actually not needed, my mistake before, sorry, calib3d.hpp is owned by package libopencv-calib3d-dev .
Comment 35 Sebastian Dröge (slomo) 2013-08-23 08:30:40 UTC
I'm using opencv 2.4.5 and there it is defined in opencv2/legacy/legacy.hpp only. opencv2/calib3d/calib3d.hpp defines CvStereoBMState though which looks like it is doing more or less the same
Comment 36 Miguel Casas 2013-08-23 09:41:24 UTC
Created attachment 252825 [details] [review]
Opencv disparity element in single patch compiling in opencv 2.4.3

Added #include <opencv2/legacy/legacy.hpp> which seems to solve the OpenCV 2.4.3+ problem with CvStereoGCState and does not have troubles with older OpenCV versions.
Comment 37 Sebastian Dröge (slomo) 2013-08-23 10:02:09 UTC
commit 5e25d41b84b61d78e6e48b4a78a253f3ec6ab5ed
Author: Sebastian Dröge <slomo@circular-chaos.org>
Date:   Fri Aug 23 12:01:07 2013 +0200

    opencv: Port to non-deprecated GMutex/GCond API

commit fbb5dd38c7d454fec3ece29493ac38b812e9106d
Author: Miguel Casas-Sanchez <miguelecasassanchez@gmail.com>
Date:   Fri Aug 23 11:38:04 2013 +0200

    opencv: Add disparity-map calculation element
    
    https://bugzilla.gnome.org/show_bug.cgi?id=704760