diff mbox

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

Message ID 1528788032-18129-1-git-send-email-ruiling.song@intel.com
State New
Headers show

Commit Message

Ruiling Song June 12, 2018, 7:20 a.m. UTC
Signed-off-by: Ruiling Song <ruiling.song@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(+)

Comments

Danil Iashchenko June 15, 2018, 2:36 a.m. UTC | #1
On 12 June 2018 at 10:20, Ruiling Song <ruiling.song@intel.com> wrote:

> Signed-off-by: Ruiling Song <ruiling.song@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.

Thanks, Danil


>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Mark Thompson June 17, 2018, 6:17 p.m. UTC | #2
On 15/06/18 03:36, Dan Yaschenko wrote:
> On 12 June 2018 at 10:20, Ruiling Song <ruiling.song@intel.com> wrote:
> 
>> Signed-off-by: Ruiling Song <ruiling.song@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
Ruiling Song June 19, 2018, 1:53 a.m. UTC | #3
> -----Original Message-----

> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf Of

> Mark Thompson

> Sent: Monday, June 18, 2018 2:17 AM

> To: ffmpeg-devel@ffmpeg.org

> Subject: Re: [FFmpeg-devel] [PATCH] lavfi: add helper macro for OpenCL error

> handling.

> 

> On 15/06/18 03:36, Dan Yaschenko wrote:

> > On 12 June 2018 at 10:20, Ruiling Song <ruiling.song@intel.com> wrote:

> >

> >> Signed-off-by: Ruiling Song <ruiling.song@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.

Ok, I will submit a new patch later.

Ruiling
> 

> Thanks,

> 

> - Mark

> _______________________________________________

> ffmpeg-devel mailing list

> ffmpeg-devel@ffmpeg.org

> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
diff mbox

Patch

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 */