[Bug 776047] opencv: add dewarp plugin

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=776047

Nicolas Dufresne (stormer) <nicolas at ndufresne.ca> changed:

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

--- Comment #4 from Nicolas Dufresne (stormer) <nicolas at ndufresne.ca> ---
Review of attachment 341897:
 --> (https://bugzilla.gnome.org/review?bug=776047&attachment=341897)

::: ext/opencv/gstdewarp.cpp
@@ +90,3 @@
+  static GType dewarp_display_mode_type = 0;
+  static const GEnumValue dewarp_display_mode[] = {
+    {GST_DEWARP_PANORAMA, "Single panorama image", "single panorama"},

We usually add "-" between words to prevent any ambiguity through using white
spaces. So single-panorama would be more consistent with the rest of the code.
It also make it so user don't need to add \" when setting this with string and
parse_launch.

@@ +92,3 @@
+    {GST_DEWARP_PANORAMA, "Single panorama image", "single panorama"},
+    {GST_DEWARP_DOUBLE_PANORAMA, "Dewarped image is splitted in two images "
+          "displayed one below the other", "double panorama"},

Same.

@@ +95,3 @@
+    {GST_DEWARP_QUAD_VIEW,
+          "Dewarped image is splitted in four images dysplayed as a quad
view",
+        "quad view"},

Same.

@@ +367,3 @@
+      break;
+  }
+  if (filter->need_map_update) {

For readability, it would benefit if you add some white lines between end of
scope and if (. It's not mandatory, but I think for this function it is
preferred.

@@ +371,3 @@
+  }
+  GST_OBJECT_UNLOCK (filter);
+  if (need_reconfigure) {

Same here, the unlock is a bit lost in the density.

@@ +435,3 @@
+  GST_DEBUG_OBJECT (filter,
+      "start update map out_width: %" G_GINT32_FORMAT " out height: %"
+      G_GINT32_FORMAT, out_width, out_height);

Some white lines here too, after the declaration block (before if),
before/after the trace, and later before for() etc.

@@ +480,3 @@
+    if (direction == GST_PAD_SINK) {
+      /* roundup is required to have integer results when we divide width,
height
+         for display mode different from GST_DEWARP_PANORAMA.

In GStreamer, pretty much all multilines comments are using the following
style:

/* Line1
 * Line2
 * ...
 */

@@ +501,3 @@
+      }
+      filter->in_width = in_width;
+      filter->in_height = in_height;

This is wrong. You should only update the new width/height at the final
negotiation step, which is set_caps(). Elements surrounding your filter can
always do caps queries and never actually change the caps. Simply transform the
caps, and chosen width/height will be given back to you, and will be in the
limited set you have transformed it to. This is what forces you to drop frames
in your transform function.

@@ +527,3 @@
+  GstDewarp *dewarp = GST_DEWARP (trans);
+
+  GstCaps *ret;

This function close to what we expect in term of white line. There is few
mistakes though.

@@ +537,3 @@
+  for (i = 0; i < gst_caps_get_size (ret); i++) {
+    GstStructure *structure = gst_caps_get_structure (ret, i);
+    if (gst_structure_get_int (structure, "width", &width) &&

I would add a white line before the if.

@@ +552,3 @@
+    GstCaps *intersection;
+    GST_DEBUG_OBJECT (dewarp, "Using filter caps %" GST_PTR_FORMAT,
+        filter_caps);

While lines before/after trace.

@@ +557,3 @@
+    gst_caps_unref (ret);
+    ret = intersection;
+    GST_DEBUG_OBJECT (dewarp, "Intersection %" GST_PTR_FORMAT, ret);

White line before trace.

@@ +560,3 @@
+  }
+  return ret;
+

Except here too, white line after } and no white line after return please.

@@ +593,3 @@
+      && img->height == filter->in_height
+      && outimg->width == filter->out_width
+      && outimg->height == filter->out_height) {

This is wrong, you should not drop data. This is caused by the bug in
transform_caps I have mentioned.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.