Apr 8, 2021, 04:48 by yejun....@intel.com: > > >> -----Original Message----- >> From: ffmpeg-devel <ffmpeg-devel-boun...@ffmpeg.org> On Behalf Of Lynne >> Sent: 2021年4月8日 5:04 >> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> >> Subject: Re: [FFmpeg-devel] [PATCH V7 4/6] lavu: add side data >> AV_FRAME_DATA_BOUNDING_BOXES >> >> Apr 7, 2021, 16:17 by yejun....@intel.com: >> >> > Signed-off-by: Guo, Yejun <yejun....@intel.com> >> > --- >> > doc/APIchanges | 2 + >> > libavutil/Makefile | 2 + >> > libavutil/boundingbox.c | 73 +++++++++++++++++++++++++ >> > libavutil/boundingbox.h | 114 >> ++++++++++++++++++++++++++++++++++++++++ >> > libavutil/frame.c | 1 + >> > libavutil/frame.h | 7 +++ >> > 6 files changed, 199 insertions(+) >> > create mode 100644 libavutil/boundingbox.c >> > create mode 100644 libavutil/boundingbox.h >> > >> > diff --git a/doc/APIchanges b/doc/APIchanges >> > index 9dfcc97d5c..81d01aac1e 100644 >> > --- a/doc/APIchanges >> > +++ b/doc/APIchanges >> > @@ -14,6 +14,8 @@ libavutil: 2017-10-21 >> > >> > >> > API changes, most recent first: >> > +2021-04-xx - xxxxxxxxxx - lavu 56.xx.100 - frame.h boundingbox.h >> > + Add AV_FRAME_DATA_BOUNDING_BOXES >> > >> > 2021-04-06 - xxxxxxxxxx - lavf 58.78.100 - avformat.h >> > Add avformat_index_get_entries_count(), avformat_index_get_entry(), >> > diff --git a/libavutil/Makefile b/libavutil/Makefile >> > index 27bafe9e12..f6d21bb5ce 100644 >> > --- a/libavutil/Makefile >> > +++ b/libavutil/Makefile >> > @@ -11,6 +11,7 @@ HEADERS = adler32.h >> \ >> > avutil.h \ >> > base64.h \ >> > blowfish.h \ >> > + boundingbox.h >> \ >> > bprint.h \ >> > bswap.h \ >> > buffer.h \ >> > @@ -104,6 +105,7 @@ OBJS = adler32.o >> \ >> > avsscanf.o >> \ >> > base64.o >> \ >> > blowfish.o >> \ >> > + boundingbox.o >> \ >> > bprint.o >> \ >> > buffer.o \ >> > cast5.o >> \ >> > diff --git a/libavutil/boundingbox.c b/libavutil/boundingbox.c >> > new file mode 100644 >> > index 0000000000..881661ec18 >> > --- /dev/null >> > +++ b/libavutil/boundingbox.c >> > @@ -0,0 +1,73 @@ >> > +/* >> > + * This file is part of FFmpeg. >> > + * >> > + * FFmpeg is free software; you can redistribute it and/or >> > + * modify it under the terms of the GNU Lesser General Public >> > + * License as published by the Free Software Foundation; either >> > + * version 2.1 of the License, or (at your option) any later version. >> > + * >> > + * FFmpeg is distributed in the hope that it will be useful, >> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> GNU >> > + * Lesser General Public License for more details. >> > + * >> > + * You should have received a copy of the GNU Lesser General Public >> > + * License along with FFmpeg; if not, write to the Free Software >> > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA >> > 02110-1301 >> USA >> > + */ >> > + >> > +#include "boundingbox.h" >> > + >> > +AVBoundingBoxHeader *av_bbox_alloc(uint32_t nb_bboxes, size_t >> *out_size) >> > +{ >> > + size_t size; >> > + struct { >> > + AVBoundingBoxHeader header; >> > + AVBoundingBox boxes[]; >> > + } *ret; >> > + >> > + size = sizeof(*ret); >> > + if (nb_bboxes > (SIZE_MAX - size) / sizeof(*ret->boxes)) >> > + return NULL; >> > + size += sizeof(*ret->boxes) * nb_bboxes; >> > + >> > + ret = av_mallocz(size); >> > + if (!ret) >> > + return NULL; >> > + >> > + ret->header.nb_bboxes = nb_bboxes; >> > + ret->header.bbox_size = sizeof(*ret->boxes); >> > + ret->header.bboxes_offset = (char *)&ret->boxes - (char >> > *)&ret->header; >> > + >> > + if (out_size) >> > + *out_size = sizeof(*ret); >> > + >> > + return &ret->header; >> > +} >> > + >> > +AVBoundingBoxHeader* av_bbox_create_side_data(AVFrame *frame, >> uint32_t nb_bboxes) >> > >> >> Fix this everywhere. We never put the * before the name. This is a misleading >> coding style that makes absolutely no sense. >> > > Do you mean change from: > AVBoundingBoxHeader* av_bbox_create_side_data > to: > AVBoundingBoxHeader *av_bbox_create_side_data > > Will fix it, thanks. > > btw, I think the coding style is to put '*' just before the name, for example, > AVFrame *frame; > const AVClass *class; > >> >> >> > +{ >> > + AVBufferRef *buf; >> > + AVBoundingBoxHeader *header; >> > + size_t size; >> > + >> > + header = av_bbox_alloc(nb_bboxes, &size); >> > + if (!header) >> > + return NULL; >> > + if (size > INT_MAX) { >> > + av_freep(&header); >> > + return NULL; >> > + } >> > + buf = av_buffer_create((uint8_t *)header, size, NULL, NULL, 0); >> > + if (!buf) { >> > + av_freep(&header); >> > + return NULL; >> > + } >> > + >> > + if (!av_frame_new_side_data_from_buf(frame, >> AV_FRAME_DATA_BOUNDING_BOXES, buf)) { >> > + av_buffer_unref(&buf); >> > + return NULL; >> > + } >> > + >> > + return header; >> > +} >> > diff --git a/libavutil/boundingbox.h b/libavutil/boundingbox.h >> > new file mode 100644 >> > index 0000000000..2b77c6c0dc >> > --- /dev/null >> > +++ b/libavutil/boundingbox.h >> > @@ -0,0 +1,114 @@ >> > +/* >> > + * This file is part of FFmpeg. >> > + * >> > + * FFmpeg is free software; you can redistribute it and/or >> > + * modify it under the terms of the GNU Lesser General Public >> > + * License as published by the Free Software Foundation; either >> > + * version 2.1 of the License, or (at your option) any later version. >> > + * >> > + * FFmpeg is distributed in the hope that it will be useful, >> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> GNU >> > + * Lesser General Public License for more details. >> > + * >> > + * You should have received a copy of the GNU Lesser General Public >> > + * License along with FFmpeg; if not, write to the Free Software >> > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA >> > 02110-1301 >> USA >> > + */ >> > + >> > +#ifndef AVUTIL_BOUNDINGBOX_H >> > +#define AVUTIL_BOUNDINGBOX_H >> > + >> > +#include "rational.h" >> > +#include "avassert.h" >> > +#include "frame.h" >> > + >> > +typedef struct AVBoundingBox { >> > + /** >> > + * Distance in pixels from the top edge of the frame to top >> > + * and bottom, and from the left edge of the frame to left and >> > + * right, defining the bounding box. >> > + */ >> > + int top; >> > + int left; >> > + int bottom; >> > + int right; >> > >> >> Why not x, y, w, h? >> It makes more sense, all of coordinates go like this. >> > > We want to keep it consistent with 'struct AVRegionOfInterest', > we also have a plan to add a filter which converts from bounding box > to RegionOfInterest which impacts the encoder. >
Not a good idea. Region of interest is only useful to indicate a single region, while this API describes multiple regions within the frame. The video_enc_params API follows the x, y, w, h because it's the easiest to work with, so I'm sure it's well worth going to such coordinates. >> >> > + >> > +#define AV_BBOX_LABEL_NAME_MAX_SIZE 32 >> > >> >> Better bump this to 64. It won't be enough, and will >> require a major bump to increment. >> > > I usually don't see a very long label name, and you are right, > we'd better bump it for safe. Will change, thanks. > >> >> >> > + >> > + /** >> > + * Detect result with confidence >> > + */ >> > + char detect_label[AV_BBOX_LABEL_NAME_MAX_SIZE]; >> > + AVRational detect_confidence; >> > + >> > + /** >> > + * At most 4 classifications based on the detected bounding box. >> > + * For example, we can get max 4 different attributes with 4 different >> > + * DNN models on one bounding box. >> > + * classify_count is zero if no classification. >> > + */ >> > +#define AV_NUM_BBOX_CLASSIFY 4 >> > + uint32_t classify_count; >> > + char >> classify_labels[AV_NUM_BBOX_CLASSIFY][AV_BBOX_LABEL_NAME_MAX_SIZE]; >> > + AVRational classify_confidences[AV_NUM_BBOX_CLASSIFY]; >> > +} AVBoundingBox; >> > + >> > +typedef struct AVBoundingBoxHeader { >> > + /** >> > + * Information about how the bounding box is generated. >> > + * for example, the DNN model name. >> > + */ >> > + char source[128]; >> > + >> > + /** >> > + * The size of frame when it is detected. >> > + */ >> > + int frame_width; >> > + int frame_height; >> > >> >> Why? This side data is attached to AVFrames only, where we >> already have width and height. >> > > The detection result will be used by other filters, for example, > dnn_classify (see https://github.com/guoyejun/ffmpeg/tree/classify). > > The filter dnn_detect detects all the objects (cat, dog, person ...) in a > frame, while dnn_classify classifies one detected object (for example, person) > for its attribute (for example, emotion, etc.) > > The filter dnn_classify have to check if the frame size is changed after > it is detected, to handle the below filter chain: > dnn_detect -> scale -> dnn_classify > This doesn't look good. Why is dnn_classify needing to know the original frame size at all? >> > + >> > + /** >> > + * Number of bounding boxes in the array. >> > + */ >> > + uint32_t nb_bboxes; >> > + >> > + /** >> > + * Offset in bytes from the beginning of this structure at which >> > + * the array of bounding boxes starts. >> > + */ >> > + size_t bboxes_offset; >> > + >> > + /** >> > + * Size of each bounding box in bytes. >> > + */ >> > + size_t bbox_size; >> > +} AVBoundingBoxHeader; >> > + >> > +/* >> > + * Get the bounding box at the specified {@code idx}. Must be between 0 >> and nb_bboxes. >> > + */ >> > +static av_always_inline AVBoundingBox* >> > +av_get_bounding_box(const AVBoundingBoxHeader *header, unsigned int >> idx) >> > +{ >> > + av_assert0(idx < header->nb_bboxes); >> > + return (AVBoundingBox *)((uint8_t *)header + header->bboxes_offset + >> > + idx * header->bbox_size); >> > +} >> > + >> > +/** >> > + * Allocates memory for AVBoundingBoxHeader, plus an array of {@code >> nb_bboxes} >> > + * AVBoundingBox, and initializes the variables. >> > + * Can be freed with a normal av_free() call. >> > + * >> > + * @param out_size if non-NULL, the size in bytes of the resulting data >> > array >> is >> > + * written here. >> > + */ >> > +AVBoundingBoxHeader *av_bbox_alloc(uint32_t nb_bboxes, size_t >> *out_size); >> > + >> > +/** >> > + * Allocates memory for AVBoundingBoxHeader, plus an array of {@code >> nb_bboxes} >> > + * AVBoundingBox, in the given AVFrame {@code frame} as >> AVFrameSideData of type >> > + * AV_FRAME_DATA_BOUNDING_BOXES and initializes the variables. >> > + */ >> > +AVBoundingBoxHeader* av_bbox_create_side_data(AVFrame *frame, >> uint32_t nb_bboxes); >> > +#endif >> > diff --git a/libavutil/frame.c b/libavutil/frame.c >> > index fefb2b69c0..d8e86263af 100644 >> > --- a/libavutil/frame.c >> > +++ b/libavutil/frame.c >> > @@ -853,6 +853,7 @@ const char *av_frame_side_data_name(enum >> AVFrameSideDataType type) >> > case AV_FRAME_DATA_VIDEO_ENC_PARAMS: return "Video >> encoding parameters"; >> > case AV_FRAME_DATA_SEI_UNREGISTERED: return "H.26[45] >> User Data Unregistered SEI message"; >> > case AV_FRAME_DATA_FILM_GRAIN_PARAMS: return "Film >> grain parameters"; >> > + case AV_FRAME_DATA_BOUNDING_BOXES: return >> "Bounding boxes"; >> > } >> > return NULL; >> > } >> > diff --git a/libavutil/frame.h b/libavutil/frame.h >> > index a5ed91b20a..41e22de02a 100644 >> > --- a/libavutil/frame.h >> > +++ b/libavutil/frame.h >> > @@ -198,6 +198,13 @@ enum AVFrameSideDataType { >> > * Must be present for every frame which should have film grain applied. >> > */ >> > AV_FRAME_DATA_FILM_GRAIN_PARAMS, >> > + >> > + /** >> > + * Bounding boxes for object detection and classification, the data >> > is a >> AVBoundingBoxHeader >> > + * followed with an array of AVBoudingBox, and >> AVBoundingBoxHeader.nb_bboxes is the number >> > + * of array element. >> > + */ >> > + AV_FRAME_DATA_BOUNDING_BOXES, >> > }; >> > >> >> Finally, why call it a Bounding Box? It's not descriptive at all. >> How about "Object Classification"? It makes much more sense, it's >> exactly what this is. So AVObjectClassification, AVObjectClassification, >> AV_FRAME_DATA_OBJECT_CLASSIFICATION and so on. >> > > In object detection papers, bounding box is usually used. > We'd better use the same term, imho, thanks. > Not in this case, API users won't have any idea what this is or what it's for. This is user-facing code after all. Papers in fields can get away with overloading language, but we're trying to make a concise API. Object classification makes sense because this is exactly what this is. _______________________________________________ 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".