| Message ID | 20210810220001.13494-1-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/3] avutil/frame: add av_frame_replace | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | success | Make fate finished |
On 8/10/2021 6:59 PM, James Almer wrote: > Signed-off-by: James Almer <jamrial@gmail.com> > --- > Not going to bother with implementing replace for side data, since it's IMO not > worth it, but patches are welcome of course. > > Missing version bump and APIchanges entry. > > libavutil/frame.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++ > libavutil/frame.h | 18 +++++++++ > 2 files changed, 116 insertions(+) Will apply the set if there are no objections.
James Almer: > On 8/10/2021 6:59 PM, James Almer wrote: >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> Not going to bother with implementing replace for side data, since >> it's IMO not >> worth it, but patches are welcome of course. >> >> Missing version bump and APIchanges entry. >> >> libavutil/frame.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++ >> libavutil/frame.h | 18 +++++++++ >> 2 files changed, 116 insertions(+) > > Will apply the set if there are no objections. How many allocations are saved by this normally? (I.e. with a random H.264 file you have)? - Andreas
On 8/13/2021 12:22 PM, Andreas Rheinhardt wrote: > James Almer: >> On 8/10/2021 6:59 PM, James Almer wrote: >>> Signed-off-by: James Almer <jamrial@gmail.com> >>> --- >>> Not going to bother with implementing replace for side data, since >>> it's IMO not >>> worth it, but patches are welcome of course. >>> >>> Missing version bump and APIchanges entry. >>> >>> libavutil/frame.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++ >>> libavutil/frame.h | 18 +++++++++ >>> 2 files changed, 116 insertions(+) >> >> Will apply the set if there are no objections. > > How many allocations are saved by this normally? (I.e. with a random > H.264 file you have)? I looked at a VP9 file with patch 4/4, and it seemed to remove about a third of the overall allocations. > > - Andreas > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
James Almer: > Signed-off-by: James Almer <jamrial@gmail.com> > --- > Not going to bother with implementing replace for side data, since it's IMO not > worth it, but patches are welcome of course. > > Missing version bump and APIchanges entry. > > libavutil/frame.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++ > libavutil/frame.h | 18 +++++++++ > 2 files changed, 116 insertions(+) > > diff --git a/libavutil/frame.h b/libavutil/frame.h > index ff2540a20f..36ed128886 100644 > --- a/libavutil/frame.h > +++ b/libavutil/frame.h > @@ -695,6 +695,24 @@ 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. > + * > + * Copy frame properties from src to dst and create a new reference for each > + * AVBufferRef from src if they differ from those in dst. > + * > + * src must be reference counted. > + * > + * @param src The source frame. If there's data described in it, it must be > + * reference counted. > + * @param dst The destination frame. > + * > + * @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. > * > av_buffer_replace() is a no-op if src == dst (and documented to be so); this is not true for this function where you instantly wipe side data. - Andreas
On 8/13/2021 12:27 PM, Andreas Rheinhardt wrote: > James Almer: >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> Not going to bother with implementing replace for side data, since it's IMO not >> worth it, but patches are welcome of course. >> >> Missing version bump and APIchanges entry. >> >> libavutil/frame.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++ >> libavutil/frame.h | 18 +++++++++ >> 2 files changed, 116 insertions(+) >> >> diff --git a/libavutil/frame.h b/libavutil/frame.h >> index ff2540a20f..36ed128886 100644 >> --- a/libavutil/frame.h >> +++ b/libavutil/frame.h >> @@ -695,6 +695,24 @@ 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. >> + * >> + * Copy frame properties from src to dst and create a new reference for each >> + * AVBufferRef from src if they differ from those in dst. >> + * >> + * src must be reference counted. >> + * >> + * @param src The source frame. If there's data described in it, it must be >> + * reference counted. >> + * @param dst The destination frame. >> + * >> + * @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. >> * >> > av_buffer_replace() is a no-op if src == dst (and documented to be so); > this is not true for this function where you instantly wipe side data. Like i said above, I'm not going to try and implement replace for side data. It's just not worth the extra complexity. > > - Andreas > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
James Almer: > On 8/13/2021 12:27 PM, Andreas Rheinhardt wrote: >> James Almer: >>> Signed-off-by: James Almer <jamrial@gmail.com> >>> --- >>> Not going to bother with implementing replace for side data, since >>> it's IMO not >>> worth it, but patches are welcome of course. >>> >>> Missing version bump and APIchanges entry. >>> >>> libavutil/frame.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++ >>> libavutil/frame.h | 18 +++++++++ >>> 2 files changed, 116 insertions(+) >>> >>> diff --git a/libavutil/frame.h b/libavutil/frame.h >>> index ff2540a20f..36ed128886 100644 >>> --- a/libavutil/frame.h >>> +++ b/libavutil/frame.h >>> @@ -695,6 +695,24 @@ 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. >>> + * >>> + * Copy frame properties from src to dst and create a new reference >>> for each >>> + * AVBufferRef from src if they differ from those in dst. >>> + * >>> + * src must be reference counted. >>> + * >>> + * @param src The source frame. If there's data described in it, it >>> must be >>> + * reference counted. >>> + * @param dst The destination frame. >>> + * >>> + * @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. >>> * >>> >> av_buffer_replace() is a no-op if src == dst (and documented to be so); >> this is not true for this function where you instantly wipe side data. > > Like i said above, I'm not going to try and implement replace for side > data. It's just not worth the extra complexity. > I don't think you got my comment: It was not about saving allocations for side data; it is about what happens when src and dst coincide. According to the documentation, you should do nothing, because src and dst already refer to the same data. Instead you wipe the side data and modify the frame. - Andreas
diff --git a/libavutil/frame.c b/libavutil/frame.c index b0ceaf7145..035a48eb48 100644 --- a/libavutil/frame.c +++ b/libavutil/frame.c @@ -421,6 +421,104 @@ fail: return ret; } +int av_frame_replace(AVFrame *dst, const AVFrame *src) +{ + int i, ret = 0; + + if (!src->buf[0]) { + if ((src->extended_data && src->extended_data[0]) || src->data[0]) + ret = AVERROR(EINVAL); + + goto fail; + } + + dst->format = src->format; + dst->width = src->width; + dst->height = src->height; + dst->channels = src->channels; + dst->channel_layout = src->channel_layout; + dst->nb_samples = src->nb_samples; + + wipe_side_data(dst); + av_dict_free(&dst->metadata); + ret = frame_copy_props(dst, src, 0); + if (ret < 0) + goto fail; + + /* replace the buffers */ + for (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 (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 (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 (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->channels; + + if (!ch) { + ret = AVERROR(EINVAL); + goto fail; + } + CHECK_CHANNELS_CONSISTENCY(src); + + 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 ff2540a20f..36ed128886 100644 --- a/libavutil/frame.h +++ b/libavutil/frame.h @@ -695,6 +695,24 @@ 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. + * + * Copy frame properties from src to dst and create a new reference for each + * AVBufferRef from src if they differ from those in dst. + * + * src must be reference counted. + * + * @param src The source frame. If there's data described in it, it must be + * reference counted. + * @param dst The destination frame. + * + * @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> --- Not going to bother with implementing replace for side data, since it's IMO not worth it, but patches are welcome of course. Missing version bump and APIchanges entry. libavutil/frame.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++ libavutil/frame.h | 18 +++++++++ 2 files changed, 116 insertions(+)