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@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
>>
>
> _______________________________________________
> 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".
_______________________________________________
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".
