[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

Sebastian Dröge (slomo) <slomo> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #249926|none                        |needs-work
             status|                            |

--- Comment #1 from Sebastian Dröge (slomo) <slomo at circular-chaos.org> 2013-07-24 07:11:32 UTC ---
Review of attachment 249926:
 --> (https://bugzilla.gnome.org/review?bug=704760&attachment=249926)

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

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