Created attachment 287122 [details] [review]
lanczos method for NV format

Tested with same and different aspect-ratios and different submethods. It is working fine. Please review the patch

Hello Ravi,

In the commit message could you please have a description of what the patch does?
Commit messages are like so

"area: summary

longer description

url to bugzilla"

Could you provide me a link to the spec of NV12 and NV21 for some quick checking and testing?

Besides those two little things it looks good :)

Thanks

Created attachment 287133 [details] [review]
lanczos method for NV formats

I used following pipelines to check:
gst-launch-1.0 videotestsrc ! "video/x-raw,format={NV12}, width=320,height=240" ! videoscale method=lanczos ! "video/x-raw,format={NV12}, width=640,height=400" ! videoconvert ! ximagesink

gst-launch-1.0 videotestsrc ! "video/x-raw,format={NV21}, width=320,height=240" ! videoscale method=lanczos ! "video/x-raw,format={NV21}, width=640,height=480" ! videoconvert ! ximagesink

couple of references I came across-
http://www.fourcc.org/yuv.php#NV12
https://wiki.videolan.org/YUV/#Semi-planar

Testing and cleaning the commit lines that just add or remove empty lines.

Comment on attachment 287133 [details] [review]
lanczos method for NV formats

Merged. Great work.

Review of attachment 287133 [details] [review]:

This is a lot of copy paste to my taste but seems to respect the existing code style. Ravi or Luis, could you please provide a patch to fix the review left over.

::: gst/videoscale/vs_lanczos.c
@@ +220,3 @@
+vs_image_scale_lanczos_NV_int16 (const VSImage * dest, const VSImage * src,
+    uint8_t * tmpbuf, double sharpness, gboolean dither, double a,
+    double sharpen);

Style: type and declaration should be on same line.

@@ +753,3 @@
+  const tap_type *tapsline; \
+  int offset; \
+  if (_shift > 0) offset = (1<<_shift)>>1; \

Style: Space before after operators

@@ +756,3 @@
+  else offset = 0; \
+  for (i = 0; i < n; i++) { \
+    srcline = src + 2*offsets[i]; \

Style: Space before after operators

@@ +765,3 @@
+    } \
+    dest[i*2+0] = (sum1 + offset) >> _shift; \
+    dest[i*2+1] = (sum2 + offset) >> _shift; \

I know this is copy pasted, but doing multiplication in index is far from ideal for performance. Aslo, you compute k*2 and i*2 twice. (Note, k * 2 == k << 1). There is also style issues with operators missing space in between.

@@ +898,2 @@
   } \
 }

Normally we try and split the unrelated style fix from our patches.

Code should be written primarily for clarity. The compiler will optimise i*2 to a bitshift (hopefully), and it can also avoid double computation of i*2 if that makes sense, and we can let the compiler decide that IMHO.

Created attachment 287159 [details] [review]
fixes as suggested by Nicolas

Thanks Nicolas. They are good suggestions.

Should I split the style changes into a second patch or have it all in this one? Currently it includes the fixes to avoid recalculating the same value multiple times.

Like so:
-      sum1 += srcline[k*4+0] * tapsline[k]; \
-      sum2 += srcline[k*4+1] * tapsline[k]; \
-      sum3 += srcline[k*4+2] * tapsline[k]; \
-      sum4 += srcline[k*4+3] * tapsline[k]; \
+      l = k * 4; \
+      sum1 += srcline[l] * tapsline[k]; \
+      sum2 += srcline[l + 1] * tapsline[k]; \
+      sum3 += srcline[l + 2] * tapsline[k]; \
+      sum4 += srcline[l + 3] * tapsline[k]; \

And also a few style fixes.

As Tim mention, we can hope that the compiler will optimize the recalculation (I've hit cases where the compiler was not doing so, so I'm careful in color transformation code these days), but I'd really like to see a patch for the style.

Review of attachment 287159 [details] [review]:

::: gst/videoscale/vs_lanczos.c
@@ +710,3 @@
 { \
+  int i, j; \
+  int k, l; \

I agree with Tim the this affects the readability a bit.

Created attachment 287161 [details] [review]
Declaring new variables in their own lines.

Review of attachment 287161 [details] [review]:

Ok, let's take this one.

Comment on attachment 287161 [details] [review]
Declaring new variables in their own lines.

Merged