diff mbox

[FFmpeg-devel] frame: add an av_frame_new_side_data_from_buf function

Message ID 20180301170929.3003-1-atomnuker@gmail.com
State Superseded
Headers show

Commit Message

Rostislav Pehlivanov March 1, 2018, 5:09 p.m. UTC
Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com>
---

Yeah, that sounds better, thanks.

 libavutil/frame.c | 45 +++++++++++++++++++++++++--------------------
 libavutil/frame.h | 16 ++++++++++++++++
 2 files changed, 41 insertions(+), 20 deletions(-)

Comments

James Almer March 1, 2018, 6:18 p.m. UTC | #1
On 3/1/2018 2:09 PM, Rostislav Pehlivanov wrote:
> Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com>
> ---
> 
> Yeah, that sounds better, thanks.
> 
>  libavutil/frame.c | 45 +++++++++++++++++++++++++--------------------
>  libavutil/frame.h | 16 ++++++++++++++++
>  2 files changed, 41 insertions(+), 20 deletions(-)
> 
> diff --git a/libavutil/frame.c b/libavutil/frame.c
> index 662a7e5ab5..a49c853493 100644
> --- a/libavutil/frame.c
> +++ b/libavutil/frame.c
> @@ -26,11 +26,6 @@
>  #include "mem.h"
>  #include "samplefmt.h"
>  
> -
> -static AVFrameSideData *frame_new_side_data(AVFrame *frame,
> -                                            enum AVFrameSideDataType type,
> -                                            AVBufferRef *buf);
> -
>  #if FF_API_FRAME_GET_SET
>  MAKE_ACCESSORS(AVFrame, frame, int64_t, best_effort_timestamp)
>  MAKE_ACCESSORS(AVFrame, frame, int64_t, pkt_duration)
> @@ -356,8 +351,10 @@ FF_ENABLE_DEPRECATION_WARNINGS
>              }
>              memcpy(sd_dst->data, sd_src->data, sd_src->size);
>          } else {
> -            sd_dst = frame_new_side_data(dst, sd_src->type, av_buffer_ref(sd_src->buf));
> +            AVBufferRef *ref = av_buffer_ref(sd_src->buf);
> +            sd_dst = av_frame_new_side_data_from_buf(dst, sd_src->type, ref);
>              if (!sd_dst) {
> +                av_buffer_unref(&ref);
>                  wipe_side_data(dst);
>                  return AVERROR(ENOMEM);
>              }
> @@ -642,9 +639,9 @@ AVBufferRef *av_frame_get_plane_buffer(AVFrame *frame, int plane)
>      return NULL;
>  }
>  
> -static AVFrameSideData *frame_new_side_data(AVFrame *frame,
> -                                            enum AVFrameSideDataType type,
> -                                            AVBufferRef *buf)
> +AVFrameSideData *av_frame_new_side_data_from_buf(AVFrame *frame,
> +                                                 enum AVFrameSideDataType type,
> +                                                 AVBufferRef *buf)
>  {
>      AVFrameSideData *ret, **tmp;
>  
> @@ -652,18 +649,20 @@ static AVFrameSideData *frame_new_side_data(AVFrame *frame,
>          return NULL;
>  
>      if (frame->nb_side_data > INT_MAX / sizeof(*frame->side_data) - 1)
> -        goto fail;
> +        return NULL;
> +
> +    ret = av_mallocz(sizeof(*ret));
> +    if (!ret)
> +        return NULL;
>  
>      tmp = av_realloc(frame->side_data,
>                       (frame->nb_side_data + 1) * sizeof(*frame->side_data));
> -    if (!tmp)
> -        goto fail;
> +    if (!tmp) {
> +        av_free(tmp);

tmp?

And I still think this change is unrelated and needs to be in a separate
patch.

> +        return NULL;
> +    }
>      frame->side_data = tmp;
>  
> -    ret = av_mallocz(sizeof(*ret));
> -    if (!ret)
> -        goto fail;
> -
>      ret->buf = buf;
>      ret->data = ret->buf->data;
>      ret->size = buf->size;
> @@ -672,17 +671,23 @@ static AVFrameSideData *frame_new_side_data(AVFrame *frame,
>      frame->side_data[frame->nb_side_data++] = ret;
>  
>      return ret;
> -fail:
> -    av_buffer_unref(&buf);
> -    return NULL;
>  }
>  
>  AVFrameSideData *av_frame_new_side_data(AVFrame *frame,
>                                          enum AVFrameSideDataType type,
>                                          int size)
>  {
> +    AVFrameSideData *ret;
> +    AVBufferRef *buf = av_buffer_alloc(size);
> +    if (!buf)
> +        return NULL;
>  
> -    return frame_new_side_data(frame, type, av_buffer_alloc(size));
> +    ret = av_frame_new_side_data_from_buf(frame, type, buf);
> +    if (!ret) {
> +        av_buffer_unref(&buf);
> +        return NULL;

Again, just return ret, regardless of successful allocation or not.

> +    }
> +    return ret;
>  }
>  
>  AVFrameSideData *av_frame_get_side_data(const AVFrame *frame,
> diff --git a/libavutil/frame.h b/libavutil/frame.h
> index d54bd9a354..59cee8ceab 100644
> --- a/libavutil/frame.h
> +++ b/libavutil/frame.h
> @@ -800,6 +800,22 @@ AVFrameSideData *av_frame_new_side_data(AVFrame *frame,
>                                          enum AVFrameSideDataType type,
>                                          int size);
>  
> +/**
> + * Add a new side data to a frame from an existing AVBufferRef
> + *
> + * @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
> + *              the reference is transferred to the frame.
> + *
> + * @return newly added side data on success, NULL on error. On failure
> + *         the frame is unchanged and the AVBufferRef remains owned by
> + *         the caller.
> + */
> +AVFrameSideData *av_frame_new_side_data_from_buf(AVFrame *frame,
> +                                                 enum AVFrameSideDataType type,
> +                                                 AVBufferRef *buf);
> +
>  /**
>   * @return a pointer to the side data of a given type on success, NULL if there
>   * is no side data with such type in this frame.
> 

LGTM otherwise.
diff mbox

Patch

diff --git a/libavutil/frame.c b/libavutil/frame.c
index 662a7e5ab5..a49c853493 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -26,11 +26,6 @@ 
 #include "mem.h"
 #include "samplefmt.h"
 
-
-static AVFrameSideData *frame_new_side_data(AVFrame *frame,
-                                            enum AVFrameSideDataType type,
-                                            AVBufferRef *buf);
-
 #if FF_API_FRAME_GET_SET
 MAKE_ACCESSORS(AVFrame, frame, int64_t, best_effort_timestamp)
 MAKE_ACCESSORS(AVFrame, frame, int64_t, pkt_duration)
@@ -356,8 +351,10 @@  FF_ENABLE_DEPRECATION_WARNINGS
             }
             memcpy(sd_dst->data, sd_src->data, sd_src->size);
         } else {
-            sd_dst = frame_new_side_data(dst, sd_src->type, av_buffer_ref(sd_src->buf));
+            AVBufferRef *ref = av_buffer_ref(sd_src->buf);
+            sd_dst = av_frame_new_side_data_from_buf(dst, sd_src->type, ref);
             if (!sd_dst) {
+                av_buffer_unref(&ref);
                 wipe_side_data(dst);
                 return AVERROR(ENOMEM);
             }
@@ -642,9 +639,9 @@  AVBufferRef *av_frame_get_plane_buffer(AVFrame *frame, int plane)
     return NULL;
 }
 
-static AVFrameSideData *frame_new_side_data(AVFrame *frame,
-                                            enum AVFrameSideDataType type,
-                                            AVBufferRef *buf)
+AVFrameSideData *av_frame_new_side_data_from_buf(AVFrame *frame,
+                                                 enum AVFrameSideDataType type,
+                                                 AVBufferRef *buf)
 {
     AVFrameSideData *ret, **tmp;
 
@@ -652,18 +649,20 @@  static AVFrameSideData *frame_new_side_data(AVFrame *frame,
         return NULL;
 
     if (frame->nb_side_data > INT_MAX / sizeof(*frame->side_data) - 1)
-        goto fail;
+        return NULL;
+
+    ret = av_mallocz(sizeof(*ret));
+    if (!ret)
+        return NULL;
 
     tmp = av_realloc(frame->side_data,
                      (frame->nb_side_data + 1) * sizeof(*frame->side_data));
-    if (!tmp)
-        goto fail;
+    if (!tmp) {
+        av_free(tmp);
+        return NULL;
+    }
     frame->side_data = tmp;
 
-    ret = av_mallocz(sizeof(*ret));
-    if (!ret)
-        goto fail;
-
     ret->buf = buf;
     ret->data = ret->buf->data;
     ret->size = buf->size;
@@ -672,17 +671,23 @@  static AVFrameSideData *frame_new_side_data(AVFrame *frame,
     frame->side_data[frame->nb_side_data++] = ret;
 
     return ret;
-fail:
-    av_buffer_unref(&buf);
-    return NULL;
 }
 
 AVFrameSideData *av_frame_new_side_data(AVFrame *frame,
                                         enum AVFrameSideDataType type,
                                         int size)
 {
+    AVFrameSideData *ret;
+    AVBufferRef *buf = av_buffer_alloc(size);
+    if (!buf)
+        return NULL;
 
-    return frame_new_side_data(frame, type, av_buffer_alloc(size));
+    ret = av_frame_new_side_data_from_buf(frame, type, buf);
+    if (!ret) {
+        av_buffer_unref(&buf);
+        return NULL;
+    }
+    return ret;
 }
 
 AVFrameSideData *av_frame_get_side_data(const AVFrame *frame,
diff --git a/libavutil/frame.h b/libavutil/frame.h
index d54bd9a354..59cee8ceab 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -800,6 +800,22 @@  AVFrameSideData *av_frame_new_side_data(AVFrame *frame,
                                         enum AVFrameSideDataType type,
                                         int size);
 
+/**
+ * Add a new side data to a frame from an existing AVBufferRef
+ *
+ * @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
+ *              the reference is transferred to the frame.
+ *
+ * @return newly added side data on success, NULL on error. On failure
+ *         the frame is unchanged and the AVBufferRef remains owned by
+ *         the caller.
+ */
+AVFrameSideData *av_frame_new_side_data_from_buf(AVFrame *frame,
+                                                 enum AVFrameSideDataType type,
+                                                 AVBufferRef *buf);
+
 /**
  * @return a pointer to the side data of a given type on success, NULL if there
  * is no side data with such type in this frame.