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

[Wei Gao]

Hi ,

Thanks for your reply. some explanations as follows

Thanks
Best regards

2013/3/30 Michael Niedermayer <michaelni at gmx.at>

> On Sat, Mar 30, 2013 at 06:47:42PM +0800, Wei Gao wrote:
> >
> [...]
> > +static const char *opencl_errstr(cl_int status)
> > +{
> > +    int i;
> > +    for (i = 0; i < sizeof(opencl_err_msg); i++) {
> > +        if (opencl_err_msg[i].err_code == status)
> > +            return opencl_err_msg[i].err_str;
> > +    }
> > +    return "unknown error";
> > +}
> > +
>
> > +AVOpenCLExternalInfo *av_opencl_malloc_external_environment(void)
> > +{
>
> > +    AVOpenCLExternalInfo *ext = (AVOpenCLExternalInfo
> *)av_mallocz(sizeof(AVOpenCLExternalInfo));
>
> unneeded cast
>
>
> > +    if (!ext) {
> > +        av_log(&openclutils, AV_LOG_ERROR,
> > +         "Could not malloc external opencl environment data space\n");
> > +    }
> > +    return ext;
> > +}
> > +
>
> > +void av_opencl_free_external_environment(AVOpenCLExternalInfo *ext)
> > +{
> > +    av_free(ext);
> > +}
>
> A release function similar to av_freep() would safer as it also zeros
> the pointer
>
>
> > +
> > +AVUserSpecificDevInfo *av_opencl_malloc_user_spec_dev_info(void)
> > +{
> > +    AVUserSpecificDevInfo *dev_info = (AVUserSpecificDevInfo
> *)av_mallocz(sizeof(AVUserSpecificDevInfo));
>
> unneeded cast
>
>
> > +    if (!dev_info) {
> > +        av_log(&openclutils, AV_LOG_ERROR, "Could not malloc device
> info data space\n");
> > +    }
> > +    return dev_info;
> > +}
> > +
>
> > +void av_opencl_free_user_spec_dev_info(AVUserSpecificDevInfo *dev_info)
> > +{
> > +    av_free(dev_info);
> > +}
>
> A release function similar to av_freep() would safer as it also zeros
> the pointer
>
>
> > +
> > +int av_opencl_register_kernel_code(const char *kernel_code)
> > +{
> > +    int ret = 0;
> > +    int i;
> > +    LOCK_OPENCL;
> > +    if (gpu_env.kernel_code_count >= MAX_REG_NUM) {
> > +        av_log(&openclutils, AV_LOG_ERROR,
> > +         "Could not register kernel code, maximum number of registered
> kernel code %d already reached\n",
> > +         MAX_REG_NUM);
> > +        ret = AVERROR(EINVAL);
> > +        goto end;
> > +    }
> > +    for (i = 0;i < gpu_env.kernel_code_count;i++) {
> > +        if (gpu_env.kernel_code[i] == kernel_code) {
> > +            av_log(&openclutils, AV_LOG_WARNING, "Same kernel code has
> been registered\n");
> > +            goto end;
> > +        }
> > +    }
> > +    gpu_env.kernel_code[gpu_env.kernel_code_count] = kernel_code;
> > +    gpu_env.kernel_code_count++;
> > +end:
> > +    UNLOCK_OPENCL;
> > +    return ret;
> > +}
>
> iam not sure droping the kernel name is a good idea, detecting
> duplicated registrations now only works if the duplicates are
> represented by the same pointer
>
it is not the kernel name, it is kernel code, it is enough to be compile
once. kerenel code should be save in a char string and pass it go GPU and
compiled.So I check whether it has been registered.

>
>
> [...]
> > +int av_opencl_init(AVDictionary *options, AVOpenCLExternalInfo
> *ext_opencl_info, AVUserSpecificDevInfo *user_specific_dev_info)
> > +{
> > +    int ret = 0;
> > +    LOCK_OPENCL
> > +    if (!gpu_env.opencl_is_inited) {
> > +        /*initialize devices, context, command_queue*/
> > +        AVDictionaryEntry *opt_entry = av_dict_get(options,
> "build_option", NULL, 0);
>
> > +        if (user_specific_dev_info) {
> > +            gpu_env.user_spec_dev_flag = 1;
> > +            gpu_env.usr_spec_dev_info.dev_idx =
> user_specific_dev_info->dev_idx;
> > +            gpu_env.usr_spec_dev_info.platform_idx=
> user_specific_dev_info->platform_idx;
> > +        }
>
> Why is AVUserSpecificDevInfo used here and not the AVDictionary ?
> (maybe theres a reason, but it seems unneeded to use 2 different
>  systems)
>
>
> [...]
> > +/**
> > + * Initialize the run time OpenCL environment.
> > + *
> > + * This must be done after all the kernels have been registered with
> > + * the av_opencl_register_kernel_code().
>
> Thats not so easy
>
> consider that libavutil is used by 2 libraries
> both libraries register their kernels and then init them
> its hard to gurantee that this happens in order
> consider one lib to be avfilter and one avcodec
> The user application doesnt (need to) know about open cl it just knows
> about the avcodec and avfilter APIs and and may very well register
> and init one before registering anything from the other.
>
> also consider that libavcodec and libavfilter themselfs can be used
> by other libs, it quickly gets hard to ensure their register and
> init happens in order in the final application
>
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> it is not once nor twice but times without number that the same ideas make
> their appearance in the world. -- Aristotle
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>