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

[Andreas Rheinhardt]

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

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.

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

Well, as you wish.

> 
> I'll push the first 3 patches in this patch set tomorrow if there's no objection.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>