[FFmpeg-devel] [PATCH] libavfilter:add opencl warpper and opencl code for deshake

Stefano Sabatini stefasab at gmail.com
Tue Feb 19 21:18:29 CET 2013


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


More information about the ffmpeg-devel mailing list