Created attachment 341898 [details]
test image1

this pipeline will output the dewarped image

gst-launch-1.0 filesrc location=/tmp/dewarp/800x600.jpg ! jpegdec ! videoconvert ! dewarp outer-radius=0.36 ! videoconvert ! jpegenc ! filesink location=/tmp/dewarp/800x600_dewarped.jpg

Created attachment 341899 [details]
test image2

this pipeline will output the dewarped image

gst-launch-1.0 filesrc location=/tmp/dewarp/1920x1080.jpg ! jpegdec ! videoconvert ! dewarp outer-radius=0.28 x-remap-correction=1.4 ! videoconvert ! jpegenc ! filesink location=/tmp/dewarp/1920x1080_dewarped.jpg

Created attachment 341900 [details]
test image3

this pipeline will output the dewarped image

gst-launch-1.0 filesrc location=/tmp/dewarp/2560x2048.jpg ! jpegdec ! videoconvert ! dewarp outer-radius=0.36 ! videoconvert ! jpegenc ! filesink location=/tmp/dewarp/2560x2048_dewarped.jpg

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.

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

(In reply to Nicolas Dufresne (stormer) from comment #4)
> Review of attachment 341897 [details] [review] [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

Created attachment 341941 [details] [review]
updated patch

updated as per comments, for now I don't removed the defensive code in transform frame function and I added a new variable to store the pad sink dimensions

Review of attachment 341941 [details] [review]:

Look good now, let's carry on future enhancement (if needed) in-tree.

Note, I made a small edit so it picked by the meson build system too.


as 8f674a363967212c23642c4cc53016b6e3a6b48c