Re: [FFmpeg-devel] [PATCH] libavfilter: Add OpenCL convolution filter v0.2: add name

2018-03-23 Thread Josh de Kock 

On 2018/03/23 13:22, Danil Iashchenko wrote:

Thanks, fixed!

---
  libavfilter/opencl/convolution.cl   | 2 ++
  libavfilter/vf_convolution_opencl.c | 2 ++
  2 files changed, 4 insertions(+)


When you update patches you should send the same patch again (yes the 
whole patch) with the required changes, and a version in the email 
subject (i.e. [PATCH v2] libavfilter: Add OpenCL convolution filter).


--
Josh
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2018-03-23 Thread Mark Thompson 
On 22/03/18 22:52, Danil Iashchenko wrote:
> Hi there!
> Thank you for advices, I have fixed the problem when per plane matrices
> application was incorrect. Now it works as expected and behaves like the 
> existing vf_convolution filter.
> 
> Tested for yuv and nv12 formats.
> The following filters from ffmpeg documentation
> (https://ffmpeg.org/ffmpeg-filters.html#Examples-43) work correctly:
> 
>  * sharpen
>  * blur
>  * edge enchance
>  * edge detect
>  * laplacian edge detector which includes diagonals
>  * emboss
>  * custom tests
> 
> I have two questions:
> 
> 1. Looks like existing convolution filter implementaion ignores
> 0rdiv and 0bias parameters for 0m='0 0 0 0 1 0 0 0 0' matrix,
> so results will be the same for following sample filter runs:
> 
> -vf convolution='0m=0 0 0 0 1 0 0 0 0:0rdiv=1:0bias=20'
> -vf convolution='0m=0 0 0 0 1 0 0 0 0:0rdiv=1/3:0bias=0'
> -vf convolution='0m=0 0 0 0 1 0 0 0 0:0rdiv=1/2:0bias=100'
> -vf convolution='0m=0 0 0 0 1 0 0 0 0:0rdiv=1/5:0bias=50'
> 
> and will not differ from
> 
> -vf convolution='0m=0 0 0 0 1 0 0 0 0:0rdiv=1:0bias=0'
> 
> My implementation does not ignore 0rdiv and 0bias parameters in case of m0='0 
> 0 0 0 1 0 0 0 0',
> and results will differ from the existing implementation so I do not know
> which one is correct: mine or exisiting.

Good question, I don't know.  I feel like your answer makes more sense, but 
it's a pretty degenerate use of the filter so maybe it doesn't matter?  Would 
anyone more familiar with this (Paul?) like to offer an opinion?

> 2. I have a local kernel (not included in this patch), but there is no
> significant speed difference comparing to global kernel. Shall I include
> it in the next patch?

Even with 7x7 matrices, which load every value many more times than 3x3?

How about making a two patch series, one without it and then a second patch on 
top of it adding the local kernel?  It would be useful to see it and I may be 
able to test on more devices, if it's not useful then we can just not apply the 
second patch.

Some more review comments below.

Thanks,

- Mark


> ---
>  configure   |   1 +
>  libavfilter/Makefile|   1 +
>  libavfilter/allfilters.c|   1 +
>  libavfilter/opencl/convolution.cl   |  40 
>  libavfilter/opencl_source.h |   1 +
>  libavfilter/vf_convolution_opencl.c | 365 
> 
>  6 files changed, 409 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..aa1db97
> --- /dev/null
> +++ b/libavfilter/opencl/convolution.cl
> @@ -0,0 +1,40 @@
> +/*
> + * 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

[FFmpeg-devel] [PATCH] libavfilter: Add OpenCL convolution filter v0.2: add name

2018-03-23 Thread Danil Iashchenko 
Thanks, fixed!

---
 libavfilter/opencl/convolution.cl   | 2 ++
 libavfilter/vf_convolution_opencl.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/libavfilter/opencl/convolution.cl 
b/libavfilter/opencl/convolution.cl
index aa1db97..c0748cc 100644
--- a/libavfilter/opencl/convolution.cl
+++ b/libavfilter/opencl/convolution.cl
@@ -1,4 +1,6 @@
 /*
+ * Copyright (c) 2018 Danil Iashchenko
+ *
  * This file is part of FFmpeg.
  *
  * FFmpeg is free software; you can redistribute it and/or
diff --git a/libavfilter/vf_convolution_opencl.c 
b/libavfilter/vf_convolution_opencl.c
index b788033..e6b5438 100644
--- a/libavfilter/vf_convolution_opencl.c
+++ b/libavfilter/vf_convolution_opencl.c
@@ -1,4 +1,6 @@
 /*
+ * Copyright (c) 2018 Danil Iashchenko
+ *
  * This file is part of FFmpeg.
  *
  * FFmpeg is free software; you can redistribute it and/or
-- 
2.7.4

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2018-03-23 Thread Carl Eugen Hoyos 
2018-03-22 23:52 GMT+01:00, Danil Iashchenko :

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

Any reason why you don't put your name here?

To state the obvious: There are now two probably qualified
students that are interested in the OpenCL task...

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.2

2018-03-22 Thread Danil Iashchenko 
Hi there!
Thank you for advices, I have fixed the problem when per plane matrices
application was incorrect. Now it works as expected and behaves like the 
existing vf_convolution filter.

Tested for yuv and nv12 formats.
The following filters from ffmpeg documentation
(https://ffmpeg.org/ffmpeg-filters.html#Examples-43) work correctly:

 * sharpen
 * blur
 * edge enchance
 * edge detect
 * laplacian edge detector which includes diagonals
 * emboss
 * custom tests

I have two questions:

1. Looks like existing convolution filter implementaion ignores
0rdiv and 0bias parameters for 0m='0 0 0 0 1 0 0 0 0' matrix,
so results will be the same for following sample filter runs:

-vf convolution='0m=0 0 0 0 1 0 0 0 0:0rdiv=1:0bias=20'
-vf convolution='0m=0 0 0 0 1 0 0 0 0:0rdiv=1/3:0bias=0'
-vf convolution='0m=0 0 0 0 1 0 0 0 0:0rdiv=1/2:0bias=100'
-vf convolution='0m=0 0 0 0 1 0 0 0 0:0rdiv=1/5:0bias=50'

and will not differ from

-vf convolution='0m=0 0 0 0 1 0 0 0 0:0rdiv=1:0bias=0'

My implementation does not ignore 0rdiv and 0bias parameters in case of m0='0 0 
0 0 1 0 0 0 0',
and results will differ from the existing implementation so I do not know
which one is correct: mine or exisiting.

2. I have a local kernel (not included in this patch), but there is no
significant speed difference comparing to global kernel. Shall I include
it in the next patch?
---
 configure   |   1 +
 libavfilter/Makefile|   1 +
 libavfilter/allfilters.c|   1 +
 libavfilter/opencl/convolution.cl   |  40 
 libavfilter/opencl_source.h |   1 +
 libavfilter/vf_convolution_opencl.c | 365 
 6 files changed, 409 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..aa1db97
--- /dev/null
+++ b/libavfilter/opencl/convolution.cl
@@ -0,0 +1,40 @@
+/*
+ * 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,
+ int coef_matrix_dim,
+ __constant float *coef_matrix,
+ float div,
+ float bias)
+{
+const sampler_t sampler = (CLK_NORMALIZED_COORDS_FALSE | 
CLK_ADDRESS_CLAMP_TO_EDGE |