[FFmpeg-devel] [PATCH] frame: add an av_frame_new_side_data_from_buf function

[James Almer]

On 10/16/2017 12:24 PM, Rostislav Pehlivanov wrote:
> On 16 October 2017 at 16:14, wm4 <nfxjfg at googlemail.com> wrote:
> 
>> On Mon, 16 Oct 2017 16:02:19 +0100
>> Rostislav Pehlivanov <atomnuker at gmail.com> wrote:
>>
>>> Signed-off-by: Rostislav Pehlivanov <atomnuker at gmail.com>
>>> ---
>>>  libavutil/frame.c | 16 ++++++----------
>>>  libavutil/frame.h | 13 +++++++++++++
>>>  2 files changed, 19 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/libavutil/frame.c b/libavutil/frame.c
>>> index d5fd2932e3..0668c888ea 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);
>>> -
>>>  MAKE_ACCESSORS(AVFrame, frame, int64_t, best_effort_timestamp)
>>>  MAKE_ACCESSORS(AVFrame, frame, int64_t, pkt_duration)
>>>  MAKE_ACCESSORS(AVFrame, frame, int64_t, pkt_pos)
>>> @@ -356,7 +351,8 @@ 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,
>>> +
>>  av_buffer_ref(sd_src->buf));
>>>              if (!sd_dst) {
>>>                  wipe_side_data(dst);
>>>                  return AVERROR(ENOMEM);
>>> @@ -636,9 +632,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;
>>>
>>> @@ -676,7 +672,7 @@ AVFrameSideData *av_frame_new_side_data(AVFrame
>> *frame,
>>>                                          int size)
>>>  {
>>>
>>> -    return frame_new_side_data(frame, type, av_buffer_alloc(size));
>>> +    return av_frame_new_side_data_from_buf(frame, type,
>> av_buffer_alloc(size));
>>>  }
>>>
>>>  AVFrameSideData *av_frame_get_side_data(const AVFrame *frame,
>>> diff --git a/libavutil/frame.h b/libavutil/frame.h
>>> index abe4f4fd17..1430d8363f 100644
>>> --- a/libavutil/frame.h
>>> +++ b/libavutil/frame.h
>>> @@ -762,6 +762,19 @@ 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 type of the added side data
>>> + * @param a valid AVBufferRef
>>> + *
>>> + * @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.
>>
>> LGTM, but doesn't document that it unrefs the buf on error. (We
>> probably want to change those semantics anyway?)
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
> 
> 
> Changed locally:
> 
>> - * @param a valid AVBufferRef
>> + * @param a valid AVBufferRef, unreferenced on error

I don't think this is a good idea. As with other similar APIs, on error
the AVFrame should be unchanged and the buffer remain intact and owned
by the caller. On success the ownership passes to the AVFrame.