[FFmpeg-devel,v2,1/4] frame: handle add side data with the same type

Submitted by Zhao Zhili on Nov. 1, 2019, 12:03 p.m.

Details

Message ID 20191101120314.88956-1-quinkblack@foxmail.com
State New
Headers show

Commit Message

Zhao Zhili Nov. 1, 2019, 12:03 p.m.
From: Zhao Zhili <zhilizhao@tencent.com>

---
 libavutil/frame.c | 13 +++++++++++++
 libavutil/frame.h |  4 ++++
 2 files changed, 17 insertions(+)

Comments

Hendrik Leppkes Nov. 1, 2019, 12:13 p.m.
On Fri, Nov 1, 2019 at 1:03 PM <quinkblack@foxmail.com> wrote:
>
> From: Zhao Zhili <zhilizhao@tencent.com>
>
> ---
>  libavutil/frame.c | 13 +++++++++++++
>  libavutil/frame.h |  4 ++++
>  2 files changed, 17 insertions(+)
>

I believe there have been some use-cases, especially around
closed-captions, where multiple blocks of the same type have been
used, somehow. Since this is really an API change, not sure its such a
good idea.

- Hendrik
Zhao Zhili Nov. 1, 2019, 12:48 p.m.
> On Nov 1, 2019, at 8:13 PM, Hendrik Leppkes <h.leppkes@gmail.com> wrote:
> 
> On Fri, Nov 1, 2019 at 1:03 PM <quinkblack@foxmail.com> wrote:
>> 
>> From: Zhao Zhili <zhilizhao@tencent.com>
>> 
>> ---
>> libavutil/frame.c | 13 +++++++++++++
>> libavutil/frame.h |  4 ++++
>> 2 files changed, 17 insertions(+)
>> 
> 
> I believe there have been some use-cases, especially around
> closed-captions, where multiple blocks of the same type have been
> used, somehow. Since this is really an API change, not sure its such a
> good idea.

I guess it may be too late to change the behavior. Whether keep the current
behavior or not, remove_side_data is broken. It can remove multiple side data
and miss a few.

It can be fixed in two different ways:

1. Remove the first side data of the supplied type, like get_side_data().
2. Remove all side data of the supplied type.

The first solution match current documentation.

> 
> - Hendrik
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Marton Balint Nov. 1, 2019, 5:16 p.m.
On Fri, 1 Nov 2019, "zhilizhao(赵志立)" wrote:

>
>
>> On Nov 1, 2019, at 8:13 PM, Hendrik Leppkes <h.leppkes@gmail.com> wrote:
>> 
>> On Fri, Nov 1, 2019 at 1:03 PM <quinkblack@foxmail.com> wrote:
>>> 
>>> From: Zhao Zhili <zhilizhao@tencent.com>
>>> 
>>> ---
>>> libavutil/frame.c | 13 +++++++++++++
>>> libavutil/frame.h |  4 ++++
>>> 2 files changed, 17 insertions(+)
>>> 
>> 
>> I believe there have been some use-cases, especially around
>> closed-captions, where multiple blocks of the same type have been
>> used, somehow. Since this is really an API change, not sure its such a
>> good idea.
>
> I guess it may be too late to change the behavior.

I am not sure, all our API around side data (get/remove) is based on the 
assumption that a single entry of a type exists. Also for packet/stream 
side data it is already assumed as far as I see. So at least for the sake 
of consistency it should be the same way. Maybe we should print a 
deprecation warning if something adds multiple side data of the same type. 
And later sometime it can be changed to actually replace the old side 
data.

> Whether keep the current
> behavior or not, remove_side_data is broken. It can remove multiple side data
> and miss a few.

Yes.

>
> It can be fixed in two different ways:
>
> 1. Remove the first side data of the supplied type, like get_side_data().
> 2. Remove all side data of the supplied type.
>
> The first solution match current documentation.

I don't see why the 2nd would contradict docs:

/**
  * If side data of the supplied type exists in the frame, free it and remove it
  * from the frame.
  */

So I'd vote for 2), which removes all side data of the specified type.

Regards,
Marton
Zhao Zhili Nov. 1, 2019, 5:34 p.m.
> On Nov 2, 2019, at 1:16 AM, Marton Balint <cus@passwd.hu> wrote:
> 
> 
> 
> On Fri, 1 Nov 2019, "zhilizhao(赵志立)" wrote:
> 
>> 
>> 
>>> On Nov 1, 2019, at 8:13 PM, Hendrik Leppkes <h.leppkes@gmail.com> wrote:
>>> On Fri, Nov 1, 2019 at 1:03 PM <quinkblack@foxmail.com> wrote:
>>>> From: Zhao Zhili <zhilizhao@tencent.com>
>>>> ---
>>>> libavutil/frame.c | 13 +++++++++++++
>>>> libavutil/frame.h |  4 ++++
>>>> 2 files changed, 17 insertions(+)
>>> I believe there have been some use-cases, especially around
>>> closed-captions, where multiple blocks of the same type have been
>>> used, somehow. Since this is really an API change, not sure its such a
>>> good idea.
>> 
>> I guess it may be too late to change the behavior.
> 
> I am not sure, all our API around side data (get/remove) is based on the assumption that a single entry of a type exists. Also for packet/stream side data it is already assumed as far as I see. So at least for the sake of consistency it should be the same way. Maybe we should print a deprecation warning if something adds multiple side data of the same type. And later sometime it can be changed to actually replace the old side data.
> 
>> Whether keep the current
>> behavior or not, remove_side_data is broken. It can remove multiple side data
>> and miss a few.
> 
> Yes.
> 
>> 
>> It can be fixed in two different ways:
>> 
>> 1. Remove the first side data of the supplied type, like get_side_data().
>> 2. Remove all side data of the supplied type.
>> 
>> The first solution match current documentation.
> 
> I don't see why the 2nd would contradict docs:
> 
> /**
> * If side data of the supplied type exists in the frame, free it and remove it
> * from the frame.
> */
> 
> So I'd vote for 2), which removes all side data of the specified type.

