> -----Original Message-----
> From: Guo, Yejun
> Sent: 2021年4月5日 17:56
> To: FFmpeg development discussions and patches <ffmpeg-devel@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-boun...@ffmpeg.org> On Behalf Of
> Nicolas
> > George
> > Sent: 2021年4月4日 18:02
> > To: FFmpeg development discussions and patches
> <ffmpeg-devel@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);
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
