[FFmpeg-devel,RFC] avutil/frame: fix remove_side_data

Submitted by Zhao Zhili on Nov. 2, 2019, 4:16 p.m.

Details

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

Commit Message

Zhao Zhili Nov. 2, 2019, 4:16 p.m.
From: Zhao Zhili <zhilizhao@tencent.com>

remove_side_data is supposed to remove a single instance by design.
Since new_side_data() doesn't forbid add multiple instance of the
same type, remove_side_data should deal with that.
---
 libavutil/frame.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

Comments

Marton Balint Nov. 2, 2019, 4:30 p.m.
On Sun, 3 Nov 2019, Zhao Zhili wrote:

> From: Zhao Zhili <zhilizhao@tencent.com>
>
> remove_side_data is supposed to remove a single instance by design.
> Since new_side_data() doesn't forbid add multiple instance of the
> same type, remove_side_data should deal with that.

Please update the docs of the function that it removes all side data 
instances of the specified type.

> ---
> libavutil/frame.c | 21 +++++++++++++--------
> 1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/libavutil/frame.c b/libavutil/frame.c
> index dcf1fc3d17..10d06dd29f 100644
> --- a/libavutil/frame.c
> +++ b/libavutil/frame.c
> @@ -805,15 +805,20 @@ int av_frame_copy(AVFrame *dst, const AVFrame *src)
> void av_frame_remove_side_data(AVFrame *frame, enum AVFrameSideDataType type)
> {
>     int i;
> -
> -    for (i = 0; i < frame->nb_side_data; i++) {
> -        AVFrameSideData *sd = frame->side_data[i];
> -        if (sd->type == type) {
> -            free_side_data(&frame->side_data[i]);
> -            frame->side_data[i] = frame->side_data[frame->nb_side_data - 1];
> -            frame->nb_side_data--;
> +    int found;
> +
> +    do {
> +        found = 0;
> +        for (i = 0; i < frame->nb_side_data; i++) {
> +            AVFrameSideData *sd = frame->side_data[i];
> +            if (sd->type == type) {
> +                free_side_data(&frame->side_data[i]);
> +                frame->side_data[i] = frame->side_data[frame->nb_side_data - 1];
> +                frame->nb_side_data--;
> +                found = 1;
> +            }
>         }
> -    }
> +    } while (found);

I am sure this can be done in a single loop. (E.g.: decrement i when side 
data is found)

Regards,
Marton

Patch hide | download patch | download mbox

diff --git a/libavutil/frame.c b/libavutil/frame.c
index dcf1fc3d17..10d06dd29f 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -805,15 +805,20 @@  int av_frame_copy(AVFrame *dst, const AVFrame *src)
 void av_frame_remove_side_data(AVFrame *frame, enum AVFrameSideDataType type)
 {
     int i;
-
-    for (i = 0; i < frame->nb_side_data; i++) {
-        AVFrameSideData *sd = frame->side_data[i];
-        if (sd->type == type) {
-            free_side_data(&frame->side_data[i]);
-            frame->side_data[i] = frame->side_data[frame->nb_side_data - 1];
-            frame->nb_side_data--;
+    int found;
+
+    do {
+        found = 0;
+        for (i = 0; i < frame->nb_side_data; i++) {
+            AVFrameSideData *sd = frame->side_data[i];
+            if (sd->type == type) {
+                free_side_data(&frame->side_data[i]);
+                frame->side_data[i] = frame->side_data[frame->nb_side_data - 1];
+                frame->nb_side_data--;
+                found = 1;
+            }
         }
-    }
+    } while (found);
 }
 
 const char *av_frame_side_data_name(enum AVFrameSideDataType type)