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

[James Almer]

On 4/4/2021 7:01 AM, Nicolas George wrote:
> 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];
>      } *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))

This solution is what was used for video_enc_params.h, so i agree it 
should be used here too. What's missing is a check for idx < nb_bbox 
before accessing the offset in question, so an inline function instead 
of a #define with a check for the above would be needed.

And since both are used as frame side data, it would be ideal that the 
signature for the public helpers on both are the same (The standard 
alloc, and the alloc + wrapping into frame side data ones).

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