> -----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. > > + > > + 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. > > > + 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".