[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月8日 0:07
> 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:
> >
> >
> >> -----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
> >>
> >>> --- /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].

Looks that there will be possible padding between boxes[0] and boxes[1]
by using 'boxes[1] or just boxes'. Based on our design, it still works by
considering these padding bytes at the end of the array. I'll continue it.

> >
> >>
> >>> +    } *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.
> >
> 
> My remark was to inform you that this check should be removed if this
> patch is committed after the bump (when the check is obsolete (and
> harmful)). You should not remove it right now.

got it, thanks.