[FFmpeg-devel] [PATCH V3 3/3] libavfilter: add filter dnn_detect for object detection
Guo, Yejun
yejun.guo at intel.com
Mon Mar 1 13:47:13 EET 2021
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> Andreas Rheinhardt
> Sent: 2021年3月1日 16:31
> To: ffmpeg-devel at ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH V3 3/3] libavfilter: add filter dnn_detect
> for object detection
>
> Guo, Yejun:
> >
> >
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> >> Andreas Rheinhardt
> >> Sent: 2021年3月1日 12:50
> >> To: ffmpeg-devel at ffmpeg.org
> >> Subject: Re: [FFmpeg-devel] [PATCH V3 3/3] libavfilter: add filter
> >> dnn_detect for object detection
> >>
> >> Guo, Yejun:
> >>> Signed-off-by: Guo, Yejun <yejun.guo at intel.com>
> >>> ---
> >>> configure | 1 +
> >>> doc/filters.texi | 40 +++
> >>> libavfilter/Makefile | 1 +
> >>> libavfilter/allfilters.c | 1 +
> >>> libavfilter/dnn/dnn_backend_openvino.c | 12 +
> >>> libavfilter/dnn_filter_common.c | 7 +
> >>> libavfilter/dnn_filter_common.h | 1 +
> >>> libavfilter/dnn_interface.h | 6 +-
> >>> libavfilter/vf_dnn_detect.c | 462
> >> +++++++++++++++++++++++++
> >>> 9 files changed, 529 insertions(+), 2 deletions(-) create mode
> >
> >>> b/libavfilter/dnn_interface.h index d3a0c58a61..90a08129f4 100644
> >>> --- a/libavfilter/dnn_interface.h
> >>> +++ b/libavfilter/dnn_interface.h
> >>> @@ -63,6 +63,8 @@ typedef struct DNNData{
> >>> DNNColorOrder order;
> >>> } DNNData;
> >>>
> >>> +typedef int (*PRE_POST_PROC)(AVFrame *frame, DNNData *model,
> >>> +AVFilterContext *filter_ctx);
> >>
> >> Why uppercase? It is a typedef, not a macro.
> >
> > will change to CamelCase, thanks.
> >
> >>
> >>> +
> >>> typedef struct DNNModel{
> >>> // Stores model that can be different for different backends.
> >>> void *model;
> >>> @@ -80,10 +82,10 @@ typedef struct DNNModel{
> >>> const char *output_name, int
> >> *output_width, int *output_height);
> >>> // set the pre process to transfer data from AVFrame to DNNData
> >>> // the default implementation within DNN is used if it is not
> >>> provided
> >> by the filter
> >>> - int (*pre_proc)(AVFrame *frame_in, DNNData *model_input,
> >> AVFilterContext *filter_ctx);
> >>> + PRE_POST_PROC pre_proc;
> >>> // set the post process to transfer data from DNNData to AVFrame
> >>> // the default implementation within DNN is used if it is not
> >>> provided
> >> by the filter
> >>> - int (*post_proc)(AVFrame *frame_out, DNNData *model_output,
> >> AVFilterContext *filter_ctx);
> >>> + PRE_POST_PROC post_proc;
> >>
> >> Spurious change.
> >
> > sorry, not quite understand this comment. It is used for code refine.
> > Maybe I need to let it in a separate patch.
> >
> >>
> >>> } DNNModel;
> >>>
> >>> +
> >>> + frame->private_ref = av_buffer_alloc(sizeof(*header) +
> >>> + sizeof(*bbox) *
> >> nb_bbox);
> >>> + if (!frame->private_ref) {
> >>> + av_log(filter_ctx, AV_LOG_ERROR, "failed to allocate buffer
> >>> + for %d
> >> bboxes\n", nb_bbox);
> >>> + return AVERROR(ENOMEM);;
> >>
> >> Double ;
> >
> > thanks, will remove it.
> >
> >>
> >>> + }
> >>> +
> >>> + header = (BoundingBoxHeader *)frame->private_ref->data;
> >>> + strncpy(header->source, ctx->dnnctx.model_filename,
> >> sizeof(header->source));
> >>> + header->source[sizeof(header->source) - 1] = '\0';
> >>> + header->bbox_size = sizeof(*bbox);
> >>> +
> >>> + bbox = (BoundingBox *)(header + 1);
> >>
> >> This does not guarantee proper alignment. You could use a flexible
> >> array member for that.
> >
> > The memory layout of frame->private_ref->data is:
> > sizeof(*header) + sizeof(*bbox) + sizeof(*bbox) + ...
> >
> > I think 'header + 1' goes correctly to the first bbox. thanks.
> >
>
> header + 1 points to the position where another BoundingBoxHeader would be
> if header were an array of BoundingBoxHeaders (i.e. it points
> sizeof(BoundingBoxHeader) bytes after header). So this guarantees that there is
> no overlap between your header and your actual boxes, but this does not
> guarantee proper alignment. This will be a problem on platforms where int is
> 64bit and requires eight byte alignment (unlikely) or if you add something that
> requires alignment of eight bytes (like a 64bit integer or a pointer on a 64bit
> system) to BoundingBox.
got the point, thanks. Will add 'BoundingBox bboxes[0]' at the end of struct BoundingBoxHeader.
>
> >>> +
> >>> + label = av_strdup(buf);
> >>> + if (!label) {
> >>> + av_log(context, AV_LOG_ERROR, "failed to allocate
> >>> + memory
> >> for label %s\n", buf);
> >>> + fclose(file);
> >>> + free_detect_labels(ctx);
> >>> + return AVERROR(ENOMEM);
> >>> + }
> >>> +
> >>> + if (av_dynarray_add_nofree(&ctx->labels, &ctx->label_count,
> >>> + label)
> >> < 0) {
> >>> + av_log(context, AV_LOG_ERROR, "failed to do
> >> av_dynarray_add\n");
> >>> + fclose(file);
> >>> + free_detect_labels(ctx);
> >>
> >> 1. You are leaking label here.
> >> 2. You are repeating yourself with the cleanup code.
> >> 3. When you return an error in a filter's init function, the filter's
> >> uninit function is called automatically. In this case this means that
> >> free_detect_labels is called twice which is not only wasteful, but
> >> harmful: You are freeing ctx->labels (and all labels contained in it)
> >> in the first run, but you are not resetting the number of labels. If
> >> ctx->label_count is > 0, there will be a segfault when
> >> free_detect_labels is called the second time.
> >
> > good catch, will free label, remove the duplicate calling, and also reset
> ctx->label_count.
> >
> >> 4. Return the error code.
> >> (5. I consider your use of av_log for every error to be excessive.)
> >
> > not quite understand this one, thanks.
> >
>
> It means that it is unnecessary to add an av_log for every failed allocation;
> returning the error code is enough.
>
> >>
> >>> + return AVERROR(ENOMEM);
> >>> + }
> >>> + }
> >>> +
> >>> + fclose(file);
> >>> + return 0;
> >>> +}
> >>> +
> _______________________________________________
> 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".
More information about the ffmpeg-devel
mailing list