| Message ID | 20220605174458.1942-1-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,v2] avutil/frame: add av_frame_replace | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_armv7_RPi4 | success | Make finished |
| andriy/make_fate_armv7_RPi4 | success | Make fate finished |
Quoting James Almer (2022-06-05 19:44:58) > + > + /* duplicate the frame data if it's not refcounted */ > + if (!src->buf[0]) { > + for (int i = 0; i < FF_ARRAY_ELEMS(dst->buf); i++) > + av_buffer_unref(&dst->buf[i]); > + for (int i = 0; i < dst->nb_extended_buf; i++) > + av_buffer_unref(&dst->extended_buf[i]); > + av_freep(&dst->extended_buf); > + > + memset(dst->data, 0, sizeof(dst->data)); > + if (dst->extended_data != dst->data) > + av_freep(&dst->extended_data); > + > + ret = av_frame_get_buffer(dst, 0); > + if (ret < 0) > + goto fail; > + > + ret = av_frame_copy(dst, src); > + if (ret < 0) > + goto fail; > + > + ret = av_buffer_replace(&dst->hw_frames_ctx, src->hw_frames_ctx); > + if (ret < 0) > + goto fail; This looks suspicious, since we just allocated normal buffers for dst. Also, can't t this whole block be replaced by just if (!src->buf[0]) { av_frame_unref(dst); return av_frame_ref(dst, src); } at the beginning of this function? > + if (src->extended_data != src->data) { > + int ch = src->ch_layout.nb_channels; > + > +#if FF_API_OLD_CHANNEL_LAYOUT > +FF_DISABLE_DEPRECATION_WARNINGS > + if (!ch) { > + ch = src->channels; > + CHECK_CHANNELS_CONSISTENCY(src); > + } > +FF_ENABLE_DEPRECATION_WARNINGS > +#endif > + if (!ch) { > + ret = AVERROR(EINVAL); > + goto fail; > + } > + > + dst->extended_data = av_malloc_array(sizeof(*dst->extended_data), ch); > + if (!dst->extended_data) { > + ret = AVERROR(ENOMEM); > + goto fail; > + } > + memcpy(dst->extended_data, src->extended_data, sizeof(*src->extended_data) * ch); nit: av_memdup()? > + } else > + dst->extended_data = dst->data; > + > + memcpy(dst->data, src->data, sizeof(src->data)); > + memcpy(dst->linesize, src->linesize, sizeof(src->linesize)); > + > + return 0; > + > +fail: > + av_frame_unref(dst); > + return ret; > +} > + > AVFrame *av_frame_clone(const AVFrame *src) > { > AVFrame *ret = av_frame_alloc(); > diff --git a/libavutil/frame.h b/libavutil/frame.h > index 33fac2054c..e5c10e2b66 100644 > --- a/libavutil/frame.h > +++ b/libavutil/frame.h > @@ -752,6 +752,19 @@ void av_frame_free(AVFrame **frame); > */ > int av_frame_ref(AVFrame *dst, const AVFrame *src); > > +/** > + * Ensure the destination frame refers to the same data described by the source > + * frame by creating a new reference for each AVBufferRef from src if they > + * differ from those in dst, or if src is not reference counted, by allocating > + * new buffers and copying data. > + * > + * Frame properties on dst will be replaced by those from src. > + * > + * @return 0 on success, a negative AVERROR on error. On error, dst is > + * unreferenced. > + */ > +int av_frame_replace(AVFrame *dst, const AVFrame *src); An important property of av_buffer_replace() is that it's equivalent to av_buffer_unref(dst) when src is NULL. It would probably be desirable for av_frame_replace() to work the same way - currently it will try to call av_frame_get_buffer() with invalid parameters and fail.
On 6/7/2022 7:21 AM, Anton Khirnov wrote: > Quoting James Almer (2022-06-05 19:44:58) >> + >> + /* duplicate the frame data if it's not refcounted */ >> + if (!src->buf[0]) { >> + for (int i = 0; i < FF_ARRAY_ELEMS(dst->buf); i++) >> + av_buffer_unref(&dst->buf[i]); >> + for (int i = 0; i < dst->nb_extended_buf; i++) >> + av_buffer_unref(&dst->extended_buf[i]); >> + av_freep(&dst->extended_buf); >> + >> + memset(dst->data, 0, sizeof(dst->data)); >> + if (dst->extended_data != dst->data) >> + av_freep(&dst->extended_data); >> + >> + ret = av_frame_get_buffer(dst, 0); >> + if (ret < 0) >> + goto fail; >> + >> + ret = av_frame_copy(dst, src); >> + if (ret < 0) >> + goto fail; >> + >> + ret = av_buffer_replace(&dst->hw_frames_ctx, src->hw_frames_ctx); >> + if (ret < 0) >> + goto fail; > > This looks suspicious, since we just allocated normal buffers for dst. Right, removed. > Also, can't t this whole block be replaced by just > > if (!src->buf[0]) { > av_frame_unref(dst); > return av_frame_ref(dst, src); > } > > at the beginning of this function? Yes, but i was copying how av_frame_ref() handled this scenario, albeit with a bit more of code. Will change, but it will need to be re-added if someone at some point decides to implement replace for side data. > >> + if (src->extended_data != src->data) { >> + int ch = src->ch_layout.nb_channels; >> + >> +#if FF_API_OLD_CHANNEL_LAYOUT >> +FF_DISABLE_DEPRECATION_WARNINGS >> + if (!ch) { >> + ch = src->channels; >> + CHECK_CHANNELS_CONSISTENCY(src); >> + } >> +FF_ENABLE_DEPRECATION_WARNINGS >> +#endif >> + if (!ch) { >> + ret = AVERROR(EINVAL); >> + goto fail; >> + } >> + >> + dst->extended_data = av_malloc_array(sizeof(*dst->extended_data), ch); >> + if (!dst->extended_data) { >> + ret = AVERROR(ENOMEM); >> + goto fail; >> + } >> + memcpy(dst->extended_data, src->extended_data, sizeof(*src->extended_data) * ch); > > nit: av_memdup()? > >> + } else >> + dst->extended_data = dst->data; >> + >> + memcpy(dst->data, src->data, sizeof(src->data)); >> + memcpy(dst->linesize, src->linesize, sizeof(src->linesize)); >> + >> + return 0; >> + >> +fail: >> + av_frame_unref(dst); >> + return ret; >> +} >> + >> AVFrame *av_frame_clone(const AVFrame *src) >> { >> AVFrame *ret = av_frame_alloc(); >> diff --git a/libavutil/frame.h b/libavutil/frame.h >> index 33fac2054c..e5c10e2b66 100644 >> --- a/libavutil/frame.h >> +++ b/libavutil/frame.h >> @@ -752,6 +752,19 @@ void av_frame_free(AVFrame **frame); >> */ >> int av_frame_ref(AVFrame *dst, const AVFrame *src); >> >> +/** >> + * Ensure the destination frame refers to the same data described by the source >> + * frame by creating a new reference for each AVBufferRef from src if they >> + * differ from those in dst, or if src is not reference counted, by allocating >> + * new buffers and copying data. >> + * >> + * Frame properties on dst will be replaced by those from src. >> + * >> + * @return 0 on success, a negative AVERROR on error. On error, dst is >> + * unreferenced. >> + */ >> +int av_frame_replace(AVFrame *dst, const AVFrame *src); > > An important property of av_buffer_replace() is that it's equivalent to > av_buffer_unref(dst) when src is NULL. It would probably be desirable > for av_frame_replace() to work the same way - currently it will try to > call av_frame_get_buffer() with invalid parameters and fail. Should it really accept NULL as src, or you meant a recently allocated/unreff'd frame as src (So src->data[0] == NULL)?
Quoting James Almer (2022-06-07 13:39:55) > >> diff --git a/libavutil/frame.h b/libavutil/frame.h > >> index 33fac2054c..e5c10e2b66 100644 > >> --- a/libavutil/frame.h > >> +++ b/libavutil/frame.h > >> @@ -752,6 +752,19 @@ void av_frame_free(AVFrame **frame); > >> */ > >> int av_frame_ref(AVFrame *dst, const AVFrame *src); > >> > >> +/** > >> + * Ensure the destination frame refers to the same data described by the source > >> + * frame by creating a new reference for each AVBufferRef from src if they > >> + * differ from those in dst, or if src is not reference counted, by allocating > >> + * new buffers and copying data. > >> + * > >> + * Frame properties on dst will be replaced by those from src. > >> + * > >> + * @return 0 on success, a negative AVERROR on error. On error, dst is > >> + * unreferenced. > >> + */ > >> +int av_frame_replace(AVFrame *dst, const AVFrame *src); > > > > An important property of av_buffer_replace() is that it's equivalent to > > av_buffer_unref(dst) when src is NULL. It would probably be desirable > > for av_frame_replace() to work the same way - currently it will try to > > call av_frame_get_buffer() with invalid parameters and fail. > > Should it really accept NULL as src, or you meant a recently > allocated/unreff'd frame as src (So src->data[0] == NULL)? I mean an equivalent of NULL buffer, so an empty (but non-NULL) frame. So all of src->data[] == NULL (some hwaccel pixfmts store things only in data[3] for hysterical raisins).
diff --git a/libavutil/frame.c b/libavutil/frame.c index 4c16488c66..86f9dcb59a 100644 --- a/libavutil/frame.c +++ b/libavutil/frame.c @@ -461,6 +461,153 @@ fail: return ret; } +int av_frame_replace(AVFrame *dst, const AVFrame *src) +{ + int ret = 0; + + if (dst == src) + return AVERROR(EINVAL); + + dst->format = src->format; + dst->width = src->width; + dst->height = src->height; + dst->nb_samples = src->nb_samples; +#if FF_API_OLD_CHANNEL_LAYOUT +FF_DISABLE_DEPRECATION_WARNINGS + dst->channels = src->channels; + dst->channel_layout = src->channel_layout; + if (!av_channel_layout_check(&src->ch_layout)) { + av_channel_layout_uninit(&dst->ch_layout); + if (src->channel_layout) + av_channel_layout_from_mask(&dst->ch_layout, src->channel_layout); + else { + dst->ch_layout.nb_channels = src->channels; + dst->ch_layout.order = AV_CHANNEL_ORDER_UNSPEC; + } + } else { +#endif + ret = av_channel_layout_copy(&dst->ch_layout, &src->ch_layout); + if (ret < 0) + goto fail; +#if FF_API_OLD_CHANNEL_LAYOUT + } +FF_ENABLE_DEPRECATION_WARNINGS +#endif + + wipe_side_data(dst); + av_dict_free(&dst->metadata); + ret = frame_copy_props(dst, src, 0); + if (ret < 0) + goto fail; + + /* duplicate the frame data if it's not refcounted */ + if (!src->buf[0]) { + for (int i = 0; i < FF_ARRAY_ELEMS(dst->buf); i++) + av_buffer_unref(&dst->buf[i]); + for (int i = 0; i < dst->nb_extended_buf; i++) + av_buffer_unref(&dst->extended_buf[i]); + av_freep(&dst->extended_buf); + + memset(dst->data, 0, sizeof(dst->data)); + if (dst->extended_data != dst->data) + av_freep(&dst->extended_data); + + ret = av_frame_get_buffer(dst, 0); + if (ret < 0) + goto fail; + + ret = av_frame_copy(dst, src); + if (ret < 0) + goto fail; + + ret = av_buffer_replace(&dst->hw_frames_ctx, src->hw_frames_ctx); + if (ret < 0) + goto fail; + + return 0; + } + + /* replace the buffers */ + for (int i = 0; i < FF_ARRAY_ELEMS(src->buf); i++) { + ret = av_buffer_replace(&dst->buf[i], src->buf[i]); + if (ret < 0) + goto fail; + } + + if (src->extended_buf) { + if (dst->nb_extended_buf != src->nb_extended_buf) { + int nb_extended_buf = FFMIN(dst->nb_extended_buf, src->nb_extended_buf); + void *tmp; + + for (int i = nb_extended_buf; i < dst->nb_extended_buf; i++) + av_buffer_unref(&dst->extended_buf[i]); + + tmp = av_realloc_array(dst->extended_buf, sizeof(*dst->extended_buf), + src->nb_extended_buf); + if (!tmp) { + ret = AVERROR(ENOMEM); + goto fail; + } + dst->extended_buf = tmp; + dst->nb_extended_buf = src->nb_extended_buf; + + memset(&dst->extended_buf[nb_extended_buf], 0, + (src->nb_extended_buf - nb_extended_buf) * sizeof(*dst->extended_buf)); + } + + for (int i = 0; i < src->nb_extended_buf; i++) { + ret = av_buffer_replace(&dst->extended_buf[i], src->extended_buf[i]); + if (ret < 0) + goto fail; + } + } else if (dst->extended_buf) { + for (int i = 0; i < dst->nb_extended_buf; i++) + av_buffer_unref(&dst->extended_buf[i]); + av_freep(&dst->extended_buf); + } + + ret = av_buffer_replace(&dst->hw_frames_ctx, src->hw_frames_ctx); + if (ret < 0) + goto fail; + + if (dst->extended_data != dst->data) + av_freep(&dst->extended_data); + + if (src->extended_data != src->data) { + int ch = src->ch_layout.nb_channels; + +#if FF_API_OLD_CHANNEL_LAYOUT +FF_DISABLE_DEPRECATION_WARNINGS + if (!ch) { + ch = src->channels; + CHECK_CHANNELS_CONSISTENCY(src); + } +FF_ENABLE_DEPRECATION_WARNINGS +#endif + if (!ch) { + ret = AVERROR(EINVAL); + goto fail; + } + + dst->extended_data = av_malloc_array(sizeof(*dst->extended_data), ch); + if (!dst->extended_data) { + ret = AVERROR(ENOMEM); + goto fail; + } + memcpy(dst->extended_data, src->extended_data, sizeof(*src->extended_data) * ch); + } else + dst->extended_data = dst->data; + + memcpy(dst->data, src->data, sizeof(src->data)); + memcpy(dst->linesize, src->linesize, sizeof(src->linesize)); + + return 0; + +fail: + av_frame_unref(dst); + return ret; +} + AVFrame *av_frame_clone(const AVFrame *src) { AVFrame *ret = av_frame_alloc(); diff --git a/libavutil/frame.h b/libavutil/frame.h index 33fac2054c..e5c10e2b66 100644 --- a/libavutil/frame.h +++ b/libavutil/frame.h @@ -752,6 +752,19 @@ void av_frame_free(AVFrame **frame); */ int av_frame_ref(AVFrame *dst, const AVFrame *src); +/** + * Ensure the destination frame refers to the same data described by the source + * frame by creating a new reference for each AVBufferRef from src if they + * differ from those in dst, or if src is not reference counted, by allocating + * new buffers and copying data. + * + * Frame properties on dst will be replaced by those from src. + * + * @return 0 on success, a negative AVERROR on error. On error, dst is + * unreferenced. + */ +int av_frame_replace(AVFrame *dst, const AVFrame *src); + /** * Create a new frame that references the same data as src. *
Signed-off-by: James Almer <jamrial@gmail.com> --- Now disallowing src == dst. libavutil/frame.c | 147 ++++++++++++++++++++++++++++++++++++++++++++++ libavutil/frame.h | 13 ++++ 2 files changed, 160 insertions(+)