Re: [FFmpeg-devel] [PATCH] libavfilter: Add OpenCL convolution filter v0.1

[Mark Thompson]

On 14/03/18 09:43, Danil Iashchenko wrote:
> Behaves like the existing convolution filter, except working on OpenCL
> hardware frames.
> Takes exactly the same options: 4 convolution matrices, 4 rdiv values, 4 bias 
> values.
> If not specified, default parameters are applied.
> Matrices can be different sizes.
> 
> NEW IN THIS PATCH:
> -fixed bug, if matrices have different dims.
> -renamed some variables due to readability.
> 
> filter applies:
> matrix0, rdiv0, bias0 to image plane0.
> matrix1, rdiv1, bias1 to image plane1.
> matrix2, rdiv2, bias2 to image plane2.
> matrix3, rdiv3, bias3 to image plane3.
> 
> About Kernel parameters:
> dst - destination image
> src - source image
> coef_matrices_dims - stores dimensions of matrix{0..3} consecutively one 
> after the other
> coef_matrices - stores matrices{0..3} consecutively one after the other
> rdivs - stores rdiv{0..3} parameters consecutively one after the other
> biases - stores bias{0..3} parameters consecutively one after the other
> 
> About sscanf. I had (!err_code) condition, because I would never get empty 
> line
> as option(if not specified, I always have default matrix), but changed to 
> (err_code != 1) due to read-ability.
> Also, before sscanf I split matrix with spaces, so I process each single value
> of matrix seperately from others and check if they are ok.

My logic here is that you want to make sure that exactly one argument has been 
converted, so checking for != 1 is the right thing to do for errors.  Even if 
you can argue that == 0 is the only possible error case and other error cases 
can't possibly happen, someone reading the code also has to think about that in 
the same way to be able to convince themselves that there isn't an error, where 
if you wrote != 1 they wouldn't need to think about it at all.

(To offer a counterexample to the suggestion that you couldn't get -1 from 
sscanf() in the previous code, note that it only split for ' ' so it would fail 
on other whitespace.  E.g. "1 2 3 4   6 7 8 9" (tab in the middle) would be 
accepted as a matrix and then invoke undefined behaviour reading the 
uninitialised value in the middle of the that matrix.)

> about rdiv_buffer, bias_buffer, dims_buffer objects: they should be buffer 
> objects, because they store sequence of values, not a single value

You only need to pass one value to each invocation of the kernel, since only 
one value is needed for each plane.