APIchanges says (if it counts as part of the documentation):

2014-02-24 - 3e1f241 / d161ae0 - lavu 52.69.100 / 53.8.0 - frame.h
  Add av_frame_remove_side_data() for removing a single side data
  instance from a frame.

> 
> Regards,
> Marton
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org <mailto:ffmpeg-devel@ffmpeg.org>
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel <https://ffmpeg.org/mailman/listinfo/ffmpeg-devel>
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org <mailto:ffmpeg-devel-request@ffmpeg.org> with subject "unsubscribe".
Marton Balint Nov. 1, 2019, 5:40 p.m.
On Sat, 2 Nov 2019, Zhao Zhili wrote:

>
>
>> On Nov 2, 2019, at 1:16 AM, Marton Balint <cus@passwd.hu> wrote:
>> 
>> 
>> 
>> On Fri, 1 Nov 2019, "zhilizhao(赵志立)" wrote:
>> 
>>> 
>>> 
>>>> On Nov 1, 2019, at 8:13 PM, Hendrik Leppkes <h.leppkes@gmail.com> wrote:
>>>> On Fri, Nov 1, 2019 at 1:03 PM <quinkblack@foxmail.com> wrote:
>>>>> From: Zhao Zhili <zhilizhao@tencent.com>
>>>>> ---
>>>>> libavutil/frame.c | 13 +++++++++++++
>>>>> libavutil/frame.h |  4 ++++
>>>>> 2 files changed, 17 insertions(+)
>>>> I believe there have been some use-cases, especially around
>>>> closed-captions, where multiple blocks of the same type have been
>>>> used, somehow. Since this is really an API change, not sure its such a
>>>> good idea.
>>> 
>>> I guess it may be too late to change the behavior.
>> 
>> I am not sure, all our API around side data (get/remove) is based on the assumption that a single entry of a type exists. Also for packet/stream side data it is already assumed as far as I see. So at least for the sake of consistency it should be the same way. Maybe we should print a deprecation warning if something adds multiple side data of the same type. And later sometime it can be changed to actually replace the old side data.
>> 
>>> Whether keep the current
>>> behavior or not, remove_side_data is broken. It can remove multiple side data
>>> and miss a few.
>> 
>> Yes.
>> 
>>> 
>>> It can be fixed in two different ways:
>>> 
>>> 1. Remove the first side data of the supplied type, like get_side_data().
>>> 2. Remove all side data of the supplied type.
>>> 
>>> The first solution match current documentation.
>> 
>> I don't see why the 2nd would contradict docs:
>> 
>> /**
>> * If side data of the supplied type exists in the frame, free it and remove it
>> * from the frame.
>> */
>> 
>> So I'd vote for 2), which removes all side data of the specified type.
>
> APIchanges says (if it counts as part of the documentation):
>
> 2014-02-24 - 3e1f241 / d161ae0 - lavu 52.69.100 / 53.8.0 - frame.h
>  Add av_frame_remove_side_data() for removing a single side data
>  instance from a frame.

I guess that can be iterpreted that way, still I'd go for 2).

Regards,
Marton
Michael Niedermayer Nov. 3, 2019, 9:04 p.m.
On Fri, Nov 01, 2019 at 06:16:38PM +0100, Marton Balint wrote:
> 
> 
> On Fri, 1 Nov 2019, "zhilizhao(赵志立)" wrote:
> 
> >
> >
> >>On Nov 1, 2019, at 8:13 PM, Hendrik Leppkes <h.leppkes@gmail.com> wrote:
> >>
> >>On Fri, Nov 1, 2019 at 1:03 PM <quinkblack@foxmail.com> wrote:
> >>>
> >>>From: Zhao Zhili <zhilizhao@tencent.com>
> >>>
> >>>---
> >>>libavutil/frame.c | 13 +++++++++++++
> >>>libavutil/frame.h |  4 ++++
> >>>2 files changed, 17 insertions(+)
> >>>
> >>
> >>I believe there have been some use-cases, especially around
> >>closed-captions, where multiple blocks of the same type have been
> >>used, somehow. Since this is really an API change, not sure its such a
> >>good idea.
> >
> >I guess it may be too late to change the behavior.
> 
> I am not sure, all our API around side data (get/remove) is based on the
> assumption that a single entry of a type exists. Also for packet/stream side
> data it is already assumed as far as I see. So at least for the sake of
> consistency it should be the same way. Maybe we should print a deprecation
> warning if something adds multiple side data of the same type. And later
> sometime it can be changed to actually replace the old side data.

