[FFmpeg-devel] [PATCH] libavfilter:add opencl warpper and opencl code for deshake
Wei Gao
highgod0401 at gmail.com
Wed Feb 20 01:38:06 CET 2013
Hi,
Thanks I will fix the patch according to the comments.
Just one more question, the patch I will submit according to the comments
is just opencl wrapper, right? Not include the deshake OpenCL filter patch?
I should submit the Deshake patch after the wrapper patch accept, right?
Thanks
Best regards.
2013/2/20 Stefano Sabatini <stefasab at gmail.com>
> On date Tuesday 2013-02-19 19:58:29 +0800, Wei Gao encoded:
> > I am sorry that I was busy on writing the opencl scale code and share
> > buffer code and did not check my email. I read the comments today.
> > I will modify the patch as the comments,but I have some questions as
> > followed:
> [...]
> > > > diff --git a/configure b/configure
> > > > index b61359c..b6d12b6 100755
> > > > --- a/configure
> > > > +++ b/configure
> > > > @@ -140,6 +140,7 @@ Component options:
> > > > --disable-rdft disable RDFT code
> > > > --disable-fft disable FFT code
> > > > --enable-dxva2 enable DXVA2 code
> > > > + --enable-opencl enable OpenCL code
> > > > --enable-vaapi enable VAAPI code [autodetect]
> > > > --enable-vda enable VDA code [autodetect]
> > > > --enable-vdpau enable VDPAU code [autodetect]
> > > > @@ -1196,6 +1197,7 @@ CONFIG_LIST="
> > > > network
> > > > nonfree
> > > > openal
> > > > + opencl
> > > > openssl
> > > > pic
> > > > rdft
> > >
> > > Don't you need to check for the presence of opencl
> > > headers/libs/whatever?
> > >
> >
> > I will add the check code, but I am not familiar with the configure file,
> > should I reference the dxva2 part?
>
> Don't know, you could manually check for the presence of the headers
> and libraries, or use pkg-config. You should find examples for both
> cases in the configure file.
>
> >
> > >
> > > > @@ -1990,6 +1992,7 @@ cropdetect_filter_deps="gpl"
> > > > decimate_filter_deps="gpl avcodec"
> > > > delogo_filter_deps="gpl"
> > > > deshake_filter_deps="avcodec"
> > > > +deshake_opencl_filter_deps="opencl deshake_filter"
> > > > drawtext_filter_deps="libfreetype"
> > > > ebur128_filter_deps="gpl"
> > > > flite_filter_deps="libflite"
> > > > @@ -4295,6 +4298,7 @@ echo "libx264 enabled ${libx264-no}"
> > > > echo "libxavs enabled ${libxavs-no}"
> > > > echo "libxvid enabled ${libxvid-no}"
> > > > echo "openal enabled ${openal-no}"
> > > > +echo "opencl enabled ${opencl-no}"
> > > > echo "openssl enabled ${openssl-no}"
> > > > echo "zlib enabled ${zlib-no}"
> > > > echo "bzlib enabled ${bzlib-no}"
> > > > diff --git a/ffmpeg.c b/ffmpeg.c
> > > > index bbd21b6..38afbe8 100644
> > > > --- a/ffmpeg.c
> > > > +++ b/ffmpeg.c
> > > > @@ -97,6 +97,10 @@
> > > > #include <pthread.h>
> > > > #endif
> > > >
> > > > +#if CONFIG_OPENCL
> > > > +#include "libavutil/openclwrapper.h"
> > > > +#endif
> > > > +
> > > > #include <time.h>
> > > >
> > > > #include "ffmpeg.h"
> > > > @@ -3307,10 +3311,18 @@ int main(int argc, char **argv)
> > > > // exit(1);
> > > > // }
> > > >
> > > > +#if CONFIG_OPENCL
> > > > + if (av_init_opencl_run_env(0,NULL,"-I.",NULL)) {
> > > > + av_log(NULL,AV_LOG_ERROR,"Init OpenCL Failed\n");
> > > > + }
> > > > +#endif
> > >
> > > I'm not sure this is correct. Ideally the init should be done in the
> > > module, so you don't need to init it in application code. This could
> > > be moved to the filter code.
> > >
> > >
>
> > filter will use it and opendecode/openencode will use it too.So both
> codec
> > and filter use the opencl init code that I am not sure where can I add
> the
> > code to ffmpeg.
>
> Usually the init function is idempotent (that is you can init multiple
> times with no side effects), or there is a counter which will be
> incremented on init and decrememented on uninit. Thus I think it
> should be safe to move the init code to the component, but make sure
> this case is indeed supported by OpenCL.
>
> [...]
> > > > +inline const float av_clipf(float a, float amin, float amax)
> > > > +{
> > > > + if (a < amin) return amin;
> > > > + else if (a > amax) return amax;
> > > > + else return a;
> > > > +}
> > >
> > > what's this good for (we already have an av_clipf() function)?
> > >
> >
>
> > Yes,FFmpeg have the av_clipf() function.but the OpenCL kernel code will
> > compile on GPU at the ffmpeg runtime,and you can see the "const char
> > *kernel_deshake:", the openclwrapper code will pass the string to GPU to
> > compile, so we need to rewrite the function for GPU to comiple.
>
> Oh I see, any need to prefix it with "av_"?
>
> > >
> > > > +
> > > > +
> > > > +kernel void avfilter_transform(global unsigned char *src,
> > > > + global unsigned char *dst,
> > > > + global float *matrix,
> > > > + global float *matrix2,
> > > > + int interpolate,
> > > > + int fillmethod,
> > > > + int src_stride_lu,
> > > > + int dst_stride_lu,
> > > > + int src_stride_ch,
> > > > + int dst_stride_ch,
> > > > + int height,
> > > > + int width,
> > > > + int ch,
> > > > + int cw)
> > >
> > > How does this relate to avfilter_transform() defined in
> > > libavfilter/avfilter.h?
> > >
> >
> > I reference the "avfilter_transform" function in transform.c, I must
> write
> > it as a string and pass it to GPU. all the function in deshake_kernel.h
> is
> > in one string "const char *kernel_deshake"
>
> I see. So basically the code doesn't have to reference any internal
> library function, is that right (sorry but I have no previous
> experience with OpenCL).
>
> [...]
> > >
> > > > + OCLCHECK( clSetKernelArg, env->kernel, 0, sizeof(cl_mem),
> > > (void*)&src);
> > > > + OCLCHECK( clSetKernelArg, env->kernel, 1, sizeof(cl_mem),
> > > (void*)&dst);
> > > > + OCLCHECK( clSetKernelArg, env->kernel, 2, sizeof(cl_mem),
> > > (void*)&matrix_buf);
> > > > + OCLCHECK( clSetKernelArg, env->kernel, 3, sizeof(cl_mem),
> > > (void*)&matrix_buf2);
> > > > + OCLCHECK( clSetKernelArg, env->kernel, 4, sizeof(cl_int),
> > > (void*)&interpolate);
> > > > + OCLCHECK( clSetKernelArg, env->kernel, 5, sizeof(cl_int),
> > > (void*)&fillmethod);
> > > > + OCLCHECK( clSetKernelArg, env->kernel, 6, sizeof(cl_int),
> > > (void*)&src_stride_lu);
> > > > + OCLCHECK( clSetKernelArg, env->kernel, 7, sizeof(cl_int),
> > > (void*)&dst_stride_lu);
> > > > + OCLCHECK( clSetKernelArg, env->kernel, 8, sizeof(cl_int),
> > > (void*)&src_stride_ch);
> > > > + OCLCHECK( clSetKernelArg, env->kernel, 9, sizeof(cl_int),
> > > (void*)&dst_stride_ch);
> > > > + OCLCHECK( clSetKernelArg, env->kernel, 10, sizeof(cl_int),
> > > (void*)&height);
> > > > + OCLCHECK( clSetKernelArg, env->kernel, 11, sizeof(cl_int),
> > > (void*)&width);
> > > > + OCLCHECK( clSetKernelArg, env->kernel, 12, sizeof(cl_int),
> > > (void*)&ch);
> > > > + OCLCHECK( clSetKernelArg, env->kernel, 13, sizeof(cl_int),
> > > (void*)&cw);
> > >
> > > You could use a macro to ease readability/refactorization.
> > >
> >
> > OCLCHECK is already a macro, it will run the function (the first
> parameter)
> > and check the return value.Do you mean I should write a mocro for
> > clSetKernelArg to encapsulate the first three parameters?
>
> I mean something like:
> #define SET_OCLCHECK(num, struct, var) \
> OCLCHECK(clSetKernelArg, env->kernel, num, sizeof(struct), (void*)&var)
>
> which should be easier on the eyes.
>
> [...]
> > > > diff --git a/libavutil/Makefile b/libavutil/Makefile
> > > > index 544c33f..307d2f8 100644
> > > > --- a/libavutil/Makefile
> > > > +++ b/libavutil/Makefile
> > >
> > > Please split the patch in two, one for libavfilter, and one for
> > > libavutil, that should simplify review.
> > >
> > >
> > the opencl filter is dependent on the opencl wrapper to run and it uses
> the
> > api of opencl wrapper.So, should I submit the opencl wrapper patch first
> > and then submit the deshake opencl filter patch until the opencl wrapper
> > patch accept?
>
> Yes, at least this should simplify review, and probably save you same
> time (smaller patches are easier to review and to update).
>
> [...]
> > > > diff --git a/libavutil/openclwrapper.c b/libavutil/openclwrapper.c
> > > > new file mode 100644
> > > > index 0000000..f11aed6
> > > > --- /dev/null
> > > > +++ b/libavutil/openclwrapper.c
> > > > @@ -0,0 +1,812 @@
> > > > +/*
> > > > + * Copyright (C) 2012 Peng Gao <peng at multicorewareinc.com>
> > > > + * Copyright (C) 2012 Li Cao <li at multicorewareinc.com>
> > > > + * Copyright (C) 2012 Wei Gao <weigao at multicorewareinc.com>
> > > > + *
> > > > + * 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
> > > > + */
> > > > +
> > >
> > > > +#include <stdio.h>
> > > > +#include <stdlib.h>
> > > > +#include <string.h>
> > >
> > > this should not be required
> > >
> > > > +#include <windows.h>
> > >
> > > That looks definitively non portable, should be avoided.
> > >
> > > > +#include "openclwrapper.h"
> > > > +#include "avstring.h"
> > > > +#include "log.h"
> > >
> > > > +#include "libavfilter/deshake_kernel.h"
> > >
> > > you are not supposed to include a libavfilter header from libavutil
> > > (which is autocontained and must not depend on other FFmpeg
> > > libraries, not even for compilation).
> > >
> >
> > The wrapper will use the kernel string to compile on GPU, the
> > deshake_kernel.h is just the kernel string.Should I move the file to
> > libavutil?
>
> Let me check the rest of the code, but no, including another libav*
> library header from libavutil is not OK, so we need a more flexible
> mechanism.
>
> >
> >
> > >
> > > > +
> > > > +
> > > > +
> > >
> > > > +#if defined(__APPLE__)
> > > > +#include <OpenCL/cl.h>
> > > > +#else
> > > > +#include <CL/cl.h>
> > > > +#endif
> > > > +
> > >
> > > This should be fixed at the configure level, feel free to ask if you
> > > don't know why.
> > >
> >
> > Sorry, I don't know what this mean.
>
> By using pkg-config or a similar mechanism it should be possible to
> do:
> #include <cl.h>
>
> or something like that, leading to more compact and portable code
> (setting different include paths depending on the platform is an
> insane practice but nothing we can do about).
>
> > > [...]
> > > > +static int generat_bin_from_kernel_source(cl_program program, const
> > > char * cl_file_name)
> > > > +{
> > > > + int i = 0;
> > > > + cl_int status;
> > > > + size_t *binarySizes, numDevices;
> > > > + cl_device_id *devices;
> > > > + char **binaries;
> > > > + char *str = NULL;
> > > > +
> > > > + status = clGetProgramInfo(program,
> > > > + CL_PROGRAM_NUM_DEVICES,
> > > > + sizeof(numDevices),
> > > > + &numDevices,
> > > > + NULL);
> > > > + if (status != CL_SUCCESS) {
> > > > +
> > >
> av_log(NULL,AV_LOG_ERROR,"generat_bin_from_kernel_source:clGetProgramInfo
> > > error,status = %d\n",status);
> > > > + return -1;
> > > > + }
> > > > + devices = (cl_device_id*)av_malloc(sizeof(cl_device_id) *
> > > numDevices);
> > > > + if( devices == NULL )
> > > > + return -1;
> > > > + /* grab the handles to all of the devices in the program. */
> > > > + status = clGetProgramInfo(program,
> > > > + CL_PROGRAM_DEVICES,
> > > > + sizeof(cl_device_id) * numDevices,
> > > > + devices,
> > > > + NULL);
> > > > +
> > > > + /* figure out the sizes of each of the binaries. */
> > > > + binarySizes = (size_t*)av_malloc(sizeof(size_t) * numDevices);
> > > > +
> > > > + status = clGetProgramInfo(program,
> > > > + CL_PROGRAM_BINARY_SIZES,
> > > > + sizeof(size_t) * numDevices,
> > > > + binarySizes, NULL);
> > >
> > > > + if (status != CL_SUCCESS) {
> > > > +
> > >
> av_log(NULL,AV_LOG_ERROR,"generat_bin_from_kernel_source:clGetProgramInfo
> > > error,status = %d\n",status);
> > > > + return -1;
> > > > + }
> > >
> > > About logging: av_log(NULL, ...) should be avoided in the lib, you
> should
> > > provide a logging context to the library.
> > >
> >
> > Is there any code I can reference?
>
> av_image_check_size in imgutils.c.
>
> [...]
> > > > +int av_run_kernel(const char *kernel_name, void **userdata);
> > > > +int av_init_opencl_run_env(int argc, char **argv, const char
> > > *build_option,void *extOpenCLInfo);
> > > > +int av_release_opencl_run_env(void);
> > > > +int av_opencl_stats(void);
> > > > +int av_init_opencl_attr(OpenCLEnv * env);
> > > > +int av_create_kernel(const char * kernelname, KernelEnv * env);
> > > > +int av_release_kernel(KernelEnv * env);
> > > > +int av_get_kernel_env(KernelEnv *env);
> > > > +int av_create_buffer(void **cl_Buf, int flags, int size);
> > > > +int av_read_opencl_buffer(void *cl_inBuf, unsigned char *outbuf, int
> > > size);
> > > > +int av_write_opencl_buffer(void *cl_inBuf, unsigned char *Ybuf,
> > > unsigned char *Ubuf, unsigned char *Vbuf, int linesize0, int
> linesize1, int
> > > linesize2, int height, int offset);
> > > > +cl_device_id av_get_device_id(void);
> > > > +cl_context av_get_context(void);
> > > > +cl_command_queue av_get_command_queue(void);
> > > > +void av_release_buffer(void *cl_Buf);
> > > > +int av_read_opencl_frame_buffer(void *cl_inBuf, unsigned char *Ybuf,
> > > unsigned char *Ubuf, unsigned char *Vbuf, int linesize0, int
> linesize1, int
> > > linesize2, int height);
> > >
> > > I suggest a common av_cl_ or av_opencl_ prefix for all the opencl
> > > functions, this is the usual convention adopted for public
> > > interfaces.
> > >
> > >
> > I will fix it.
>
> Note also that you may move all this to libavfilter as internal
> symbols, so you don't have to worry about a public interface. This
> seems a good idea, especially considering that at the moment there are
> no other uses of OpenCL outside the filter.
> --
> FFmpeg = Faithless and Frightening Merciless Pitiful Epic Gigant
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
More information about the ffmpeg-devel
mailing list