> ---
>  configure   |   1 +
>  libavfilter/Makefile|   1 +
>  libavfilter/allfilters.c|   1 +
>  libavfilter/opencl/convolution.cl   |  46 
>  libavfilter/opencl_source.h |   1 +
>  libavfilter/vf_convolution_opencl.c | 449 
> 
>  6 files changed, 499 insertions(+)
>  create mode 100644 libavfilter/opencl/convolution.cl
>  create mode 100644 libavfilter/vf_convolution_opencl.c
> 
> diff --git a/configure b/configure
> index 6916b45..bf5c312 100755
> --- a/configure
> +++ b/configure
> @@ -3210,6 +3210,7 @@ blackframe_filter_deps="gpl"
>  boxblur_filter_deps="gpl"
>  bs2b_filter_deps="libbs2b"
>  colormatrix_filter_deps="gpl"
> +convolution_opencl_filter_deps="opencl"
>  convolve_filter_deps="avcodec"
>  convolve_filter_select="fft"
>  coreimage_filter_deps="coreimage appkit"
> diff --git a/libavfilter/Makefile b/libavfilter/Makefile
> index 6a60836..d005934 100644
> --- a/libavfilter/Makefile
> +++ b/libavfilter/Makefile
> @@ -156,6 +156,7 @@ OBJS-$(CONFIG_COLORLEVELS_FILTER)+= 
> vf_colorlevels.o
>  OBJS-$(CONFIG_COLORMATRIX_FILTER)+= vf_colormatrix.o
>  OBJS-$(CONFIG_COLORSPACE_FILTER) += vf_colorspace.o 
> colorspacedsp.o
>  OBJS-$(CONFIG_CONVOLUTION_FILTER)+= vf_convolution.o
> +OBJS-$(CONFIG_CONVOLUTION_OPENCL_FILTER) += vf_convolution_opencl.o 
> opencl.o opencl/convolution.o
>  OBJS-$(CONFIG_CONVOLVE_FILTER)   += vf_convolve.o framesync.o
>  OBJS-$(CONFIG_COPY_FILTER)   += vf_copy.o
>  OBJS-$(CONFIG_COREIMAGE_FILTER)  += vf_coreimage.o
> diff --git a/libavfilter/allfilters.c b/libavfilter/allfilters.c
> index 9adb109..f2dc55e 100644
> --- a/libavfilter/allfilters.c
> +++ b/libavfilter/allfilters.c
> @@ -166,6 +166,7 @@ static void register_all(void)
>  REGISTER_FILTER(COLORMATRIX,colormatrix,vf);
>  REGISTER_FILTER(COLORSPACE, colorspace, vf);
>  REGISTER_FILTER(CONVOLUTION,convolution,vf);
> +REGISTER_FILTER(CONVOLUTION_OPENCL, convolution_opencl, vf);
>  REGISTER_FILTER(CONVOLVE,   convolve,   vf);
>  REGISTER_FILTER(COPY,   copy,   vf);
>  REGISTER_FILTER(COREIMAGE,  coreimage,  vf);
> diff --git a/libavfilter/opencl/convolution.cl 
> b/libavfilter/opencl/convolution.cl
> new file mode 100644
> index 000..192f1ef
> --- /dev/null
> +++ b/libavfilter/opencl/convolution.cl
> @@ -0,0 +1,46 @@
> +/*
> + * This file is part of 

Re: [FFmpeg-devel] [PATCH] libavfilter: Add OpenCL convolution filter v0.1

[Carl Eugen Hoyos]

2018-03-14 10:43 GMT+01:00, Danil Iashchenko :

> About sscanf. I had (!err_code) condition, because I would never
> get empty line as option (if not specified, I always have default
> matrix), but changed to (err_code != 1) due to read-ability.

Was this requested?
I ask because several developers believe the opposite is true...

[...]

> --- /dev/null
> +++ b/libavfilter/opencl/convolution.cl
> @@ -0,0 +1,46 @@
> +/*
> + * This file is part of FFmpeg.

Please add your name.

[...]

> +for (i = 0; i < 4; i++) {
> +p = ctx->matrix_str[i];
> +while (ctx->matrix_size[i] < 49) {

> +if (!(arg = av_strtok(p, " ", )))

Please split this line.

And ask one of the mentors to add your name to the OpenCL project
on https://trac.ffmpeg.org/wiki/SponsoringPrograms/GSoC/2018 -
other potential students should know that you are interested in this
project and that you have (nearly) completed a qualification task.

Thank you, Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] libavfilter: Add OpenCL convolution filter v0.1

[Danil Iashchenko]

Behaves like the existing convolution filter, except working on OpenCL
hardware frames.
Takes exactly the same options: 4 convolution matrices, 4 rdiv values, 4 bias 
values.
If not specified, default parameters are applied.
Matrices can be different sizes.

NEW IN THIS PATCH:
-fixed bug, if matrices have different dims.
-renamed some variables due to readability.

filter applies:
matrix0, rdiv0, bias0 to image plane0.
matrix1, rdiv1, bias1 to image plane1.
matrix2, rdiv2, bias2 to image plane2.
matrix3, rdiv3, bias3 to image plane3.

About Kernel parameters:
dst - destination image
src - source image
coef_matrices_dims - stores dimensions of matrix{0..3} consecutively one after 
the other
coef_matrices - stores matrices{0..3} consecutively one after the other
rdivs - stores rdiv{0..3} parameters consecutively one after the other
biases - stores bias{0..3} parameters consecutively one after the other

About sscanf. I had (!err_code) condition, because I would never get empty line
as option(if not specified, I always have default matrix), but changed to 
(err_code != 1) due to read-ability.
Also, before sscanf I split matrix with spaces, so I process each single value
of matrix seperately from others and check if they are ok.

about rdiv_buffer, bias_buffer, dims_buffer objects: they should be buffer 
objects, because they store sequence of values, not a single value

---
 configure   |   1 +
 libavfilter/Makefile|   1 +
 libavfilter/allfilters.c|   1 +
 libavfilter/opencl/convolution.cl   |  46 
 libavfilter/opencl_source.h |   1 +
 libavfilter/vf_convolution_opencl.c | 449 
 6 files changed, 499 insertions(+)
 create mode 100644 libavfilter/opencl/convolution.cl
 create mode 100644 libavfilter/vf_convolution_opencl.c

diff --git a/configure b/configure
index 6916b45..bf5c312 100755
--- a/configure
+++ b/configure
@@ -3210,6 +3210,7 @@ blackframe_filter_deps="gpl"
 boxblur_filter_deps="gpl"
 bs2b_filter_deps="libbs2b"
 colormatrix_filter_deps="gpl"
+convolution_opencl_filter_deps="opencl"
 convolve_filter_deps="avcodec"
 convolve_filter_select="fft"
 coreimage_filter_deps="coreimage appkit"
diff --git a/libavfilter/Makefile b/libavfilter/Makefile
index 6a60836..d005934 100644
--- a/libavfilter/Makefile
+++ b/libavfilter/Makefile
@@ -156,6 +156,7 @@ OBJS-$(CONFIG_COLORLEVELS_FILTER)+= 
vf_colorlevels.o
 OBJS-$(CONFIG_COLORMATRIX_FILTER)+= vf_colormatrix.o
 OBJS-$(CONFIG_COLORSPACE_FILTER) += vf_colorspace.o colorspacedsp.o
 OBJS-$(CONFIG_CONVOLUTION_FILTER)+= vf_convolution.o
+OBJS-$(CONFIG_CONVOLUTION_OPENCL_FILTER) += vf_convolution_opencl.o 
opencl.o opencl/convolution.o
 OBJS-$(CONFIG_CONVOLVE_FILTER)   += vf_convolve.o framesync.o
 OBJS-$(CONFIG_COPY_FILTER)   += vf_copy.o
 OBJS-$(CONFIG_COREIMAGE_FILTER)  += vf_coreimage.o
diff --git a/libavfilter/allfilters.c b/libavfilter/allfilters.c
index 9adb109..f2dc55e 100644
--- a/libavfilter/allfilters.c
+++ b/libavfilter/allfilters.c
@@ -166,6 +166,7 @@ static void register_all(void)
 REGISTER_FILTER(COLORMATRIX,colormatrix,vf);
 REGISTER_FILTER(COLORSPACE, colorspace, vf);
 REGISTER_FILTER(CONVOLUTION,convolution,vf);
+REGISTER_FILTER(CONVOLUTION_OPENCL, convolution_opencl, vf);
 REGISTER_FILTER(CONVOLVE,   convolve,   vf);
 REGISTER_FILTER(COPY,   copy,   vf);
 REGISTER_FILTER(COREIMAGE,  coreimage,  vf);
diff --git a/libavfilter/opencl/convolution.cl 
b/libavfilter/opencl/convolution.cl
new file mode 100644
index 000..192f1ef
--- /dev/null
+++ b/libavfilter/opencl/convolution.cl
@@ -0,0 +1,46 @@
+/*
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * FFmpeg is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with FFmpeg; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+__kernel void convolution_global(__write_only image2d_t dst,
+ __read_only  image2d_t src,
+ __constant int *coef_matrices_dims,
+ __constant float *coef_matrices,
+ __constant float *rdivs,
+ __constant float *biases)
+{
+const sampler_t sampler =