[FFmpeg-devel] [PATCH] lavfi: add nlmeans_opencl filter

[Song, Ruiling]

> -----Original Message-----
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf Of
> Mark Thompson
> Sent: Sunday, April 14, 2019 1:23 AM
> To: ffmpeg-devel at ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH] lavfi: add nlmeans_opencl filter
> 
> On 12/04/2019 08:38, Song, Ruiling wrote:
> >>>> +#define RELEASE_KERNEL(k)                                    \
> >>>> +do {                                                         \
> >>>> +    if (k) {                                                 \
> >>>> +        cle = clReleaseKernel(k);                            \
> >>>> +        if (cle != CL_SUCCESS)                               \
> >>>> +            av_log(avctx, AV_LOG_ERROR, "Failed to release " \
> >>>> +                   "kernel: %d.\n", cle);                    \
> >>>> +    }                                                        \
> >>>> +} while(0)
> >>>
> >>> This appears multiple times here and also in other filters.  Maybe it should
> be a
> >>> macro in opencl.h like CL_SET_KERNEL_ARG?
> > Hi Mark,
> >
> > I am rethinking about this problem, can we just simply call clReleaseKernel()
> and not checking the input and the error_code.
> > OpenCL spec has require implementation to check the input argument. So I
> think we can just ignore the if-null check.
> 
> I'm not sure that's true?  The spec allows a CL_INVALID_KERNEL error, but
> doesn't offer any clear indication of when it should be returned (NULL is
> distinguished in other cases, but not here).  Random pointers certainly do crash
> implementations, so they aren't interpreting it as a requirement to validate the
> pointer generally (against some list in the context, say).
Yes, seems the spec does not say about null pointer check clearly.
Because the null pointer check is cheap, so I thought every good programmed OpenCL driver should be able to check that.
Maybe you are right. I am not quite sure now:(
So we can keep the check as before. I have added the macro to do this. Please help take a look at V2 when you have time.

Thanks!
Ruiling
> 
> The standard ICD loader does have a null check returning CL_INVALID_KERNEL,
> but there is no requirement that it is used rather than linking to a particular ICD
> directly.
> 
> > As we are destroying the objects, is it still useful to care the error code
> returned?
> 
> Probably not, I agree.
> 
> - Mark
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".