Re: [FFmpeg-devel] [PATCH 1/7 v4] avutil/frame: add a flag to allow overwritting existing entries
On 3/28/2024 9:21 AM, Anton Khirnov wrote:
Quoting James Almer (2024-03-28 12:41:40)
On 3/28/2024 8:23 AM, Anton Khirnov wrote:
Quoting James Almer (2024-03-28 04:12:04)
Enable it only for side data types that don't allow more than one entry.
Signed-off-by: James Almer
---
libavutil/frame.c | 59 ---
libavutil/frame.h | 27 +-
libavutil/tests/side_data_array.c | 52 +++
tests/ref/fate/side_data_array| 22 ++--
4 files changed, 115 insertions(+), 45 deletions(-)
diff --git a/libavutil/frame.c b/libavutil/frame.c
index ef1613c344..d9bd19b2aa 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -799,12 +799,34 @@ AVFrameSideData *av_frame_side_data_new(AVFrameSideData
***sd, int *nb_sd,
enum AVFrameSideDataType type,
size_t size, unsigned int flags)
{
-AVBufferRef *buf = av_buffer_alloc(size);
+const AVSideDataDescriptor *desc = av_frame_side_data_desc(type);
+AVBufferRef *buf;
AVFrameSideData *ret = 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)) {
desc == NULL means type is invalid
This being a generic alloc function, it should IMO not be limited to
currently defined types. And I chose to treat them as non multi, as
that's the most common scenario.
Andreas argued in the thread adding descriptors that side data type
without a descriptor is invalid and should explode. I tend to agree.
I agree with that when you're handling side data that comes from
something like a filter or similar modules within our libraries, as
that'd be a bug. But this is the public core API for side data, and i
don't see a reason why we should fail if the caller does
av_frame_side_data_new(, _sd, 1000, 1, 0);. We never did before,
and doing it now feels wrong.
___
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".
Re: [FFmpeg-devel] [PATCH 1/7 v4] avutil/frame: add a flag to allow overwritting existing entries
Quoting James Almer (2024-03-28 12:41:40)
> On 3/28/2024 8:23 AM, Anton Khirnov wrote:
> > Quoting James Almer (2024-03-28 04:12:04)
> >> Enable it only for side data types that don't allow more than one entry.
> >>
> >> Signed-off-by: James Almer
> >> ---
> >> libavutil/frame.c | 59 ---
> >> libavutil/frame.h | 27 +-
> >> libavutil/tests/side_data_array.c | 52 +++
> >> tests/ref/fate/side_data_array| 22 ++--
> >> 4 files changed, 115 insertions(+), 45 deletions(-)
> >>
> >> diff --git a/libavutil/frame.c b/libavutil/frame.c
> >> index ef1613c344..d9bd19b2aa 100644
> >> --- a/libavutil/frame.c
> >> +++ b/libavutil/frame.c
> >> @@ -799,12 +799,34 @@ AVFrameSideData
> >> *av_frame_side_data_new(AVFrameSideData ***sd, int *nb_sd,
> >> enum AVFrameSideDataType type,
> >> size_t size, unsigned int flags)
> >> {
> >> -AVBufferRef *buf = av_buffer_alloc(size);
> >> +const AVSideDataDescriptor *desc = av_frame_side_data_desc(type);
> >> +AVBufferRef *buf;
> >> AVFrameSideData *ret = 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)) {
> >
> > desc == NULL means type is invalid
>
> This being a generic alloc function, it should IMO not be limited to
> currently defined types. And I chose to treat them as non multi, as
> that's the most common scenario.
Andreas argued in the thread adding descriptors that side data type
without a descriptor is invalid and should explode. I tend to agree.
--
Anton Khirnov
___
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".
Re: [FFmpeg-devel] [PATCH 1/7 v4] avutil/frame: add a flag to allow overwritting existing entries
On 3/28/2024 8:23 AM, Anton Khirnov wrote:
Quoting James Almer (2024-03-28 04:12:04)
Enable it only for side data types that don't allow more than one entry.
Signed-off-by: James Almer
---
libavutil/frame.c | 59 ---
libavutil/frame.h | 27 +-
libavutil/tests/side_data_array.c | 52 +++
tests/ref/fate/side_data_array| 22 ++--
4 files changed, 115 insertions(+), 45 deletions(-)
diff --git a/libavutil/frame.c b/libavutil/frame.c
index ef1613c344..d9bd19b2aa 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -799,12 +799,34 @@ AVFrameSideData *av_frame_side_data_new(AVFrameSideData
***sd, int *nb_sd,
enum AVFrameSideDataType type,
size_t size, unsigned int flags)
{
-AVBufferRef *buf = av_buffer_alloc(size);
+const AVSideDataDescriptor *desc = av_frame_side_data_desc(type);
+AVBufferRef *buf;
AVFrameSideData *ret = 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)) {
desc == NULL means type is invalid
This being a generic alloc function, it should IMO not be limited to
currently defined types. And I chose to treat them as non multi, as
that's the most common scenario.
+for (int i = 0; i < *nb_sd; i++) {
+AVFrameSideData *entry = ((*sd)[i]);
+if (entry->type != type)
+continue;
Why are you not calling av_frame_side_data_get()?
An oversight i guess :p
@@ -822,13 +845,41 @@ int av_frame_side_data_clone(AVFrameSideData ***sd, int
*nb_sd,
if (!sd || !src || !nb_sd || (*nb_sd && !*sd))
return AVERROR(EINVAL);
+desc = av_frame_side_data_desc(src->type);
+if (flags & AV_FRAME_SIDE_DATA_FLAG_UNIQUE)
+remove_side_data(sd, nb_sd, src->type);
+if (!desc || !(desc->props & AV_SIDE_DATA_PROP_MULTI)) {
+for (int i = 0; i < *nb_sd; i++) {
+AVFrameSideData *entry = ((*sd)[i]);
+AVDictionary *dict = NULL;
+
+if (entry->type != src->type)
+continue;
+if (!(flags & AV_FRAME_SIDE_DATA_FLAG_REPLACE))
+return AVERROR(EEXIST);
+
+ret = av_dict_copy(, src->metadata, 0);
+if (ret < 0)
+return ret;
+
+ret = av_buffer_replace(>buf, src->buf);
+if (ret < 0) {
+av_dict_free();
+return ret;
+}
+
+av_dict_free(>metadata);
+entry->metadata = dict;
+entry->data = src->data;
+entry->size = src->size;
+return 0;
+}
This whole block looks very similar to the one you're adding to
av_frame_side_data_new().
I can try to factorize it.
+}
+
buf = av_buffer_ref(src->buf);
if (!buf)
return AVERROR(ENOMEM);
-if (flags & AV_FRAME_SIDE_DATA_FLAG_UNIQUE)
-remove_side_data(sd, nb_sd, src->type);
-
sd_dst = add_side_data_from_buf(sd, nb_sd, src->type, buf);
if (!sd_dst) {
av_buffer_unref();
diff --git a/libavutil/frame.h b/libavutil/frame.h
index 3b6d746a16..2ea129888e 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -1040,7 +1040,14 @@ const AVSideDataDescriptor *av_frame_side_data_desc(enum
AVFrameSideDataType typ
*/
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.
This should mention that it only applies to MULTI side data types.
Ok.
___
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".
Re: [FFmpeg-devel] [PATCH 1/7 v4] avutil/frame: add a flag to allow overwritting existing entries
Quoting James Almer (2024-03-28 04:12:04)
> Enable it only for side data types that don't allow more than one entry.
>
> Signed-off-by: James Almer
> ---
> libavutil/frame.c | 59 ---
> libavutil/frame.h | 27 +-
> libavutil/tests/side_data_array.c | 52 +++
> tests/ref/fate/side_data_array| 22 ++--
> 4 files changed, 115 insertions(+), 45 deletions(-)
>
> diff --git a/libavutil/frame.c b/libavutil/frame.c
> index ef1613c344..d9bd19b2aa 100644
> --- a/libavutil/frame.c
> +++ b/libavutil/frame.c
> @@ -799,12 +799,34 @@ AVFrameSideData *av_frame_side_data_new(AVFrameSideData
> ***sd, int *nb_sd,
> enum AVFrameSideDataType type,
> size_t size, unsigned int flags)
> {
> -AVBufferRef *buf = av_buffer_alloc(size);
> +const AVSideDataDescriptor *desc = av_frame_side_data_desc(type);
> +AVBufferRef *buf;
> AVFrameSideData *ret = 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)) {
desc == NULL means type is invalid
> +for (int i = 0; i < *nb_sd; i++) {
> +AVFrameSideData *entry = ((*sd)[i]);
> +if (entry->type != type)
> +continue;
Why are you not calling av_frame_side_data_get()?
> @@ -822,13 +845,41 @@ int av_frame_side_data_clone(AVFrameSideData ***sd, int
> *nb_sd,
> if (!sd || !src || !nb_sd || (*nb_sd && !*sd))
> return AVERROR(EINVAL);
>
> +desc = av_frame_side_data_desc(src->type);
> +if (flags & AV_FRAME_SIDE_DATA_FLAG_UNIQUE)
> +remove_side_data(sd, nb_sd, src->type);
> +if (!desc || !(desc->props & AV_SIDE_DATA_PROP_MULTI)) {
> +for (int i = 0; i < *nb_sd; i++) {
> +AVFrameSideData *entry = ((*sd)[i]);
> +AVDictionary *dict = NULL;
> +
> +if (entry->type != src->type)
> +continue;
> +if (!(flags & AV_FRAME_SIDE_DATA_FLAG_REPLACE))
> +return AVERROR(EEXIST);
> +
> +ret = av_dict_copy(, src->metadata, 0);
> +if (ret < 0)
> +return ret;
> +
> +ret = av_buffer_replace(>buf, src->buf);
> +if (ret < 0) {
> +av_dict_free();
> +return ret;
> +}
> +
> +av_dict_free(>metadata);
> +entry->metadata = dict;
> +entry->data = src->data;
> +entry->size = src->size;
> +return 0;
> +}
This whole block looks very similar to the one you're adding to
av_frame_side_data_new().
> +}
> +
> buf = av_buffer_ref(src->buf);
> if (!buf)
> return AVERROR(ENOMEM);
>
> -if (flags & AV_FRAME_SIDE_DATA_FLAG_UNIQUE)
> -remove_side_data(sd, nb_sd, src->type);
> -
> sd_dst = add_side_data_from_buf(sd, nb_sd, src->type, buf);
> if (!sd_dst) {
> av_buffer_unref();
> diff --git a/libavutil/frame.h b/libavutil/frame.h
> index 3b6d746a16..2ea129888e 100644
> --- a/libavutil/frame.h
> +++ b/libavutil/frame.h
> @@ -1040,7 +1040,14 @@ const AVSideDataDescriptor
> *av_frame_side_data_desc(enum AVFrameSideDataType typ
> */
> 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.
This should mention that it only applies to MULTI side data types.
--
Anton Khirnov
___
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] [PATCH 1/7 v4] avutil/frame: add a flag to allow overwritting existing entries
Enable it only for side data types that don't allow more than one entry.
Signed-off-by: James Almer
---
libavutil/frame.c | 59 ---
libavutil/frame.h | 27 +-
libavutil/tests/side_data_array.c | 52 +++
tests/ref/fate/side_data_array| 22 ++--
4 files changed, 115 insertions(+), 45 deletions(-)
diff --git a/libavutil/frame.c b/libavutil/frame.c
index ef1613c344..d9bd19b2aa 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -799,12 +799,34 @@ AVFrameSideData *av_frame_side_data_new(AVFrameSideData
***sd, int *nb_sd,
enum AVFrameSideDataType type,
size_t size, unsigned int flags)
{
-AVBufferRef *buf = av_buffer_alloc(size);
+const AVSideDataDescriptor *desc = av_frame_side_data_desc(type);
+AVBufferRef *buf;
AVFrameSideData *ret = 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 = av_buffer_alloc(size);
+if (!buf)
+return NULL;
+
+av_buffer_unref(>buf);
+av_dict_free(>metadata);
+entry->buf = buf;
+entry->data = buf->data;
+entry->size = buf->size;
+return entry;
+}
+}
+buf = av_buffer_alloc(size);
ret = add_side_data_from_buf(sd, nb_sd, type, buf);
if (!ret)
av_buffer_unref();
@@ -815,6 +837,7 @@ AVFrameSideData *av_frame_side_data_new(AVFrameSideData
***sd, int *nb_sd,
int av_frame_side_data_clone(AVFrameSideData ***sd, int *nb_sd,
const AVFrameSideData *src, unsigned int flags)
{
+const AVSideDataDescriptor *desc;
AVBufferRef *buf= NULL;
AVFrameSideData *sd_dst = NULL;
int ret= AVERROR_BUG;
@@ -822,13 +845,41 @@ int av_frame_side_data_clone(AVFrameSideData ***sd, int
*nb_sd,
if (!sd || !src || !nb_sd || (*nb_sd && !*sd))
return AVERROR(EINVAL);
+desc = av_frame_side_data_desc(src->type);
+if (flags & AV_FRAME_SIDE_DATA_FLAG_UNIQUE)
+remove_side_data(sd, nb_sd, src->type);
+if (!desc || !(desc->props & AV_SIDE_DATA_PROP_MULTI)) {
+for (int i = 0; i < *nb_sd; i++) {
+AVFrameSideData *entry = ((*sd)[i]);
+AVDictionary *dict = NULL;
+
+if (entry->type != src->type)
+continue;
+if (!(flags & AV_FRAME_SIDE_DATA_FLAG_REPLACE))
+return AVERROR(EEXIST);
+
+ret = av_dict_copy(, src->metadata, 0);
+if (ret < 0)
+return ret;
+
+ret = av_buffer_replace(>buf, src->buf);
+if (ret < 0) {
+av_dict_free();
+return ret;
+}
+
+av_dict_free(>metadata);
+entry->metadata = dict;
+entry->data = src->data;
+entry->size = src->size;
+return 0;
+}
+}
+
buf = av_buffer_ref(src->buf);
if (!buf)
return AVERROR(ENOMEM);
-if (flags & AV_FRAME_SIDE_DATA_FLAG_UNIQUE)
-remove_side_data(sd, nb_sd, src->type);
-
sd_dst = add_side_data_from_buf(sd, nb_sd, src->type, buf);
if (!sd_dst) {
av_buffer_unref();
diff --git a/libavutil/frame.h b/libavutil/frame.h
index 3b6d746a16..2ea129888e 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -1040,7 +1040,14 @@ const AVSideDataDescriptor *av_frame_side_data_desc(enum
AVFrameSideDataType typ
*/
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_REPLACE (1 << 1)
/**
* Add new side data entry to an array.
@@ -1053,10 +1060,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
