[FFmpeg-devel] [PATCH 1/2] libavutil/libavfilter: opencl wrapper based on comments on 20130328

[Stefano Sabatini]

On date Friday 2013-03-29 16:51:30 +0800, Wei Gao encoded:
> Hi, Thanks for your reply. some questions and explanations
> 
> Thanks
> Best regards
> 
> 
> 2013/3/29 Stefano Sabatini <stefasab at gmail.com>
> 
> > On date Thursday 2013-03-28 20:57:44 +0800, Wei Gao encoded:
> > >
> >
> > > From 38aa9bb65c63d44cd1586383f37197bee93c61ab Mon Sep 17 00:00:00 2001
> > > From: highgod0401 <highgod0401 at gmail.com>
> > > Date: Thu, 28 Mar 2013 20:42:21 +0800
> > > Subject: [PATCH 1/2] opencl wrapper based on comments on 20130328
> > >
> > > ---
> >
> >
> > > +        /*initialize devices, context, command_queue*/
> > > +        AVDictionaryEntry *opt_entry = av_dict_get(options,
> > "build_option", NULL, 0);
> >
> > "build_options"?
> >
> > "build_flags" also seems fine, whatever is more consistent with the
> > specification text.
> >
> I reference the document of OpenCL Specification Version: 1.2, it named
> build options.

OK so let's stick with it.

> 
> >
> > > +        ret = init_opencl_env(&gpu_env, ext_opencl_info);
> > > +        if (ret < 0)
> > > +            goto end;
> > > +        /*initialize program, kernel_name, kernel_count*/
> >
> > > +        ret = compile_kernel_file(&gpu_env, opt_entry->value);
> >
> > crash if opt_entry is NULL?
> >
> will not crush, the OpenCL API "clBuildProgram" accept the NULL pointer,
> the kernel can be compiled too.

opt_entry->value

NULL pointer dereference in case opt_entry is NULL (this happens in
case "build_options" is not defined in the dictionary).

> >
> >  *
> >  * A kernel with name kernel_name must have been registered and
> >  * created with av_opencl_create_kernel(). This is requested for
> >  * executing the kernel code through av_opencl_run_kernel().
> >  *
> >  * @param kernel_name      name used to find the kernel in OpenCL runtime
> > environment
> >  * @param function         user defined function, should not be NULL, used
> > to set the input parameter in the kernel environment
> >  * @return >=0 on success, a negative error code on failure
> >  */
> > int av_opencl_register_kernel_function(const char *kernel_name,
> > av_opencl_kernel_function function);
> >
> > Also: why don't you register the function handle when you create the
> > kernel?
> > This would simplify usage and reduce API complexity.
> >
[...] 
> > > +
> > > +
> > > +/**
> > > + * Create kernel object on the specified OpenCL run in env.
> > > + *
> > > + * @param env                   the kernel environment which has been
> > created by av_opencl_init
> >
> > why do you need to pass this (since there is a unique global context,
> > why do you need to pass it)?
> >
> 
> sorry about the comment of env it should be "it is used for user to get the
> kernel env created by av_opencl_init, user will use it to run kernel".

Yes but again if there is a single global environment, why do you need
to pass it around?

> 
> >
> > > + * @param kernel_entry_point      kernel name
> >
> > wrong
> >
> 
> 
> > Also you may specify that this is the name of a function which must be
> > specified in the kernel code (BTW what happens in case it is not?).
> >
> 
> this is the real kernel function name which is in the kernel code. the
> function wtich have the  prefix "kernel" or "__kernel" is the kernel
> function. this name is the function's name, you can reference
> deshake_kernel.h OpenCL API clCreateKernel will use this name to find
> whether the kernel is in the program which has been compiled. if yes, it
> will return a handle cl_kernel, and user can use it to call the function in
> GPU. So it is different from register_name
> if the name is not found in program, the API will return error, and handle
> should be NULL.

OK, my comment was about the fact that "kernel name" is not the same
as "kernel entry point".

[...]
-- 
FFmpeg = Freak and Fancy Multipurpose Ponderous Extensive Game