| Message ID | 20200821114727.29233-1-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Commit | 128e6df1cd79076e3d5f51bbc88607b3d1c62689 |
| Headers | show |
| Series | [FFmpeg-devel] dnn_backend_native_layer_avgpool: Fix invalid assignment, use av_assert | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > Andreas Rheinhardt > Sent: 2020年8月21日 19:47 > To: ffmpeg-devel@ffmpeg.org > Cc: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > Subject: [FFmpeg-devel] [PATCH] dnn_backend_native_layer_avgpool: Fix invalid > assignment, use av_assert > > dnn_execute_layer_avg_pool() contains the following line: > > assert(avgpool_params->padding_method = VALID); > > This statement contains an assignment where obviously a comparison was > intended. Furthermore, *avgpool_params is const, so that the attempted > assignment leads to a compilation failure if asserts are enabled (i.e. if DEBUG is > defined which leads libavutil/internal.h to not define NDEBUG). Moreover, the > enumeration constant VALID actually has the value 0, so that the assert would > be triggered if a compiler compiles this with asserts enabled. Finally, the > statement uses assert() directly instead of av_assert*(). > > All these errors have been fixed. > > Thanks to ubitux for providing a FATE-box [1] where DEBUG is defined. > > [1]: http://fate.ffmpeg.org/history.cgi?slot=x86_64-archlinux-gcc-ddebug > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > I was unsure which assert level to use and therefore simply opted for 0. > > libavfilter/dnn/dnn_backend_native_layer_avgpool.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavfilter/dnn/dnn_backend_native_layer_avgpool.c > b/libavfilter/dnn/dnn_backend_native_layer_avgpool.c > index d745c35b4a..8d4d8db98c 100644 > --- a/libavfilter/dnn/dnn_backend_native_layer_avgpool.c > +++ b/libavfilter/dnn/dnn_backend_native_layer_avgpool.c > @@ -91,7 +91,7 @@ int dnn_execute_layer_avg_pool(DnnOperand *operands, > const int32_t *input_operan > output_height = ceil(height / (kernel_strides * 1.0)); > output_width = ceil(width / (kernel_strides * 1.0)); > } else { > - assert(avgpool_params->padding_method = VALID); > + av_assert0(avgpool_params->padding_method == VALID); > height_end = height - avgpool_params->kernel_size + 1; > width_end = width - avgpool_params->kernel_size + 1; > height_radius = 0; thanks for the patch, will push now.
Guo, Yejun: > > >> -----Original Message----- >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of >> Andreas Rheinhardt >> Sent: 2020年8月21日 19:47 >> To: ffmpeg-devel@ffmpeg.org >> Cc: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> Subject: [FFmpeg-devel] [PATCH] dnn_backend_native_layer_avgpool: Fix invalid >> assignment, use av_assert >> >> dnn_execute_layer_avg_pool() contains the following line: >> >> assert(avgpool_params->padding_method = VALID); >> >> This statement contains an assignment where obviously a comparison was >> intended. Furthermore, *avgpool_params is const, so that the attempted >> assignment leads to a compilation failure if asserts are enabled (i.e. if DEBUG is >> defined which leads libavutil/internal.h to not define NDEBUG). Moreover, the >> enumeration constant VALID actually has the value 0, so that the assert would >> be triggered if a compiler compiles this with asserts enabled. Finally, the >> statement uses assert() directly instead of av_assert*(). >> >> All these errors have been fixed. >> >> Thanks to ubitux for providing a FATE-box [1] where DEBUG is defined. >> >> [1]: http://fate.ffmpeg.org/history.cgi?slot=x86_64-archlinux-gcc-ddebug >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> I was unsure which assert level to use and therefore simply opted for 0. >> >> libavfilter/dnn/dnn_backend_native_layer_avgpool.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/libavfilter/dnn/dnn_backend_native_layer_avgpool.c >> b/libavfilter/dnn/dnn_backend_native_layer_avgpool.c >> index d745c35b4a..8d4d8db98c 100644 >> --- a/libavfilter/dnn/dnn_backend_native_layer_avgpool.c >> +++ b/libavfilter/dnn/dnn_backend_native_layer_avgpool.c >> @@ -91,7 +91,7 @@ int dnn_execute_layer_avg_pool(DnnOperand *operands, >> const int32_t *input_operan >> output_height = ceil(height / (kernel_strides * 1.0)); >> output_width = ceil(width / (kernel_strides * 1.0)); >> } else { >> - assert(avgpool_params->padding_method = VALID); >> + av_assert0(avgpool_params->padding_method == VALID); >> height_end = height - avgpool_params->kernel_size + 1; >> width_end = width - avgpool_params->kernel_size + 1; >> height_radius = 0; > > thanks for the patch, will push now. I actually push my patches myself. Too late now. - Andreas
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > Andreas Rheinhardt > Sent: 2020年8月21日 22:21 > To: ffmpeg-devel@ffmpeg.org > Subject: Re: [FFmpeg-devel] [PATCH] dnn_backend_native_layer_avgpool: Fix > invalid assignment, use av_assert > > Guo, Yejun: > > > > > >> -----Original Message----- > >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > >> Andreas Rheinhardt > >> Sent: 2020年8月21日 19:47 > >> To: ffmpeg-devel@ffmpeg.org > >> Cc: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > >> Subject: [FFmpeg-devel] [PATCH] dnn_backend_native_layer_avgpool: Fix > >> invalid assignment, use av_assert > >> > >> dnn_execute_layer_avg_pool() contains the following line: > >> > >> assert(avgpool_params->padding_method = VALID); > >> > >> This statement contains an assignment where obviously a comparison > >> was intended. Furthermore, *avgpool_params is const, so that the > >> attempted assignment leads to a compilation failure if asserts are > >> enabled (i.e. if DEBUG is defined which leads libavutil/internal.h to > >> not define NDEBUG). Moreover, the enumeration constant VALID actually > >> has the value 0, so that the assert would be triggered if a compiler > >> compiles this with asserts enabled. Finally, the statement uses assert() > directly instead of av_assert*(). > >> > >> All these errors have been fixed. > >> > >> Thanks to ubitux for providing a FATE-box [1] where DEBUG is defined. > >> > >> [1]: > >> http://fate.ffmpeg.org/history.cgi?slot=x86_64-archlinux-gcc-ddebug > >> > >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > >> --- > >> I was unsure which assert level to use and therefore simply opted for 0. > >> > >> libavfilter/dnn/dnn_backend_native_layer_avgpool.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/libavfilter/dnn/dnn_backend_native_layer_avgpool.c > >> b/libavfilter/dnn/dnn_backend_native_layer_avgpool.c > >> index d745c35b4a..8d4d8db98c 100644 > >> --- a/libavfilter/dnn/dnn_backend_native_layer_avgpool.c > >> +++ b/libavfilter/dnn/dnn_backend_native_layer_avgpool.c > >> @@ -91,7 +91,7 @@ int dnn_execute_layer_avg_pool(DnnOperand > >> *operands, const int32_t *input_operan > >> output_height = ceil(height / (kernel_strides * 1.0)); > >> output_width = ceil(width / (kernel_strides * 1.0)); > >> } else { > >> - assert(avgpool_params->padding_method = VALID); > >> + av_assert0(avgpool_params->padding_method == VALID); > >> height_end = height - avgpool_params->kernel_size + 1; > >> width_end = width - avgpool_params->kernel_size + 1; > >> height_radius = 0; > > > > thanks for the patch, will push now. > > I actually push my patches myself. Too late now. > sure, no problem for the push. Just found that my push command works, so I checked the repo at https://git.ffmpeg.org/gitweb/ffmpeg.git/commit/128e6df1cd79076e3d5f51bbc88607b3d1c62689, looks that your push does not work for some reason. just fyi.
Guo, Yejun: > > >> -----Original Message----- >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of >> Andreas Rheinhardt >> Sent: 2020年8月21日 22:21 >> To: ffmpeg-devel@ffmpeg.org >> Subject: Re: [FFmpeg-devel] [PATCH] dnn_backend_native_layer_avgpool: Fix >> invalid assignment, use av_assert >> >> Guo, Yejun: >>> >>> >>>> -----Original Message----- >>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of >>>> Andreas Rheinhardt >>>> Sent: 2020年8月21日 19:47 >>>> To: ffmpeg-devel@ffmpeg.org >>>> Cc: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >>>> Subject: [FFmpeg-devel] [PATCH] dnn_backend_native_layer_avgpool: Fix >>>> invalid assignment, use av_assert >>>> >>>> dnn_execute_layer_avg_pool() contains the following line: >>>> >>>> assert(avgpool_params->padding_method = VALID); >>>> >>>> This statement contains an assignment where obviously a comparison >>>> was intended. Furthermore, *avgpool_params is const, so that the >>>> attempted assignment leads to a compilation failure if asserts are >>>> enabled (i.e. if DEBUG is defined which leads libavutil/internal.h to >>>> not define NDEBUG). Moreover, the enumeration constant VALID actually >>>> has the value 0, so that the assert would be triggered if a compiler >>>> compiles this with asserts enabled. Finally, the statement uses assert() >> directly instead of av_assert*(). >>>> >>>> All these errors have been fixed. >>>> >>>> Thanks to ubitux for providing a FATE-box [1] where DEBUG is defined. >>>> >>>> [1]: >>>> http://fate.ffmpeg.org/history.cgi?slot=x86_64-archlinux-gcc-ddebug >>>> >>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >>>> --- >>>> I was unsure which assert level to use and therefore simply opted for 0. >>>> >>>> libavfilter/dnn/dnn_backend_native_layer_avgpool.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/libavfilter/dnn/dnn_backend_native_layer_avgpool.c >>>> b/libavfilter/dnn/dnn_backend_native_layer_avgpool.c >>>> index d745c35b4a..8d4d8db98c 100644 >>>> --- a/libavfilter/dnn/dnn_backend_native_layer_avgpool.c >>>> +++ b/libavfilter/dnn/dnn_backend_native_layer_avgpool.c >>>> @@ -91,7 +91,7 @@ int dnn_execute_layer_avg_pool(DnnOperand >>>> *operands, const int32_t *input_operan >>>> output_height = ceil(height / (kernel_strides * 1.0)); >>>> output_width = ceil(width / (kernel_strides * 1.0)); >>>> } else { >>>> - assert(avgpool_params->padding_method = VALID); >>>> + av_assert0(avgpool_params->padding_method == VALID); >>>> height_end = height - avgpool_params->kernel_size + 1; >>>> width_end = width - avgpool_params->kernel_size + 1; >>>> height_radius = 0; >>> >>> thanks for the patch, will push now. >> >> I actually push my patches myself. Too late now. >> > > sure, no problem for the push. > > Just found that my push command works, so I checked the repo at > https://git.ffmpeg.org/gitweb/ffmpeg.git/commit/128e6df1cd79076e3d5f51bbc88607b3d1c62689, > looks that your push does not work for some reason. just fyi. I had no problem with my push; it's just that you had already pushed before I realized your approval and then it was of course too late (so I never attempted to push this). But as has already been said: Too late now. - Andreas
diff --git a/libavfilter/dnn/dnn_backend_native_layer_avgpool.c b/libavfilter/dnn/dnn_backend_native_layer_avgpool.c index d745c35b4a..8d4d8db98c 100644 --- a/libavfilter/dnn/dnn_backend_native_layer_avgpool.c +++ b/libavfilter/dnn/dnn_backend_native_layer_avgpool.c @@ -91,7 +91,7 @@ int dnn_execute_layer_avg_pool(DnnOperand *operands, const int32_t *input_operan output_height = ceil(height / (kernel_strides * 1.0)); output_width = ceil(width / (kernel_strides * 1.0)); } else { - assert(avgpool_params->padding_method = VALID); + av_assert0(avgpool_params->padding_method == VALID); height_end = height - avgpool_params->kernel_size + 1; width_end = width - avgpool_params->kernel_size + 1; height_radius = 0;
dnn_execute_layer_avg_pool() contains the following line: assert(avgpool_params->padding_method = VALID); This statement contains an assignment where obviously a comparison was intended. Furthermore, *avgpool_params is const, so that the attempted assignment leads to a compilation failure if asserts are enabled (i.e. if DEBUG is defined which leads libavutil/internal.h to not define NDEBUG). Moreover, the enumeration constant VALID actually has the value 0, so that the assert would be triggered if a compiler compiles this with asserts enabled. Finally, the statement uses assert() directly instead of av_assert*(). All these errors have been fixed. Thanks to ubitux for providing a FATE-box [1] where DEBUG is defined. [1]: http://fate.ffmpeg.org/history.cgi?slot=x86_64-archlinux-gcc-ddebug Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- I was unsure which assert level to use and therefore simply opted for 0. libavfilter/dnn/dnn_backend_native_layer_avgpool.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)