| Message ID | 20240328031210.21407-2-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/7,v4] avutil/frame: add a flag to allow overwritting existing entries | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
Quoting James Almer (2024-03-28 04:12:05) > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavutil/frame.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++ > libavutil/frame.h | 34 ++++++++++++++++++++++++++++++ > 2 files changed, 87 insertions(+) > > diff --git a/libavutil/frame.c b/libavutil/frame.c > index d9bd19b2aa..a165e56a64 100644 > --- a/libavutil/frame.c > +++ b/libavutil/frame.c > @@ -834,6 +834,59 @@ AVFrameSideData *av_frame_side_data_new(AVFrameSideData ***sd, int *nb_sd, > return ret; > } > > +AVFrameSideData *av_frame_side_data_add(AVFrameSideData ***sd, int *nb_sd, > + enum AVFrameSideDataType type, > + AVBufferRef **pbuf, unsigned int flags) > +{ > + const AVSideDataDescriptor *desc = av_frame_side_data_desc(type); > + AVFrameSideData *sd_dst = NULL; > + AVBufferRef *buf; > + > + if (!sd || !pbuf || !*pbuf || !nb_sd || (*nb_sd && !*sd)) Overzealous checks like these tend to hide bugs. Any of these conditions being false means the caller is insane and we should crash. > + return NULL; > + > + if (flags & AV_FRAME_SIDE_DATA_FLAG_UNIQUE) > + remove_side_data(sd, nb_sd, type); > + if (!desc || !(desc->props & AV_SIDE_DATA_PROP_MULTI)) { > + for (int i = 0; i < *nb_sd; i++) { > + AVFrameSideData *entry = ((*sd)[i]); > + > + if (entry->type != type) > + continue; > + if (!(flags & AV_FRAME_SIDE_DATA_FLAG_REPLACE)) > + return NULL; > + > + buf = *pbuf; > + if (flags & AV_FRAME_SIDE_DATA_FLAG_NEW_REF) { > + int ret = av_buffer_replace(&entry->buf, buf); > + if (ret < 0) > + return NULL; > + } else > + *pbuf = NULL; > + > + av_dict_free(&entry->metadata); > + entry->data = buf->data; > + entry->size = buf->size; > + return entry; This again looks like a minor variation of the block you've added twice already. > + } > + } > + > + buf = (flags & AV_FRAME_SIDE_DATA_FLAG_NEW_REF) ? > + av_buffer_ref(*pbuf) : *pbuf; > + > + sd_dst = add_side_data_from_buf(sd, nb_sd, type, buf); > + if (!sd_dst) { > + if (flags & AV_FRAME_SIDE_DATA_FLAG_NEW_REF) > + av_buffer_unref(&buf); > + return NULL; > + } > + > + if (!(flags & AV_FRAME_SIDE_DATA_FLAG_NEW_REF)) > + *pbuf = NULL; > + > + return sd_dst; > +} > + > int av_frame_side_data_clone(AVFrameSideData ***sd, int *nb_sd, > const AVFrameSideData *src, unsigned int flags) > { > diff --git a/libavutil/frame.h b/libavutil/frame.h > index 2ea129888e..3e5d170a5b 100644 > --- a/libavutil/frame.h > +++ b/libavutil/frame.h > @@ -1048,6 +1048,10 @@ void av_frame_side_data_free(AVFrameSideData ***sd, int *nb_sd); > * Don't add a new entry if another of the same type exists. > */ > #define AV_FRAME_SIDE_DATA_FLAG_REPLACE (1 << 1) > +/** > + * Create a new reference instead of taking ownership of the passed in one. > + */ > +#define AV_FRAME_SIDE_DATA_FLAG_NEW_REF (1 << 2) Who needs this?
On 3/28/2024 8:27 AM, Anton Khirnov wrote: > Quoting James Almer (2024-03-28 04:12:05) >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavutil/frame.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++ >> libavutil/frame.h | 34 ++++++++++++++++++++++++++++++ >> 2 files changed, 87 insertions(+) >> >> diff --git a/libavutil/frame.c b/libavutil/frame.c >> index d9bd19b2aa..a165e56a64 100644 >> --- a/libavutil/frame.c >> +++ b/libavutil/frame.c >> @@ -834,6 +834,59 @@ AVFrameSideData *av_frame_side_data_new(AVFrameSideData ***sd, int *nb_sd, >> return ret; >> } >> >> +AVFrameSideData *av_frame_side_data_add(AVFrameSideData ***sd, int *nb_sd, >> + enum AVFrameSideDataType type, >> + AVBufferRef **pbuf, unsigned int flags) >> +{ >> + const AVSideDataDescriptor *desc = av_frame_side_data_desc(type); >> + AVFrameSideData *sd_dst = NULL; >> + AVBufferRef *buf; >> + >> + if (!sd || !pbuf || !*pbuf || !nb_sd || (*nb_sd && !*sd)) > > Overzealous checks like these tend to hide bugs. Any of these conditions > being false means the caller is insane and we should crash. I'll remove some, but others simplify the code below (like knowing beforehand that *pbuf is not NULL). > >> + return NULL; >> + >> + if (flags & AV_FRAME_SIDE_DATA_FLAG_UNIQUE) >> + remove_side_data(sd, nb_sd, type); >> + if (!desc || !(desc->props & AV_SIDE_DATA_PROP_MULTI)) { >> + for (int i = 0; i < *nb_sd; i++) { >> + AVFrameSideData *entry = ((*sd)[i]); >> + >> + if (entry->type != type) >> + continue; >> + if (!(flags & AV_FRAME_SIDE_DATA_FLAG_REPLACE)) >> + return NULL; >> + >> + buf = *pbuf; >> + if (flags & AV_FRAME_SIDE_DATA_FLAG_NEW_REF) { >> + int ret = av_buffer_replace(&entry->buf, buf); >> + if (ret < 0) >> + return NULL; >> + } else >> + *pbuf = NULL; >> + >> + av_dict_free(&entry->metadata); >> + entry->data = buf->data; >> + entry->size = buf->size; >> + return entry; > > This again looks like a minor variation of the block you've added twice > already. > >> + } >> + } >> + >> + buf = (flags & AV_FRAME_SIDE_DATA_FLAG_NEW_REF) ? >> + av_buffer_ref(*pbuf) : *pbuf; >> + >> + sd_dst = add_side_data_from_buf(sd, nb_sd, type, buf); >> + if (!sd_dst) { >> + if (flags & AV_FRAME_SIDE_DATA_FLAG_NEW_REF) >> + av_buffer_unref(&buf); >> + return NULL; >> + } >> + >> + if (!(flags & AV_FRAME_SIDE_DATA_FLAG_NEW_REF)) >> + *pbuf = NULL; >> + >> + return sd_dst; >> +} >> + >> int av_frame_side_data_clone(AVFrameSideData ***sd, int *nb_sd, >> const AVFrameSideData *src, unsigned int flags) >> { >> diff --git a/libavutil/frame.h b/libavutil/frame.h >> index 2ea129888e..3e5d170a5b 100644 >> --- a/libavutil/frame.h >> +++ b/libavutil/frame.h >> @@ -1048,6 +1048,10 @@ void av_frame_side_data_free(AVFrameSideData ***sd, int *nb_sd); >> * Don't add a new entry if another of the same type exists. >> */ >> #define AV_FRAME_SIDE_DATA_FLAG_REPLACE (1 << 1) >> +/** >> + * Create a new reference instead of taking ownership of the passed in one. >> + */ >> +#define AV_FRAME_SIDE_DATA_FLAG_NEW_REF (1 << 2) > > Who needs this? Someone who wants to keep the reference around, like when attaching a buffer to several outputs (global to individual output frames).
Quoting James Almer (2024-03-28 12:49:08) > On 3/28/2024 8:27 AM, Anton Khirnov wrote: > > Quoting James Almer (2024-03-28 04:12:05) > >> Signed-off-by: James Almer <jamrial@gmail.com> > >> --- > >> libavutil/frame.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++ > >> libavutil/frame.h | 34 ++++++++++++++++++++++++++++++ > >> 2 files changed, 87 insertions(+) > >> > >> diff --git a/libavutil/frame.c b/libavutil/frame.c > >> index d9bd19b2aa..a165e56a64 100644 > >> --- a/libavutil/frame.c > >> +++ b/libavutil/frame.c > >> @@ -834,6 +834,59 @@ AVFrameSideData *av_frame_side_data_new(AVFrameSideData ***sd, int *nb_sd, > >> return ret; > >> } > >> > >> +AVFrameSideData *av_frame_side_data_add(AVFrameSideData ***sd, int *nb_sd, > >> + enum AVFrameSideDataType type, > >> + AVBufferRef **pbuf, unsigned int flags) > >> +{ > >> + const AVSideDataDescriptor *desc = av_frame_side_data_desc(type); > >> + AVFrameSideData *sd_dst = NULL; > >> + AVBufferRef *buf; > >> + > >> + if (!sd || !pbuf || !*pbuf || !nb_sd || (*nb_sd && !*sd)) > > > > Overzealous checks like these tend to hide bugs. Any of these conditions > > being false means the caller is insane and we should crash. > > I'll remove some, but others simplify the code below (like knowing > beforehand that *pbuf is not NULL). You can just assume them all to be true. Or use av_assert0(). > >> diff --git a/libavutil/frame.h b/libavutil/frame.h > >> index 2ea129888e..3e5d170a5b 100644 > >> --- a/libavutil/frame.h > >> +++ b/libavutil/frame.h > >> @@ -1048,6 +1048,10 @@ void av_frame_side_data_free(AVFrameSideData ***sd, int *nb_sd); > >> * Don't add a new entry if another of the same type exists. > >> */ > >> #define AV_FRAME_SIDE_DATA_FLAG_REPLACE (1 << 1) > >> +/** > >> + * Create a new reference instead of taking ownership of the passed in one. > >> + */ > >> +#define AV_FRAME_SIDE_DATA_FLAG_NEW_REF (1 << 2) > > > > Who needs this? > > Someone who wants to keep the reference around, like when attaching a > buffer to several outputs (global to individual output frames). Is that a common enough use case to warrant this flag? It complicates the code quite substantially. And if you're making some side data instance global, what is the point of also attaching it to frames?
On 3/28/2024 9:19 AM, Anton Khirnov wrote: > Quoting James Almer (2024-03-28 12:49:08) >> On 3/28/2024 8:27 AM, Anton Khirnov wrote: >>> Quoting James Almer (2024-03-28 04:12:05) >>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>> --- >>>> libavutil/frame.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++ >>>> libavutil/frame.h | 34 ++++++++++++++++++++++++++++++ >>>> 2 files changed, 87 insertions(+) >>>> >>>> diff --git a/libavutil/frame.c b/libavutil/frame.c >>>> index d9bd19b2aa..a165e56a64 100644 >>>> --- a/libavutil/frame.c >>>> +++ b/libavutil/frame.c >>>> @@ -834,6 +834,59 @@ AVFrameSideData *av_frame_side_data_new(AVFrameSideData ***sd, int *nb_sd, >>>> return ret; >>>> } >>>> >>>> +AVFrameSideData *av_frame_side_data_add(AVFrameSideData ***sd, int *nb_sd, >>>> + enum AVFrameSideDataType type, >>>> + AVBufferRef **pbuf, unsigned int flags) >>>> +{ >>>> + const AVSideDataDescriptor *desc = av_frame_side_data_desc(type); >>>> + AVFrameSideData *sd_dst = NULL; >>>> + AVBufferRef *buf; >>>> + >>>> + if (!sd || !pbuf || !*pbuf || !nb_sd || (*nb_sd && !*sd)) >>> >>> Overzealous checks like these tend to hide bugs. Any of these conditions >>> being false means the caller is insane and we should crash. >> >> I'll remove some, but others simplify the code below (like knowing >> beforehand that *pbuf is not NULL). > > You can just assume them all to be true. Or use av_assert0(). > >>>> diff --git a/libavutil/frame.h b/libavutil/frame.h >>>> index 2ea129888e..3e5d170a5b 100644 >>>> --- a/libavutil/frame.h >>>> +++ b/libavutil/frame.h >>>> @@ -1048,6 +1048,10 @@ void av_frame_side_data_free(AVFrameSideData ***sd, int *nb_sd); >>>> * Don't add a new entry if another of the same type exists. >>>> */ >>>> #define AV_FRAME_SIDE_DATA_FLAG_REPLACE (1 << 1) >>>> +/** >>>> + * Create a new reference instead of taking ownership of the passed in one. >>>> + */ >>>> +#define AV_FRAME_SIDE_DATA_FLAG_NEW_REF (1 << 2) >>> >>> Who needs this? >> >> Someone who wants to keep the reference around, like when attaching a >> buffer to several outputs (global to individual output frames). > > Is that a common enough use case to warrant this flag? It complicates > the code quite substantially. > > And if you're making some side data instance global, what is the point > of also attaching it to frames? To pass it to API that does not handle global entries, like currently happens with filtergraphs. I can remove the flag for now in any case. It can be added later trivially.
diff --git a/libavutil/frame.c b/libavutil/frame.c index d9bd19b2aa..a165e56a64 100644 --- a/libavutil/frame.c +++ b/libavutil/frame.c @@ -834,6 +834,59 @@ AVFrameSideData *av_frame_side_data_new(AVFrameSideData ***sd, int *nb_sd, return ret; } +AVFrameSideData *av_frame_side_data_add(AVFrameSideData ***sd, int *nb_sd, + enum AVFrameSideDataType type, + AVBufferRef **pbuf, unsigned int flags) +{ + const AVSideDataDescriptor *desc = av_frame_side_data_desc(type); + AVFrameSideData *sd_dst = NULL; + AVBufferRef *buf; + + if (!sd || !pbuf || !*pbuf || !nb_sd || (*nb_sd && !*sd)) + return NULL; + + if (flags & AV_FRAME_SIDE_DATA_FLAG_UNIQUE) + remove_side_data(sd, nb_sd, type); + if (!desc || !(desc->props & AV_SIDE_DATA_PROP_MULTI)) { + for (int i = 0; i < *nb_sd; i++) { + AVFrameSideData *entry = ((*sd)[i]); + + if (entry->type != type) + continue; + if (!(flags & AV_FRAME_SIDE_DATA_FLAG_REPLACE)) + return NULL; + + buf = *pbuf; + if (flags & AV_FRAME_SIDE_DATA_FLAG_NEW_REF) { + int ret = av_buffer_replace(&entry->buf, buf); + if (ret < 0) + return NULL; + } else + *pbuf = NULL; + + av_dict_free(&entry->metadata); + entry->data = buf->data; + entry->size = buf->size; + return entry; + } + } + + buf = (flags & AV_FRAME_SIDE_DATA_FLAG_NEW_REF) ? + av_buffer_ref(*pbuf) : *pbuf; + + sd_dst = add_side_data_from_buf(sd, nb_sd, type, buf); + if (!sd_dst) { + if (flags & AV_FRAME_SIDE_DATA_FLAG_NEW_REF) + av_buffer_unref(&buf); + return NULL; + } + + if (!(flags & AV_FRAME_SIDE_DATA_FLAG_NEW_REF)) + *pbuf = NULL; + + return sd_dst; +} + int av_frame_side_data_clone(AVFrameSideData ***sd, int *nb_sd, const AVFrameSideData *src, unsigned int flags) { diff --git a/libavutil/frame.h b/libavutil/frame.h index 2ea129888e..3e5d170a5b 100644 --- a/libavutil/frame.h +++ b/libavutil/frame.h @@ -1048,6 +1048,10 @@ void av_frame_side_data_free(AVFrameSideData ***sd, int *nb_sd); * Don't add a new entry if another of the same type exists. */ #define AV_FRAME_SIDE_DATA_FLAG_REPLACE (1 << 1) +/** + * Create a new reference instead of taking ownership of the passed in one. + */ +#define AV_FRAME_SIDE_DATA_FLAG_NEW_REF (1 << 2) /** * Add new side data entry to an array. @@ -1066,11 +1070,40 @@ void av_frame_side_data_free(AVFrameSideData ***sd, int *nb_sd); * is attempted. * @note In case of AV_FRAME_SIDE_DATA_FLAG_REPLACE being set, if an * entry of the same type already exists, it will be replaced instead. + * @note AV_FRAME_SIDE_DATA_FLAG_NEW_REF has no effect in this function. */ AVFrameSideData *av_frame_side_data_new(AVFrameSideData ***sd, int *nb_sd, enum AVFrameSideDataType type, size_t size, unsigned int flags); +/** + * Add a new side data entry to an array from an existing AVBufferRef. + * + * @param sd pointer to array of side data to which to add another entry, + * or to NULL in order to start a new array. + * @param nb_sd pointer to an integer containing the number of entries in + * the array. + * @param type type of the added side data + * @param buf Pointer to AVBufferRef to add to the array. On success, + * the function takes ownership of the AVBufferRef and *buf is + * set to NULL, unless AV_FRAME_SIDE_DATA_FLAG_NEW_REF is set + * in which case the ownership will remain with the caller. + * @param flags Some combination of AV_FRAME_SIDE_DATA_FLAG_* flags, or 0. + * + * @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_REPLACE being set, if an + * entry of the same type already exists, it will be replaced instead. + * @note In case of AV_FRAME_SIDE_DATA_FLAG_NEW_REF being set, the ownership + * of *buf will remain with the caller. + * + */ +AVFrameSideData *av_frame_side_data_add(AVFrameSideData ***sd, int *nb_sd, + enum AVFrameSideDataType type, + AVBufferRef **buf, unsigned int flags); + /** * Add a new side data entry to an array based on existing side data, taking * a reference towards the contained AVBufferRef. @@ -1089,6 +1122,7 @@ AVFrameSideData *av_frame_side_data_new(AVFrameSideData ***sd, int *nb_sd, * is attempted. * @note In case of AV_FRAME_SIDE_DATA_FLAG_REPLACE being set, if an * entry of the same type already exists, it will be replaced instead. + * @note AV_FRAME_SIDE_DATA_FLAG_NEW_REF has no effect in this function. */ 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 | 53 +++++++++++++++++++++++++++++++++++++++++++++++ libavutil/frame.h | 34 ++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+)