[FFmpeg-devel] [PATCH V7 4/6] lavu: add side data AV_FRAME_DATA_BOUNDING_BOXES
Lynne
dev at lynne.ee
Thu Apr 8 07:13:47 EEST 2021
Apr 8, 2021, 04:48 by yejun.guo at intel.com:
>
>
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of Lynne
>> Sent: 2021年4月8日 5:04
>> To: FFmpeg development discussions and patches <ffmpeg-devel at 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.guo at intel.com:
>>
>> > Signed-off-by: Guo, Yejun <yejun.guo at 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.
More information about the ffmpeg-devel
mailing list