[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:24
> 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日 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).
> 
> Notice that in this case it is undefined behaviour to access any of the
> boxes outside of BoundingBoxHeader (i.e. when using header->bboxes[i],
> the compiler is allowed to infer that i == 0 as all other cases would be
> undefined behaviour).
> 
> - Andreas

Thanks for let me know it. I'll not use header->bboxes[i], but will use:
bbox = header->bboxes;
bbox++;

and i'll also add comment in code to warn: don't use header->bboxes[i].