| Message ID | 20210310215446.1396386-6-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Commit | 639b3bbbc7a3736db9255ad6da2e513a529fb4d3 |
| Headers | show |
| Series | [FFmpeg-devel,1/8] avcodec/cbs: Remove redundant checks for CodedBitstreamContext.codec | expand |
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > Andreas Rheinhardt > Sent: 2021年3月11日 5:55 > To: ffmpeg-devel@ffmpeg.org > Cc: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > Subject: [FFmpeg-devel] [PATCH 15/23] > dnn/dnn_backend_native_layer_conv2d: Join two arrays, avoid allocation > > Fixes Coverity issue #1473507. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavfilter/dnn/dnn_backend_native_layer_conv2d.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/libavfilter/dnn/dnn_backend_native_layer_conv2d.c > b/libavfilter/dnn/dnn_backend_native_layer_conv2d.c > index 2b83896da9..2e5aacdc2b 100644 > --- a/libavfilter/dnn/dnn_backend_native_layer_conv2d.c > +++ b/libavfilter/dnn/dnn_backend_native_layer_conv2d.c > @@ -38,6 +38,9 @@ typedef struct ThreadCommonParam{ typedef struct > ThreadParam{ > ThreadCommonParam *thread_common_param; > int thread_start, thread_end; > +#if HAVE_PTHREAD_CANCEL > + pthread_t thread; > +#endif > } ThreadParam; > > int ff_dnn_load_layer_conv2d(Layer *layer, AVIOContext *model_file_context, > int file_size, int operands_num) @@ -187,7 +190,6 @@ int > ff_dnn_execute_layer_conv2d(DnnOperand *operands, const int32_t > *input_opera > int thread_num = (ctx->options.conv2d_threads <= 0 || > ctx->options.conv2d_threads > av_cpu_count()) > ? (av_cpu_count() + 1) : (ctx->options.conv2d_threads); #if > HAVE_PTHREAD_CANCEL > - pthread_t *thread_id = av_malloc_array(thread_num, sizeof(*thread_id)); > int thread_stride; > #endif > ThreadParam **thread_param = av_malloc_array(thread_num, > sizeof(*thread_param)); @@ -230,17 +232,15 @@ int > ff_dnn_execute_layer_conv2d(DnnOperand *operands, const int32_t > *input_opera > thread_param[i]->thread_common_param = > &thread_common_param; > thread_param[i]->thread_start = thread_stride * i + pad_size; > thread_param[i]->thread_end = (i == thread_num - 1) ? (height - > pad_size) : (thread_param[i]->thread_start + thread_stride); > - pthread_create(&thread_id[i], NULL, > dnn_execute_layer_conv2d_thread, (void *)thread_param[i]); > + pthread_create(&thread_param[i]->thread, NULL, > + dnn_execute_layer_conv2d_thread, (void *)thread_param[i]); > } > > //join threads, res gets function return > for (int i = 0; i < thread_num; i++){ > - pthread_join(thread_id[i], NULL); > + pthread_join(thread_param[i]->thread, NULL); > } > > //release memory > - av_freep(&thread_id); > - > for (int i = 0; i < thread_num; i++){ > av_freep(&thread_param[i]); > } LGMT, and just one question: shall we reduce the allocation as less as possible? thanks.
Guo, Yejun: > > >> -----Original Message----- >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of >> Andreas Rheinhardt >> Sent: 2021年3月11日 5:55 >> To: ffmpeg-devel@ffmpeg.org >> Cc: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> Subject: [FFmpeg-devel] [PATCH 15/23] >> dnn/dnn_backend_native_layer_conv2d: Join two arrays, avoid allocation >> >> Fixes Coverity issue #1473507. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> libavfilter/dnn/dnn_backend_native_layer_conv2d.c | 10 +++++----- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/libavfilter/dnn/dnn_backend_native_layer_conv2d.c >> b/libavfilter/dnn/dnn_backend_native_layer_conv2d.c >> index 2b83896da9..2e5aacdc2b 100644 >> --- a/libavfilter/dnn/dnn_backend_native_layer_conv2d.c >> +++ b/libavfilter/dnn/dnn_backend_native_layer_conv2d.c >> @@ -38,6 +38,9 @@ typedef struct ThreadCommonParam{ typedef struct >> ThreadParam{ >> ThreadCommonParam *thread_common_param; >> int thread_start, thread_end; >> +#if HAVE_PTHREAD_CANCEL >> + pthread_t thread; >> +#endif >> } ThreadParam; >> >> int ff_dnn_load_layer_conv2d(Layer *layer, AVIOContext *model_file_context, >> int file_size, int operands_num) @@ -187,7 +190,6 @@ int >> ff_dnn_execute_layer_conv2d(DnnOperand *operands, const int32_t >> *input_opera >> int thread_num = (ctx->options.conv2d_threads <= 0 || >> ctx->options.conv2d_threads > av_cpu_count()) >> ? (av_cpu_count() + 1) : (ctx->options.conv2d_threads); #if >> HAVE_PTHREAD_CANCEL >> - pthread_t *thread_id = av_malloc_array(thread_num, sizeof(*thread_id)); >> int thread_stride; >> #endif >> ThreadParam **thread_param = av_malloc_array(thread_num, >> sizeof(*thread_param)); @@ -230,17 +232,15 @@ int >> ff_dnn_execute_layer_conv2d(DnnOperand *operands, const int32_t >> *input_opera >> thread_param[i]->thread_common_param = >> &thread_common_param; >> thread_param[i]->thread_start = thread_stride * i + pad_size; >> thread_param[i]->thread_end = (i == thread_num - 1) ? (height - >> pad_size) : (thread_param[i]->thread_start + thread_stride); >> - pthread_create(&thread_id[i], NULL, >> dnn_execute_layer_conv2d_thread, (void *)thread_param[i]); >> + pthread_create(&thread_param[i]->thread, NULL, >> + dnn_execute_layer_conv2d_thread, (void *)thread_param[i]); >> } >> >> //join threads, res gets function return >> for (int i = 0; i < thread_num; i++){ >> - pthread_join(thread_id[i], NULL); >> + pthread_join(thread_param[i]->thread, NULL); >> } >> >> //release memory >> - av_freep(&thread_id); >> - >> for (int i = 0; i < thread_num; i++){ >> av_freep(&thread_param[i]); >> } > > LGMT, and just one question: shall we reduce the allocation as less as possible? thanks. > If it is doable without too much effort as is here, then the answer is yes. Actually, one could go even further than what this patchset did: One could keep the thread_param array instead of constantly allocating and freeing it. With much more effort, one could even keep the threads. Anyway, applied this patchset with the changes you requested. - Andreas
> -----Original Message----- > >> -----Original Message----- > >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > >> Andreas Rheinhardt > >> Sent: 2021年3月11日 5:55 > >> To: ffmpeg-devel@ffmpeg.org > >> Cc: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > >> Subject: [FFmpeg-devel] [PATCH 15/23] > >> dnn/dnn_backend_native_layer_conv2d: Join two arrays, avoid allocation > >> > >> Fixes Coverity issue #1473507. > >> > >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > >> --- > >> libavfilter/dnn/dnn_backend_native_layer_conv2d.c | 10 +++++----- > >> 1 file changed, 5 insertions(+), 5 deletions(-) > >> } > >> > >> //release memory > >> - av_freep(&thread_id); > >> - > >> for (int i = 0; i < thread_num; i++){ > >> av_freep(&thread_param[i]); > >> } > > > > LGMT, and just one question: shall we reduce the allocation as less as > possible? thanks. > > > If it is doable without too much effort as is here, then the answer is > yes. Actually, one could go even further than what this patchset did: > One could keep the thread_param array instead of constantly allocating > and freeing it. With much more effort, one could even keep the threads. thanks, and is the reason that: we need to check the return value for dynamic allocation, and also need some effort to make sure to free it for all possible paths in the code followed?
Guo, Yejun: > > >> -----Original Message----- >>>> -----Original Message----- >>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of >>>> Andreas Rheinhardt >>>> Sent: 2021年3月11日 5:55 >>>> To: ffmpeg-devel@ffmpeg.org >>>> Cc: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >>>> Subject: [FFmpeg-devel] [PATCH 15/23] >>>> dnn/dnn_backend_native_layer_conv2d: Join two arrays, avoid allocation >>>> >>>> Fixes Coverity issue #1473507. >>>> >>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >>>> --- >>>> libavfilter/dnn/dnn_backend_native_layer_conv2d.c | 10 +++++----- >>>> 1 file changed, 5 insertions(+), 5 deletions(-) >>>> } >>>> >>>> //release memory >>>> - av_freep(&thread_id); >>>> - >>>> for (int i = 0; i < thread_num; i++){ >>>> av_freep(&thread_param[i]); >>>> } >>> >>> LGMT, and just one question: shall we reduce the allocation as less as >> possible? thanks. >>> >> If it is doable without too much effort as is here, then the answer is >> yes. Actually, one could go even further than what this patchset did: >> One could keep the thread_param array instead of constantly allocating >> and freeing it. With much more effort, one could even keep the threads. > > thanks, and is the reason that: we need to check the return value for dynamic > allocation, and also need some effort to make sure to free it for all possible > paths in the code followed? > Yes, reducing the amount of allocations also reduces the amount of checks and frees one has to perform. You can see the latter in this patchset, namely in this patch and in the one that stopped allocating thread_param separately (which enabled to remove the loop for freeing them). - Andreas
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > Andreas Rheinhardt > Sent: 2021年3月12日 15:59 > To: ffmpeg-devel@ffmpeg.org > Subject: Re: [FFmpeg-devel] [PATCH 15/23] > dnn/dnn_backend_native_layer_conv2d: Join two arrays, avoid allocation > > Guo, Yejun: > > > > > >> -----Original Message----- > >>>> -----Original Message----- > >>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > >>>> Andreas Rheinhardt > >>>> Sent: 2021年3月11日 5:55 > >>>> To: ffmpeg-devel@ffmpeg.org > >>>> Cc: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > >>>> Subject: [FFmpeg-devel] [PATCH 15/23] > >>>> dnn/dnn_backend_native_layer_conv2d: Join two arrays, avoid allocation > >>>> > >>>> Fixes Coverity issue #1473507. > >>>> > >>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > >>>> --- > >>>> libavfilter/dnn/dnn_backend_native_layer_conv2d.c | 10 +++++----- > >>>> 1 file changed, 5 insertions(+), 5 deletions(-) > >>>> } > >>>> > >>>> //release memory > >>>> - av_freep(&thread_id); > >>>> - > >>>> for (int i = 0; i < thread_num; i++){ > >>>> av_freep(&thread_param[i]); > >>>> } > >>> > >>> LGMT, and just one question: shall we reduce the allocation as less as > >> possible? thanks. > >>> > >> If it is doable without too much effort as is here, then the answer is > >> yes. Actually, one could go even further than what this patchset did: > >> One could keep the thread_param array instead of constantly allocating > >> and freeing it. With much more effort, one could even keep the threads. > > > > thanks, and is the reason that: we need to check the return value for > dynamic > > allocation, and also need some effort to make sure to free it for all possible > > paths in the code followed? > > > Yes, reducing the amount of allocations also reduces the amount of > checks and frees one has to perform. You can see the latter in this > patchset, namely in this patch and in the one that stopped allocating > thread_param separately (which enabled to remove the loop for freeing them). got it, thanks.
diff --git a/libavfilter/dnn/dnn_backend_native_layer_conv2d.c b/libavfilter/dnn/dnn_backend_native_layer_conv2d.c index 2b83896da9..2e5aacdc2b 100644 --- a/libavfilter/dnn/dnn_backend_native_layer_conv2d.c +++ b/libavfilter/dnn/dnn_backend_native_layer_conv2d.c @@ -38,6 +38,9 @@ typedef struct ThreadCommonParam{ typedef struct ThreadParam{ ThreadCommonParam *thread_common_param; int thread_start, thread_end; +#if HAVE_PTHREAD_CANCEL + pthread_t thread; +#endif } ThreadParam; int ff_dnn_load_layer_conv2d(Layer *layer, AVIOContext *model_file_context, int file_size, int operands_num) @@ -187,7 +190,6 @@ int ff_dnn_execute_layer_conv2d(DnnOperand *operands, const int32_t *input_opera int thread_num = (ctx->options.conv2d_threads <= 0 || ctx->options.conv2d_threads > av_cpu_count()) ? (av_cpu_count() + 1) : (ctx->options.conv2d_threads); #if HAVE_PTHREAD_CANCEL - pthread_t *thread_id = av_malloc_array(thread_num, sizeof(*thread_id)); int thread_stride; #endif ThreadParam **thread_param = av_malloc_array(thread_num, sizeof(*thread_param)); @@ -230,17 +232,15 @@ int ff_dnn_execute_layer_conv2d(DnnOperand *operands, const int32_t *input_opera thread_param[i]->thread_common_param = &thread_common_param; thread_param[i]->thread_start = thread_stride * i + pad_size; thread_param[i]->thread_end = (i == thread_num - 1) ? (height - pad_size) : (thread_param[i]->thread_start + thread_stride); - pthread_create(&thread_id[i], NULL, dnn_execute_layer_conv2d_thread, (void *)thread_param[i]); + pthread_create(&thread_param[i]->thread, NULL, dnn_execute_layer_conv2d_thread, (void *)thread_param[i]); } //join threads, res gets function return for (int i = 0; i < thread_num; i++){ - pthread_join(thread_id[i], NULL); + pthread_join(thread_param[i]->thread, NULL); } //release memory - av_freep(&thread_id); - for (int i = 0; i < thread_num; i++){ av_freep(&thread_param[i]); }
Fixes Coverity issue #1473507. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- libavfilter/dnn/dnn_backend_native_layer_conv2d.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)