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

- Andreas