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