[Bug 776047] opencv: add dewarp plugin

[GStreamer (GNOME Bugzilla)]

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

--- Comment #5 from Nicola <lists at svrinformatica.it> ---
(In reply to Nicolas Dufresne (stormer) from comment #4)
> Review of attachment 341897 [details] [review]:
> 
> ::: 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.

ok

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

ok

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

ok

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

this is intented, if direction is GST_PAD_SINK I calcuate dimension with this
formula:

GST_ROUND_UP_8 ((gint) ((2.0 * G_PI) * ((r2 + r1) / 2.0)));

as you can see this is a double rounded to int so we loss precision,

when the direction is GST_PAD_SRC, gstreamer pass the rounded value and it
expects the original width/height, if I simply use the reverse formula I don't
get the same value and caps negotiation fails, maybe I can use a different name
to make clear what this width,height are, do you have other suggestions?

please advise

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

ok

> 
> @@ +557,3 @@
> +    gst_caps_unref (ret);
> +    ret = intersection;
> +    GST_DEBUG_OBJECT (dewarp, "Intersection %" GST_PTR_FORMAT, ret);
> 
> White line before trace.
> 

ok

> @@ +560,3 @@
> +  }
> +  return ret;
> +
> 
> Except here too, white line after } and no white line after return please.
> 

ok

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

This is just defensive programming, I never see the log in the else clause,
this code could prevent a segfault for unexpected cases, I can remove it if you
prefer

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