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