[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.

> > +
> > +        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.

> 
> > +            return AVERROR(ENOMEM);
> > +        }
> > +    }
> > +
> > +    fclose(file);
> > +    return 0;
> > +}
> > +