[FFmpeg-devel] [PATCH V3 3/3] libavfilter: add filter dnn_detect for object detection
Andreas Rheinhardt
andreas.rheinhardt at gmail.com
Mon Mar 1 14:13:13 EET 2021
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.
>>
>>>>> +
>>>>> + 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;
>>>>> +}
>>>>> +
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email ffmpeg-devel-request at ffmpeg.org
>> with subject "unsubscribe".
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>
More information about the ffmpeg-devel
mailing list