| Message ID | 20240325200602.63020-1-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/6,v2] avutil/frame: add a flag to not create duplicate entries in a side data array | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
Quoting James Almer (2024-03-25 21:05:57) > +/** > + * Remove existing entries before adding new ones. > + */ > #define AV_FRAME_SIDE_DATA_FLAG_UNIQUE (1 << 0) > +/** > + * Don't add a new entry if another of the same type exists. > + */ > +#define AV_FRAME_SIDE_DATA_FLAG_DONT_APPEND (1 << 1) I don't really like this API, because it leaves too much work to the user. With my descriptor set, we know when it makes sense to have duplicate side data. So the cases that can occur when adding side data of type T are: * T not present, call succeeds * T present, then * T does not have a MULTI prop, then user decides whether to replace or do nothing (or perhaps fail) * T does have a MULTI prop, then user decides whether to replace, add, or do nothing I think the default behaviour for MULTI types should be adding, and the other two cases should be covered by the user explicitly. Then we only need a single flag, which only applies to non-MULTI types, and chooses whether to replace or not.
On 3/27/2024 5:05 AM, Anton Khirnov wrote: > Quoting James Almer (2024-03-25 21:05:57) >> +/** >> + * Remove existing entries before adding new ones. >> + */ >> #define AV_FRAME_SIDE_DATA_FLAG_UNIQUE (1 << 0) >> +/** >> + * Don't add a new entry if another of the same type exists. >> + */ >> +#define AV_FRAME_SIDE_DATA_FLAG_DONT_APPEND (1 << 1) > > I don't really like this API, because it leaves too much work to the > user. > > With my descriptor set, we know when it makes sense to have duplicate > side data. So the cases that can occur when adding side data of type T > are: > * T not present, call succeeds > * T present, then > * T does not have a MULTI prop, then user decides whether to > replace or do nothing (or perhaps fail) av_frame_side_data_new() returns an entry, so for non-MULTI types, it should return the existing one. There's no "do nothing" scenario, and i don't particularly like failing, more so now that 7.0 is branched so we can't stealthly change behavior and for example define EEXIST error code as "did nothing". > * T does have a MULTI prop, then user decides whether to replace, > add, or do nothing > > I think the default behaviour for MULTI types should be adding, and the > other two cases should be covered by the user explicitly. > > Then we only need a single flag, which only applies to non-MULTI types, > and chooses whether to replace or not. I don't follow. How does a single flag let you choose between all these scenarios?
On 3/27/2024 8:49 AM, James Almer wrote: > On 3/27/2024 5:05 AM, Anton Khirnov wrote: >> Quoting James Almer (2024-03-25 21:05:57) >>> +/** >>> + * Remove existing entries before adding new ones. >>> + */ >>> #define AV_FRAME_SIDE_DATA_FLAG_UNIQUE (1 << 0) >>> +/** >>> + * Don't add a new entry if another of the same type exists. >>> + */ >>> +#define AV_FRAME_SIDE_DATA_FLAG_DONT_APPEND (1 << 1) >> >> I don't really like this API, because it leaves too much work to the >> user. >> >> With my descriptor set, we know when it makes sense to have duplicate >> side data. So the cases that can occur when adding side data of type T >> are: >> * T not present, call succeeds >> * T present, then >> * T does not have a MULTI prop, then user decides whether to >> replace or do nothing (or perhaps fail) > > av_frame_side_data_new() returns an entry, so for non-MULTI types, it > should return the existing one. There's no "do nothing" scenario, and i > don't particularly like failing, more so now that 7.0 is branched so we > can't stealthly change behavior and for example define EEXIST error code > as "did nothing". Err, error code would be for av_frame_side_data_clone(). av_frame_side_data_new() can either return an entry or NULL, which the user will consider a failure. > >> * T does have a MULTI prop, then user decides whether to replace, >> add, or do nothing >> >> I think the default behaviour for MULTI types should be adding, and the >> other two cases should be covered by the user explicitly. >> >> Then we only need a single flag, which only applies to non-MULTI types, >> and chooses whether to replace or not. > > I don't follow. How does a single flag let you choose between all these > scenarios?
Quoting James Almer (2024-03-27 12:49:49) > > * T does have a MULTI prop, then user decides whether to replace, > > add, or do nothing > > > > I think the default behaviour for MULTI types should be adding, and the > > other two cases should be covered by the user explicitly. > > > > Then we only need a single flag, which only applies to non-MULTI types, > > and chooses whether to replace or not. > > I don't follow. How does a single flag let you choose between all these > scenarios? I doesn't, my point is that for MULTI types the functions should always add a new instance, because very few scenarios need anything else.
diff --git a/libavutil/frame.c b/libavutil/frame.c index d7a32cdc92..a780e62fd0 100644 --- a/libavutil/frame.c +++ b/libavutil/frame.c @@ -774,6 +774,13 @@ AVFrameSideData *av_frame_side_data_new(AVFrameSideData ***sd, int *nb_sd, if (flags & AV_FRAME_SIDE_DATA_FLAG_UNIQUE) remove_side_data(sd, nb_sd, type); + if (flags & AV_FRAME_SIDE_DATA_FLAG_DONT_APPEND) { + for (int i = 0; i < *nb_sd; i++) { + AVFrameSideData *entry = ((*sd)[i]); + if (entry->type == type) + return entry; + } + } ret = add_side_data_from_buf(sd, nb_sd, type, buf); if (!ret) @@ -798,6 +805,13 @@ int av_frame_side_data_clone(AVFrameSideData ***sd, int *nb_sd, if (flags & AV_FRAME_SIDE_DATA_FLAG_UNIQUE) remove_side_data(sd, nb_sd, src->type); + if (flags & AV_FRAME_SIDE_DATA_FLAG_DONT_APPEND) { + for (int i = 0; i < *nb_sd; i++) { + AVFrameSideData *entry = ((*sd)[i]); + if (entry->type == src->type) + return 0; + } + } sd_dst = add_side_data_from_buf(sd, nb_sd, src->type, buf); if (!sd_dst) { diff --git a/libavutil/frame.h b/libavutil/frame.h index 8aa05ec127..7cc55a455e 100644 --- a/libavutil/frame.h +++ b/libavutil/frame.h @@ -1003,7 +1003,14 @@ const char *av_frame_side_data_name(enum AVFrameSideDataType type); */ void av_frame_side_data_free(AVFrameSideData ***sd, int *nb_sd); +/** + * Remove existing entries before adding new ones. + */ #define AV_FRAME_SIDE_DATA_FLAG_UNIQUE (1 << 0) +/** + * Don't add a new entry if another of the same type exists. + */ +#define AV_FRAME_SIDE_DATA_FLAG_DONT_APPEND (1 << 1) /** * Add new side data entry to an array. @@ -1016,10 +1023,12 @@ void av_frame_side_data_free(AVFrameSideData ***sd, int *nb_sd); * @param size size of the side data * @param flags Some combination of AV_FRAME_SIDE_DATA_FLAG_* flags, or 0. * - * @return newly added side data on success, NULL on error. In case of - * AV_FRAME_SIDE_DATA_FLAG_UNIQUE being set, entries of matching - * AVFrameSideDataType will be removed before the addition is - * attempted. + * @return newly added side data on success, NULL on error. + * @note In case of AV_FRAME_SIDE_DATA_FLAG_UNIQUE being set, entries of + * matching AVFrameSideDataType will be removed before the addition + * is attempted. + * @note In case of AV_FRAME_SIDE_DATA_FLAG_DONT_APPEND being set, if an + * entry of the same type already exists, it will be returned instead. */ AVFrameSideData *av_frame_side_data_new(AVFrameSideData ***sd, int *nb_sd, enum AVFrameSideDataType type, @@ -1037,10 +1046,13 @@ AVFrameSideData *av_frame_side_data_new(AVFrameSideData ***sd, int *nb_sd, * for the buffer. * @param flags Some combination of AV_FRAME_SIDE_DATA_FLAG_* flags, or 0. * - * @return negative error code on failure, >=0 on success. In case of - * AV_FRAME_SIDE_DATA_FLAG_UNIQUE being set, entries of matching - * AVFrameSideDataType will be removed before the addition is - * attempted. + * @return negative error code on failure, >=0 on success. + * @note In case of AV_FRAME_SIDE_DATA_FLAG_UNIQUE being set, entries of + * matching AVFrameSideDataType will be removed before the addition + * is attempted. + * @note In case of AV_FRAME_SIDE_DATA_FLAG_DONT_APPEND being set, if an + * entry of the same type as the one from src already exists, this + * function will be a no-op. */ int av_frame_side_data_clone(AVFrameSideData ***sd, int *nb_sd, const AVFrameSideData *src, unsigned int flags);
Signed-off-by: James Almer <jamrial@gmail.com> --- libavutil/frame.c | 14 ++++++++++++++ libavutil/frame.h | 28 ++++++++++++++++++++-------- 2 files changed, 34 insertions(+), 8 deletions(-)