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

[Guo, Yejun]

> -----Original Message-----
> From: Guo, Yejun
> Sent: 2021年4月5日 17:56
> To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
> Subject: RE: [FFmpeg-devel] [PATCH V6 4/6] lavu: add side data
> AV_FRAME_DATA_BOUNDING_BOXES
> 
> 
> 
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> Nicolas
> > George
> > Sent: 2021年4月4日 18:02
> > To: FFmpeg development discussions and patches
> <ffmpeg-devel at ffmpeg.org>
> > Subject: Re: [FFmpeg-devel] [PATCH V6 4/6] lavu: add side data
> > AV_FRAME_DATA_BOUNDING_BOXES
> >
> > Guo, Yejun (12021-04-01):
> > > > Is this allowed to be negative? Can/should this be size_t?
> > > There was a long discussion about size_t/int/uint32_t when I added
> > > struct AVRegionOfInterest in frame.h, and finally size_t is not chosen.
> >
> > Then at least unsigned.
> 
> yes, 'unsigned int' is another option. but imho, it might be better to use
> the same data type of AVFrame.width/height, they are 'int'.
> 
> >
> > > yes, we can add a version number (enum AVBBoxHeaderVersion, see below)
> > at the
> > > beginning of the struct, and the user needs to set it with
> > AV_BBOX_HEADER_CURRENT.
> > > It is not elegant, is this what we really want?
> >
> > A version number does not make the structure compatible. At best,
> > applications check the version and detect they do not support this one
> > and report an error. At worse, they do not bother to check and it causes
> > strange bugs. The version number does not help with compatibility, it
> > just makes it detectable.
> 
> My idea was to use separate code path for AVBoundingBoxHeaderV1,
> AVBoundingBoxHeaderV2, ... (not union in my previous email) according to
> the version number. And yes, it doesn't work if the user does not set or
> check the version number correctly.
> 
> >
> > To make the structure compatible, we have to ask ourselves: What happens
> > if we update it? What breaks compatibility? Once we have an answer, we
> > find a workaround.
> >
> > In this case, what happens is that the variable array must be at the
> > end, and therefore its offset changes. And we cannot have a second
> > variable array (like the name! you had to set a constant size), and we
> > cannot update the type of the array elements.
> >
> > And the solution becomes obvious: let us store the offsets in the
> > structure.
> 
> Thanks a lot for your nice explanation! thank you.
> 
> >
> > So, that would be:
> >
> > 	typedef struct AVBoundingBoxHeader {
> > 	...
> > 	    /**
> > 	     * Offset of the array of bounding boxes.
> > 	     */
> > 	    size_t bboxes_offset;
> >
> > 	    /**
> > 	     * Offset increment in the array of bounding boxes.
> > 	     */
> > 	    size_t bboxes_step;
> > 	};
> >
> > Note that with that solution, we do not need the empty array, we can do
> > this:
> >
> > AVBoundingBoxHeader *ff_bounding_box_alloc(size_t nb_bbox)
> > {
> >     struct {
> > 	AVBoundingBoxHeader header;
> > 	AVBoundingBox boxes[nb_bbox];
> >     } *ret;
> >     ret = av_mallocz(sizeof(*ret));
> >     /* add error checks */
> >     ret->header->bboxes_offset = (char *)&ret->boxes - (char *)ret->header;
> >     ret->header->bboxes_step = sizeof(*ret->boxes);
> > }
> >
> > #define AV_BOUNDING_BOX_GET(header, idx) \
> >     ((AVBoundingBox *)((char *)(header) + (header)->bboxes_offset + (idx) *
> > (header)->bboxes_step))
> 
> thanks, and I'll update the naming as what James mentioned.
> 
> >
> > You can do the same to lift the 128 limit on the name.
> 
> yes, we can now handle more variable arrays with this method.

Looks that we could not support more variable arrays without requiring special
compiler feature. See the sample code below, and I got compile error on my
ubuntu 18.04 with default compiler and default configure option.

sample code:
AVBoundingBoxHeader *
av_bbox_alloc(uint32_t nb_bboxes, size_t name_size, size_t *out_size)
{
    struct {
	    AVBoundingBoxHeader header;
	    AVBoundingBox boxes[nb_bboxes];
        char name[name_size];
    } *ret;
    ...
}

compile error on ubuntu 18.04 with default setting:
error: ISO C90 forbids variable length array ‘boxes’ [-Werror=vla]
      AVBoundingBox boxes[nb_bboxes];
...

So, I might still use 'char source[128]' in AVBoundingBoxHeader, and use the
following code to meet the alignment requirement.

    struct {
	    AVBoundingBoxHeader header;
	    AVBoundingBox boxes[];
    } *ret;
    ret = av_mallocz(sizeof(*ret) + sizeof(AVBoundingBox) * nb_bboxes);