[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 #13 from Miguel (elmiguelao) Casas-Sanchez <miguelecasassanchez at gmail.com> 2013-08-19 14:46:01 UTC ---
(In reply to comment #12)
> Review of attachment 250442 [details]:
> 
> ::: 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 :)

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