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