Re: [FFmpeg-devel] [PATCH] hwcontext_opencl: Use correct function to enumerate devices
On 28/11/2018 00:50, Song, Ruiling wrote: >> -Original Message- >> From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@ffmpeg.org] On Behalf Of >> Mark Thompson >> Sent: Wednesday, November 28, 2018 8:17 AM >> To: ffmpeg-devel@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-boun...@ffmpeg.org] On Behalf >> Of >>>> Mark Thompson >>>> Sent: Monday, November 26, 2018 6:08 AM >>>> To: FFmpeg development discussions and patches > de...@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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] hwcontext_opencl: Use correct function to enumerate devices
> -Original Message- > From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@ffmpeg.org] On Behalf Of > Mark Thompson > Sent: Wednesday, November 28, 2018 8:17 AM > To: ffmpeg-devel@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-boun...@ffmpeg.org] On Behalf > Of > >> Mark Thompson > >> Sent: Monday, November 26, 2018 6:08 AM > >> To: FFmpeg development discussions and patches de...@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. Ruiling > > - Mark > > > libavutil/hwcontext_opencl.c | 9 ++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/libavutil/hwcontext_opencl.c b/libavutil/hwcontext_opencl.c > index be71c8323e..d3df6221c4 100644 > --- a/libavutil/hwcontext_opencl.c > +++ b/libavutil/hwcontext_opencl.c > @@ -500,6 +500,9 @@ static int > opencl_device_create_internal(AVHWDeviceContext *hwdev, > *device_name_src = NULL; > int err, found, p, d; > > +av_assert0(selector->enumerate_platforms && > + selector->enumerate_devices); > + > err = selector->enumerate_platforms(hwdev, &nb_platforms, &platforms, > selector->context); > if (err) > @@ -531,9 +534,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); > if (err < 0) > continue; > > -- > 2.19.1 > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[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-boun...@ffmpeg.org] On Behalf Of >> Mark Thompson >> Sent: Monday, November 26, 2018 6:08 AM >> To: FFmpeg development discussions and patches >> 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? - Mark libavutil/hwcontext_opencl.c | 9 ++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/libavutil/hwcontext_opencl.c b/libavutil/hwcontext_opencl.c index be71c8323e..d3df6221c4 100644 --- a/libavutil/hwcontext_opencl.c +++ b/libavutil/hwcontext_opencl.c @@ -500,6 +500,9 @@ static int opencl_device_create_internal(AVHWDeviceContext *hwdev, *device_name_src = NULL; int err, found, p, d; +av_assert0(selector->enumerate_platforms && + selector->enumerate_devices); + err = selector->enumerate_platforms(hwdev, &nb_platforms, &platforms, selector->context); if (err) @@ -531,9 +534,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); if (err < 0) continue; -- 2.19.1 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] hwcontext_opencl: Use correct function to enumerate devices
> -Original Message- > From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@ffmpeg.org] On Behalf Of > Mark Thompson > Sent: Monday, November 26, 2018 6:08 AM > To: FFmpeg development discussions and patches > 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. Ruiling > if (err < 0) > continue; > > -- > 2.19.1 > > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[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); if (err < 0) continue; -- 2.19.1 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
