Message ID | 20180301135512.24265-1-atomnuker@gmail.com |
---|---|
State | Superseded |
Headers | show |
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,
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.
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(-)