[FFmpeg-devel] [PATCH] hwcontext_opencl: Use correct function to enumerate devices

[Mark Thompson]

On 28/11/2018 00:50, Song, Ruiling wrote:
>> -----Original Message-----
>> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf Of
>> Mark Thompson
>> Sent: Wednesday, November 28, 2018 8:17 AM
>> To: ffmpeg-devel at ffmpeg.org
>> Subject: [FFmpeg-devel] [PATCH] hwcontext_opencl: Use correct function to
>> enumerate devices
>>
>> Also assert that all required functions are present.
>> ---
>> On 26/11/2018 08:57, Song, Ruiling wrote:
>>>> -----Original Message-----
>>>> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf
>> Of
>>>> Mark Thompson
>>>> Sent: Monday, November 26, 2018 6:08 AM
>>>> To: FFmpeg development discussions and patches <ffmpeg-
>> devel at ffmpeg.org>
>>>> Subject: [FFmpeg-devel] [PATCH] hwcontext_opencl: Use correct function to
>>>> enumerate devices
>>>>
>>>> ---
>>>>  libavutil/hwcontext_opencl.c | 6 +++---
>>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/libavutil/hwcontext_opencl.c b/libavutil/hwcontext_opencl.c
>>>> index e6cef74269..6a26354c87 100644
>>>> --- a/libavutil/hwcontext_opencl.c
>>>> +++ b/libavutil/hwcontext_opencl.c
>>>> @@ -542,9 +542,9 @@ static int
>>>> opencl_device_create_internal(AVHWDeviceContext *hwdev,
>>>>                  continue;
>>>>          }
>>>>
>>>> -        err = opencl_enumerate_devices(hwdev, platforms[p], platform_name,
>>>> -                                       &nb_devices, &devices,
>>>> -                                       selector->context);
>>>> +        err = selector->enumerate_devices(hwdev, platforms[p],
>> platform_name,
>>>> +                                          &nb_devices, &devices,
>>>> +                                          selector->context);
>>> I think it is better to check enumerate_devices  against null pointer before
>> calling it, although it should works well currently.
>>
>> The two enumerate functions should always be set when entering the function,
>> since they are always required.  (Unlike the filter, where "do nothing" is a
>> reasonable case.)
>>
>> How about an assert at the start of the function to check that, like this?
> Yes, that's good. It is just helpful when somebody add new platform support but happened to forget to give a meaningful function pointer.

Yep, exactly so :)

Applied.

Thanks,

- Mark