[FFmpeg-devel] [PATCH 1/2] libavutil/libavfilter: add opencl wrapper to ffmpeg
Wei Gao
highgod0401 at gmail.com
Sun Mar 24 11:36:37 CET 2013
2013/3/24 Stefano Sabatini <stefasab at gmail.com>
> On date Sunday 2013-03-24 12:08:11 +0800, Wei Gao encoded:
> > Hi,
> >
> > Stefano Sabatini, thanks for your comments, and some questions and
> > explanations are as follows.
> >
> > Thanks
> > Best regards
> >
> > 2013/3/22 Stefano Sabatini <stefasab at gmail.com>
> > > > diff --git a/libavutil/opencl.c b/libavutil/opencl.c
> > > > new file mode 100644
> > > > index 0000000..c577280
> > > > --- /dev/null
> > > > +++ b/libavutil/opencl.c
> > > > @@ -0,0 +1,988 @@
> [...]
> > > > +#define MAX_KERNEL_STRING_LEN 64
> > > > +#define MAX_CLFILE_NUM 50
> > >
> > > > +#define MAX_CLKERNEL_NUM 200
> > > > +#define MAX_CLFILE_PATH 255
> > > > +#define MAX_KERNEL_NUM 50
> > >
> > > What's the difference between MAX_CLKERNEL_NUM and MAX_KERNEL_NUM?
> > >
> > MAX_KERNEL_NUM is defined but not used, I will delete it.
> >
> > >
> > > > +#define MAX_FILTER_NAME_LEN 64
> > > > +#define MAX_FILTER_NUM 200
> > > > +#define MAX_KERNEL_SRCFILE_LEN 256
> > > > +
> > > > +
> > > > +typedef struct OpenCLEnv {
> > > > + cl_platform_id platform;
> > > > + cl_context context;
> > > > + cl_device_id devices;
> > > > + cl_command_queue command_queue;
> > > > +} OpenCLEnv;
> > > > +
> > > > +typedef struct GPUEnv {
> > >
> > > > + //share vb in all modules in hb library
> > >
> > > it is not obvious to me what "vb" and "hb" stand for
> > >
> > These files are referenced our other projects, I forgot to delete this
> > note, sorry.
> >
> > >
> > > > + cl_platform_id platform;
> > > > + cl_device_type device_type;
> > > > + cl_context context;
> > > > + cl_device_id *device_ids;
> > > > + cl_device_id device_id;
> > > > + cl_command_queue command_queue;
> > > > + cl_kernel kernels[MAX_CLFILE_NUM];
> > > > + cl_program programs[MAX_CLFILE_NUM]; //one program object maps
> one
> > > kernel source file
> > > > + char kernel_srcfile[MAX_CLFILE_NUM][MAX_KERNEL_SRCFILE_LEN];
> > > //the max len of kernel file name is 256
> > > > + int file_count; // only one kernel file
> > > > +
> > > > + char kernel_names[MAX_CLKERNEL_NUM][MAX_KERNEL_STRING_LEN+1];
> > > > + av_opencl_kernel_function kernel_functions[MAX_CLKERNEL_NUM];
> > > > + const char *kernel_code[MAX_CLKERNEL_NUM];
> > > > + int kernel_count;
> > >
> > > > + int reg_kernel_count;
> > >
> > > more meaningful name (e.g. kernel_function_count?)
> > >
> > this variable means that the register kernel which used in the
> runtime.For
> > example there are 10 kernels has been registered, but use only 1 in
> > runtime. so the reg_kernel_count is 1, and kernel_count is 10, should I
> > rename it to runtime_kernel_count?
>
> runtime_kernel_count sounds OK.
>
> [...]
> > > > diff --git a/libavutil/opencl.h b/libavutil/opencl.h
> > > > new file mode 100644
> > > > index 0000000..e703cbc
> > > > --- /dev/null
> > > > +++ b/libavutil/opencl.h
> > > > @@ -0,0 +1,219 @@
> > > > +/*
> > > > + * 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 "config.h"
> > > > +
> > > > +#ifndef LIBAVUTIL_OPENCLWRAPPER_H
> > > > +#define LIBAVUTIL_OPENCLWRAPPER_H
> > > > +
> > > > +#include <CL/cl.h>
> > > > +
> > >
> > > > +#define AV_OPENCL_KERNEL( ... )# __VA_ARGS__
> > >
> > > is this useful?
> > >
> > it is used to define kernel string in deshake_kernel.h:const char
> > *ff_kernel_deshake_opencl = AV_OPENCL_KERNEL(......);
>
> Allright keep it for now.
>
> [...]
> > > > +/**
> > > > + * Write data from memory to OpenCL buffer. Src is frames
> > > buffer(data[0],data[1]...), dst is OpenCL buffer.
> > > > + *
> > > > + *@param cl_buf The pointer of OpenCL buffer.
> > > > + *@param cl_buffer_size OpenCL buffer size
> > > > + *@param data Picture or audio data for each plane
> > > > + *@plane_size Size of each plane
> > > > + *@param plane_num The input plane number
> > > > + *@param offset The offset of OpenCL buffer start
> > > position
> > > > + *@return 0 on success, other values on error
> > > > + */
> > > > +
> > > > +int av_opencl_write_buffer(void *cl_inbuf, size_t cl_buffer_size,
> > > uint8_t **data, int *plane_size, int plane_num, int offset);
> > >
> > > I suggest to skip this API for this round of review, especially if it
> > > is not yet used by the deshake filter.
> > >
>
> > Yes, it is used by deshake filter. The OpenCL work flow is that copy the
> > data from CPU buffers to GPU buffer, then GPU process the data using
> > kernel, then copy the processed data to CPU buffer, this function
> implement
> > the first step:copy data from CPU buffers to GPU buffer. should I rename
> it
> > to av_opencl_copy_data_to_gpu
>
> The current name may be OK, but I need to see the whole API.
>
> [...]
> > > > +
> > > > +/**
> > > > + * Read frame data from OpenCL buffer to frame buffer, src buffer
> is
> > > OpenCL buffer, dst buffer is frame buffer(data[0],data[1]....).
> > > > + *
> > > > + *@param cl_buf The pointer of OpenCL buffer.
> > > > + *@param cl_buffer_size OpenCL buffer size
> > > > + *@param data Picture or audio data for each plane
> > > > + *@plane_size Size of each plane
> > > > + *@param plane_num The input plane number
> > > > + *@return 0 on success, other values on error
> > > > + */
> > > > +
> > > > +int av_opencl_read_to_frame_buffer(void *cl_inbuf, size_t
> > > cl_buffer_size, uint8_t **data,
> > > > + int *plane_size, int
> > > plane_num);
> > >
> > > should be put together with the other buffer-related functions.
> > > Also "av_opencl_copy_buffer" is probably a better name.
> > >
> > This function implements copying data from CPU to CPU, should I rename it
> > to av_opencl_copy_data_to_cpu?
>
>
> int av_opencl_create_buffer(void **cl_buf, int flags, size_t size,void
> *host_ptr);
> int av_opencl_read_buffer(void *cl_inbuf, uint8_t *outbuf, size_t size);
>
> This is used to create and read a buffer, and seems mostly OK.
>
> ...
>
> int av_opencl_write_buffer(void *cl_inbuf, size_t cl_buffer_size, uint8_t
> **data, int *plane_size, int plane_num, int offset);
> int av_opencl_read_to_frame_buffer(void *cl_inbuf, size_t cl_buffer_size,
> uint8_t **data, int *plane_size, int plane_num);
>
> This is used to access image buffers, so maybe;
> av_opencl_write_image
> av_opencl_read_image
>
> Or we can use hierarchical prefixing, and have:
> int av_opencl_buffer_create(void **cl_buf, int flags, size_t size,void
> *host_ptr);
> int av_opencl_buffer_read(void *cl_inbuf, uint8_t *outbuf, size_t size);
> int av_opencl_buffer_write_image(void *cl_inbuf, size_t cl_buffer_size,
> uint8_t **data, int *plane_size, int plane_num, int offset);
> int av_opencl_buffer_read_image(void *cl_inbuf, size_t cl_buffer_size,
> uint8_t **data, int *plane_size, int plane_num);
>
> CPU and GPU are maybe too specific terms, which should probably be
> avoided (theoretically the "GPU" from which you read could be not
> "graphical" but just another processing unit with dedicated memory).
>
OK, I will fix them and re-submit, one more question, the following comment
you give me, I didn't get what you mean,sorry
> + if (status != CL_SUCCESS) {
> + av_log(&openclutils,AV_LOG_ERROR,"generate_bin_from_kernel_source
error,clGetProgramInfo:%s\n",opencl_errstr(status));
> + return AVERROR_EXTERNAL;
> + }
> + devices = av_mallocz(sizeof(cl_device_id) * numdevices);
> + if (!devices)
> + return AVERROR(ENOMEM);
> + /* grab the handles to all of the devices in the program. */
> + status = clGetProgramInfo(program,
> + CL_PROGRAM_DEVICES,
> + sizeof(cl_device_id) * numdevices,
> + devices,
> + NULL);
this code could be possibly be factorized
>
> [...]
> --
> FFmpeg = Faithful Formidable Minimalistic Political Efficient Gospel
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
More information about the ffmpeg-devel
mailing list