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

>>> +
>>> +        label = av_strdup(buf);
>>> +        if (!label) {
>>> +            av_log(context, AV_LOG_ERROR, "failed to allocate memory
>> for label %s\n", buf);
>>> +            fclose(file);
>>> +            free_detect_labels(ctx);
>>> +            return AVERROR(ENOMEM);
>>> +        }
>>> +
>>> +        if (av_dynarray_add_nofree(&ctx->labels, &ctx->label_count, label)
>> < 0) {
>>> +            av_log(context, AV_LOG_ERROR, "failed to do
>> av_dynarray_add\n");
>>> +            fclose(file);
>>> +            free_detect_labels(ctx);
>>
>> 1. You are leaking label here.
>> 2. You are repeating yourself with the cleanup code.
>> 3. When you return an error in a filter's init function, the filter's uninit function
>> is called automatically. In this case this means that free_detect_labels is called
>> twice which is not only wasteful, but
>> harmful: You are freeing ctx->labels (and all labels contained in it) in the first
>> run, but you are not resetting the number of labels. If
>> ctx->label_count is > 0, there will be a segfault when
>> free_detect_labels is called the second time.
> 
> good catch, will free label, remove the duplicate calling, and also reset ctx->label_count.
> 
>> 4. Return the error code.
>> (5. I consider your use of av_log for every error to be excessive.)
> 
> not quite understand this one, thanks.
> 

It means that it is unnecessary to add an av_log for every failed
allocation; returning the error code is enough.

>>
>>> +            return AVERROR(ENOMEM);
>>> +        }
>>> +    }
>>> +
>>> +    fclose(file);
>>> +    return 0;
>>> +}
>>> +