[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日 20:13
> 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日 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.
> >
> 
> An array with zero elements is not allowed in ISO C (see 6.7.6.2 1 in
> C11 or 6.7.5.2 1 in C99), although many compilers support it.

thanks for the info, this struct is expected to be in side_data in the future, 
I'll add 'bboxes[1]' in it, and allocate sizeof(*header) + (nb_bbox - 1) * sizeof(*bbox).