[FFmpeg-devel] [PATCH V7 4/6] lavu: add side data AV_FRAME_DATA_BOUNDING_BOXES

[Guo, Yejun]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> Andreas Rheinhardt
> Sent: 2021年4月7日 22:44
> To: ffmpeg-devel at ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH V7 4/6] lavu: add side data
> AV_FRAME_DATA_BOUNDING_BOXES
> 
> Guo, Yejun:
> > 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[];
> 
> Hopefully all compilers accept a flexible array member; if not, using
> boxes[1] (or just boxes) would be sufficient.

patchwork is passed, but I'm not 100% sure all the compilers accept it,
I'll change to boxes[1].

> 
> > +    } *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;
> 
> Using offsetof would be clearer (for this you have to declare a proper
> type).

maybe both are ok.

> 
> > +
> > +    if (out_size)
> > +        *out_size = sizeof(*ret);
> > +
> > +    return &ret->header;
> > +}
> > +
> > +AVBoundingBoxHeader* av_bbox_create_side_data(AVFrame *frame,
> uint32_t nb_bboxes)
> > +{
> > +    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;
> > +    }
> 
> Will be obsolete soon.

will remove the check, thanks.

> 
> > +    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;
> > +}
> 
> Overall, the code duplication with video_enc_params.c should be factored
> out.

Yes, there's some code duplication. But some code are relative to different
struct (AVBoundingBoxHeader and AVVideoEncParams), and we could not
unify them without type template. The left common code is about av_buffer_create
and av_frame_new_side_data_from_buf, do we have necessarity to put them
together? My current answer is no, I'll continue to think about it tomorrow.


I'll push the first 3 patches in this patch set tomorrow if there's no objection.