[FFmpeg-devel] [PATCH] lavfi: add helper macro for OpenCL error handling.

[Mark Thompson]

On 15/06/18 03:36, Dan Yaschenko wrote:
> On 12 June 2018 at 10:20, Ruiling Song <ruiling.song at intel.com> wrote:
> 
>> Signed-off-by: Ruiling Song <ruiling.song at intel.com>
>> ---
>> I am not sure whether do you think this would be useful?
>> the main purpose is to make OpenCL error check code simpler.
>> If we think this is good, I can go to replace current
>> OpenCL filters to use this macro.
> 
> 
>> for example:
>>         if (cle != CL_SUCCESS) {
>>             av_log(avctx, AV_LOG_ERROR, "Failed to enqueue kernel: %d.\n",
>>                    cle);
>>             err = AVERROR(EIO);
>>             goto fail;
>>         }
>> can be replaced with:
>> OCL_FAIL_ON_ERR(avctx, cle, AVERROR(EIO), "Failed to enqueue kernel:
>> %d.\n", cle);
>>
>> Thanks!
>> Ruiling
>>  libavfilter/opencl.h | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/libavfilter/opencl.h b/libavfilter/opencl.h
>> index c0a4519..c33df1c 100644
>> --- a/libavfilter/opencl.h
>> +++ b/libavfilter/opencl.h
>> @@ -97,5 +97,16 @@ int ff_opencl_filter_work_size_from_image(AVFilterContext
>> *avctx,
>>                                            size_t *work_size,
>>                                            AVFrame *frame, int plane,
>>                                            int block_alignment);
>> +/**
>> + * A helper macro to handle OpenCL error. It will assign errcode to
>> + * variable err, log error msg, and jump to fail label on error.
>> + */
>> +#define OCL_FAIL_ON_ERR(logctx, cle, errcode, ...) do {\
>> +    if (cle != CL_SUCCESS) {\
>> +        av_log(logctx, AV_LOG_ERROR, __VA_ARGS__);\
>> +        err = errcode;\
>> +        goto fail;\
>> +    }\
>> +} while(0)
>>
>>  #endif /* AVFILTER_OPENCL_H */
>> --
>> 2.7.4
>>
> 
> Hi!
> I like your idea, but still don't know which one is better.
> My idea relies on fact, that there are only few OpenCL functions which are
> used multiple times in filters:
> setKernelArg, clCreateKernel(in case when there are multiple kernels) and
> maybe
> clEnqueueNDRangeKernel. So that is why my purpose is totally wrap them and
> significantly reduce code,
> but yes, there are some restrictions, like you can't use kernel_arg++ when
> setting kernel arguments.
> And still most of cl-error checking statements appear after using
> cl-functions listed above.

The specific one for kernel args is the largest source of extra boilerplate, so I've applied it already.  (Even with the more general setup it would still make sense for a separate macro to exist to do the kernel arguments.)

This more general case seems reasonable to me if you'd like to implement it.  To make it slightly simpler, it's probably ok to assume that the first two arguments have the fixed names used in all current filters ("avctx" for the AVFilterContext, "cle" for the CL error code we want to check).  I'd probably also make it "CL_FAIL_ON_ERROR" rather than "OCL_FAIL_ON_ERR" to better match existing style.

Thanks,

- Mark