[FFmpeg-devel] [PATCH 1/2] libavutil/libavfilter: add opencl wrapper to ffmpeg
Stefano Sabatini
stefasab at gmail.com
Sun Mar 24 11:23:44 CET 2013
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).
[...]
--
FFmpeg = Faithful Formidable Minimalistic Political Efficient Gospel
More information about the ffmpeg-devel
mailing list