Guo, Yejun: > > >> -----Original Message----- >> From: ffmpeg-devel <ffmpeg-devel-boun...@ffmpeg.org> On Behalf Of >> Andreas Rheinhardt >> Sent: 2021年3月1日 16:31 >> To: ffmpeg-devel@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-boun...@ffmpeg.org> On Behalf Of >>>> Andreas Rheinhardt >>>> Sent: 2021年3月1日 12:50 >>>> To: ffmpeg-devel@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....@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@ffmpeg.org >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> >> To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org >> with subject "unsubscribe". > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". > _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".