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

[Andreas Rheinhardt]

Nicolas George:
> 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, 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.
> 
> 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.
> 
> 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];

That's a variable-length array. That's a C99 feature we do not require.

>     } *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))
> 
> You can do the same to lift the 128 limit on the name.
> 
> Regards,
> 
> 
> _______________________________________________
> 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".
>