[FFmpeg-devel] frame: add an av_frame_new_side_data_from_buf function

Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com>
---

Sending again, forgot to amend.

Updated with description of that the function will do.
Originally the function made a reference from the bufferref but someone
said that it shouldn't. I don't think that this is very useful so I've
changed it back so that it refs the buffer (and leaves it up to API users
to unref it if they no longer need it).
The function will still do nothing in case it fails and returns NULL.
Also, the previous version still had the old function forward declared,
fixed that by removing it.

 libavutil/frame.c | 43 ++++++++++++++++++++++++-------------------
 libavutil/frame.h | 16 ++++++++++++++++
 2 files changed, 40 insertions(+), 19 deletions(-)

On 3/1/2018 10:55 AM, Rostislav Pehlivanov wrote:
> Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com>
> ---
> 
> Sending again, forgot to amend.
> 
> Updated with description of that the function will do.
> Originally the function made a reference from the bufferref but someone
> said that it shouldn't. I don't think that this is very useful so I've
> changed it back so that it refs the buffer (and leaves it up to API users
> to unref it if they no longer need it).
> The function will still do nothing in case it fails and returns NULL.
> Also, the previous version still had the old function forward declared,
> fixed that by removing it.

I prefer if the function takes ownership of the reference rather than
create a new one on success.
If it creates a new one, then the caller still needs to do something
with the reference passed to the function afterwards regardless of
outcome, as shown in your changes to av_frame_new_side_data().
This is also extra overhead and one more point of failure for the
function (creating the reference) for no real gain. Let the user decide
if they want to keep the reference they passed or if they have no use
for it and would rather have the side data take ownership of it.

I'll leave the choice to wm4 anyway since he's the one interested in
this addition the most.

> 
>  libavutil/frame.c | 43 ++++++++++++++++++++++++-------------------
>  libavutil/frame.h | 16 ++++++++++++++++
>  2 files changed, 40 insertions(+), 19 deletions(-)
> 
> diff --git a/libavutil/frame.c b/libavutil/frame.c
> index 662a7e5ab5..869b7866c8 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,7 +351,7 @@ 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));
> +            sd_dst = av_frame_new_side_data_from_buf(dst, sd_src->type, sd_src->buf);
>              if (!sd_dst) {
>                  wipe_side_data(dst);
>                  return AVERROR(ENOMEM);
> @@ -642,29 +637,30 @@ 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;
> +    AVFrameSideData *ret, **tmp = NULL;
> +    AVBufferRef *ref = av_buffer_ref(buf);
>  
> -    if (!buf)
> -        return NULL;
> +    if (!buf || !ref)
> +        goto fail;
>  
>      if (frame->nb_side_data > INT_MAX / sizeof(*frame->side_data) - 1)
>          goto fail;
>  
> +    ret = av_mallocz(sizeof(*ret));
> +    if (!ret)
> +        goto fail;
> +
>      tmp = av_realloc(frame->side_data,
>                       (frame->nb_side_data + 1) * sizeof(*frame->side_data));
>      if (!tmp)
>          goto fail;
>      frame->side_data = tmp;
>  
> -    ret = av_mallocz(sizeof(*ret));
> -    if (!ret)
> -        goto fail;
> -
> -    ret->buf = buf;
> +    ret->buf  = ref;
>      ret->data = ret->buf->data;
>      ret->size = buf->size;
>      ret->type = type;
> @@ -673,7 +669,8 @@ static AVFrameSideData *frame_new_side_data(AVFrame *frame,
>  
>      return ret;
>  fail:
> -    av_buffer_unref(&buf);
> +    av_buffer_unref(&ref);
> +    av_free(tmp);
>      return NULL;
>  }
>  
> @@ -681,8 +678,16 @@ 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);
> +    av_buffer_unref(&buf);
> +    if (!ret)
> +        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..504ddce073 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
> + *
> + * A new reference of the buffer will be created and used.
> + * On error, nothing will be changed and the function will return NULL.
> + *
> + * @param frame a frame to which the side data should be added
> + * @param type type of the added side data
> + * @param buf the AVBufferRef to add as side data
> + *
> + * @return newly added side data on success, NULL on error
> + */
> +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.
>

James Almer (2018-03-01):
> I prefer if the function takes ownership of the reference rather than
> create a new one on success.
> If it creates a new one, then the caller still needs to do something
> with the reference passed to the function afterwards regardless of
> outcome, as shown in your changes to av_frame_new_side_data().
> This is also extra overhead and one more point of failure for the
> function (creating the reference) for no real gain. Let the user decide
> if they want to keep the reference they passed or if they have no use
> for it and would rather have the side data take ownership of it.

I second all these arguments. Making an extra reference can always be
done by the caller. Avoiding the extra reference would not be possible.

It would have been nice to think of a consistent naming scheme for that,
though.

Regards,