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

[Andreas Rheinhardt]

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

Actually nothing in video_enc_params.c guarantees proper alignment for
AVVideoBlockParams. If we added a type requiring 64bit alignment to
AVVideoBlockParams and are on a 32bit system, then the blocks will be
misaligned.

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