[FFmpeg-devel] [PATCH V3 3/3] libavfilter: add filter dnn_detect for object detection
Guo, Yejun
yejun.guo at intel.com
Mon Mar 1 16:44:25 EET 2021
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> Andreas Rheinhardt
> Sent: 2021年3月1日 21: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:
> >
> >
> >> -----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].
>
> Well, the spec-compliant and most elegant way to deal with such situations is
> a flexible array member: Add "BoundingBox boxes[];" to BoundingBoxHeader
> and allocate the memory the way you do now. You could then access these via
> header->boxes[i]. But this is a C99 feature that apparently older versions of
> MSVC don't support. So I don't know whether we are allowed to use it or not.
> Just do whatever works.
> Apologies for wasting your time here.
>
Anyway, I at least learned a lot, thanks.
More information about the ffmpeg-devel
mailing list