In respect to adding side data when the same type already exists,
it may be more robust to error out in such a case instead of replacing.

Also we may handle deprecation in a type specific manner
In cases where a duplicated type makes semantically no sense and also doesnt
occur it could possibly be considered an error immedeatly
Only cases where it makes sense or does actually happen need a deprecation
period i think. That is if someone wants to implement this at such a
fine grained level ...

[...]
Zhao Zhili Nov. 4, 2019, 1:47 p.m.
> On Nov 4, 2019, at 5:04 AM, Michael Niedermayer <michael@niedermayer.cc> wrote:
> 
> On Fri, Nov 01, 2019 at 06:16:38PM +0100, Marton Balint wrote:
>> 
>> 
>> On Fri, 1 Nov 2019, "zhilizhao(赵志立)" wrote:
>> 
>>> 
>>> 
>>>> On Nov 1, 2019, at 8:13 PM, Hendrik Leppkes <h.leppkes@gmail.com> wrote:
>>>> 
>>>> On Fri, Nov 1, 2019 at 1:03 PM <quinkblack@foxmail.com> wrote:
>>>>> 
>>>>> From: Zhao Zhili <zhilizhao@tencent.com>
>>>>> 
>>>>> ---
>>>>> libavutil/frame.c | 13 +++++++++++++
>>>>> libavutil/frame.h |  4 ++++
>>>>> 2 files changed, 17 insertions(+)
>>>>> 
>>>> 
>>>> I believe there have been some use-cases, especially around
>>>> closed-captions, where multiple blocks of the same type have been
>>>> used, somehow. Since this is really an API change, not sure its such a
>>>> good idea.
>>> 
>>> I guess it may be too late to change the behavior.
>> 
>> I am not sure, all our API around side data (get/remove) is based on the
>> assumption that a single entry of a type exists. Also for packet/stream side
>> data it is already assumed as far as I see. So at least for the sake of
>> consistency it should be the same way. Maybe we should print a deprecation
>> warning if something adds multiple side data of the same type. And later
>> sometime it can be changed to actually replace the old side data.
> 
> In respect to adding side data when the same type already exists,
> it may be more robust to error out in such a case instead of replacing.

I like the idea of error out.

> 
> Also we may handle deprecation in a type specific manner
> In cases where a duplicated type makes semantically no sense and also doesnt
> occur it could possibly be considered an error immedeatly
> Only cases where it makes sense or does actually happen need a deprecation
> period i think. That is if someone wants to implement this at such a
> fine grained level …

It’s hard to tell which types actually been used with multiple entries.

> 
> [...]
> 
> -- 
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> 
> In fact, the RIAA has been known to suggest that students drop out
> of college or go to community college in order to be able to afford
> settlements. -- The RIAA
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org <mailto:ffmpeg-devel@ffmpeg.org>
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel <https://ffmpeg.org/mailman/listinfo/ffmpeg-devel>
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org <mailto:ffmpeg-devel-request@ffmpeg.org> with subject "unsubscribe".

Patch hide | download patch | download mbox

diff --git a/libavutil/frame.c b/libavutil/frame.c
index dcf1fc3d17..bb20e99331 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -692,10 +692,23 @@  AVFrameSideData *av_frame_new_side_data_from_buf(AVFrame *frame,
                                                  AVBufferRef *buf)
 {
     AVFrameSideData *ret, **tmp;
+    int i;
 
     if (!buf)
         return NULL;
 
+    for (i = 0; i < frame->nb_side_data; i++) {
+        AVFrameSideData *sd = frame->side_data[i];
+        if (sd->type == type) {
+            av_buffer_unref(&sd->buf);
+            av_dict_free(&sd->metadata);
+            sd->buf = buf;
+            sd->data = buf->data;
+            sd->size = buf->size;
+            return sd;
+        }
+    }
+
     if (frame->nb_side_data > INT_MAX / sizeof(*frame->side_data) - 1)
         return NULL;
 
diff --git a/libavutil/frame.h b/libavutil/frame.h
index 5d3231e7bb..906143f894 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -886,6 +886,8 @@  AVBufferRef *av_frame_get_plane_buffer(AVFrame *frame, int plane);
 /**
  * Add a new side data to a frame.
  *
+ * @note replace existing side data of the same type
+ *
  * @param frame a frame to which the side data should be added
  * @param type type of the added side data
  * @param size size of the side data
@@ -899,6 +901,8 @@  AVFrameSideData *av_frame_new_side_data(AVFrame *frame,
 /**
  * Add a new side data to a frame from an existing AVBufferRef
  *
+ * @note replace existing side data of the same type
+ *
  * @param frame a frame to which the side data should be added
  * @param type  the type of the added side data
  * @param buf   an AVBufferRef to add as side data. The ownership of