[FFmpeg-devel] [PATCH V3 3/3] libavfilter: add filter dnn_detect for object detection

[Andreas Rheinhardt]

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