[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 #250442|none                        |needs-work
             status|                            |

--- Comment #12 from Sebastian Dröge (slomo) <slomo at circular-chaos.org> 2013-08-19 10:11:13 UTC ---
Review of attachment 250442:
 --> (https://bugzilla.gnome.org/review?bug=704760&attachment=250442)

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

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