| Message ID | 20210304061909.8114-1-suji.velupillai@broadcom.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avutil/hwcontext: add support to allow hardware to ref/unref frame | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make_warn | warning | New warnings during build |
| 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 Thu, Mar 4, 2021 at 7:20 AM <suji.velupillai@broadcom.com> wrote: > > From: Patrick Rault <patrick.rault@broadcom.com> > > Add support to allow attached hardware to add/remove reference to the > frame buffer mirroring the ffmpeg. > > Signed-off-by: Patrick Rault <patrick.rault@broadcom.com> > Signed-off-by: Suji Velupillai <suji.velupillai@broadcom.com> New API additions should include an explanation of which problems this solves, how it should be used, and why it is necessary. Otherwise we have no reference to even judge the usefulness of such an API. Looking at the patch, the change to av_frame_ref looks fishy to me. Why would dst reference itself, and not src? - Hendrik
Thank you Hendrik for your feedback. Let me look into it addressing it. Thank you Suji On Thu, Mar 4, 2021 at 12:11 AM Hendrik Leppkes <h.leppkes@gmail.com> wrote: > On Thu, Mar 4, 2021 at 7:20 AM <suji.velupillai@broadcom.com> wrote: > > > > From: Patrick Rault <patrick.rault@broadcom.com> > > > > Add support to allow attached hardware to add/remove reference to the > > frame buffer mirroring the ffmpeg. > > > > Signed-off-by: Patrick Rault <patrick.rault@broadcom.com> > > Signed-off-by: Suji Velupillai <suji.velupillai@broadcom.com> > > New API additions should include an explanation of which problems this > solves, how it should be used, and why it is necessary. Otherwise we > have no reference to even judge the usefulness of such an API. > > Looking at the patch, the change to av_frame_ref looks fishy to me. > Why would dst reference itself, and not src? > > - Hendrik > _______________________________________________ > 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".
On 3/4/2021 3:19 AM, suji.velupillai@broadcom.com wrote: > From: Patrick Rault <patrick.rault@broadcom.com> > > Add support to allow attached hardware to add/remove reference to the > frame buffer mirroring the ffmpeg. > > Signed-off-by: Patrick Rault <patrick.rault@broadcom.com> > Signed-off-by: Suji Velupillai <suji.velupillai@broadcom.com> > --- > libavutil/frame.c | 7 ++++++- > libavutil/hwcontext.c | 21 +++++++++++++++++++++ > libavutil/hwcontext.h | 15 +++++++++++++++ > libavutil/hwcontext_internal.h | 2 ++ > 4 files changed, 44 insertions(+), 1 deletion(-) > > diff --git a/libavutil/frame.c b/libavutil/frame.c > index eab51b6a32..6f8db14bf5 100644 > --- a/libavutil/frame.c > +++ b/libavutil/frame.c > @@ -22,6 +22,7 @@ > #include "common.h" > #include "dict.h" > #include "frame.h" > +#include "hwcontext.h" > #include "imgutils.h" > #include "mem.h" > #include "samplefmt.h" > @@ -492,7 +493,7 @@ int av_frame_ref(AVFrame *dst, const AVFrame *src) > dst->nb_extended_buf = src->nb_extended_buf; > > for (i = 0; i < src->nb_extended_buf; i++) { > - dst->extended_buf[i] = av_buffer_ref(src->extended_buf[i]); > + dst->extended_buf[i] = av_buffer_ref(dst); dst is an AVFrame, not AVBufferRef.
diff --git a/libavutil/frame.c b/libavutil/frame.c index eab51b6a32..6f8db14bf5 100644 --- a/libavutil/frame.c +++ b/libavutil/frame.c @@ -22,6 +22,7 @@ #include "common.h" #include "dict.h" #include "frame.h" +#include "hwcontext.h" #include "imgutils.h" #include "mem.h" #include "samplefmt.h" @@ -492,7 +493,7 @@ int av_frame_ref(AVFrame *dst, const AVFrame *src) dst->nb_extended_buf = src->nb_extended_buf; for (i = 0; i < src->nb_extended_buf; i++) { - dst->extended_buf[i] = av_buffer_ref(src->extended_buf[i]); + dst->extended_buf[i] = av_buffer_ref(dst); if (!dst->extended_buf[i]) { ret = AVERROR(ENOMEM); goto fail; @@ -530,6 +531,8 @@ int av_frame_ref(AVFrame *dst, const AVFrame *src) memcpy(dst->data, src->data, sizeof(src->data)); memcpy(dst->linesize, src->linesize, sizeof(src->linesize)); + av_hwframe_ref_buffer(dst); + return 0; fail: @@ -571,6 +574,8 @@ FF_DISABLE_DEPRECATION_WARNINGS FF_ENABLE_DEPRECATION_WARNINGS #endif + av_hwframe_unref_buffer(frame); + av_buffer_unref(&frame->hw_frames_ctx); av_buffer_unref(&frame->opaque_ref); diff --git a/libavutil/hwcontext.c b/libavutil/hwcontext.c index d13d0f7c9b..5e2209725e 100644 --- a/libavutil/hwcontext.c +++ b/libavutil/hwcontext.c @@ -562,6 +562,27 @@ int av_hwframe_get_buffer(AVBufferRef *hwframe_ref, AVFrame *frame, int flags) return 0; } +int av_hwframe_ref_buffer(AVFrame *frame) +{ + int ret = 0; + + if (frame->hw_frames_ctx) { + AVHWFramesContext *ctx = (AVHWFramesContext*)frame->hw_frames_ctx->data; + if (ctx->internal->hw_type->frames_ref_buffer) + ret = ctx->internal->hw_type->frames_ref_buffer(frame); + } + return ret; +} + +void av_hwframe_unref_buffer(AVFrame *frame) +{ + if (frame->hw_frames_ctx) { + AVHWFramesContext *ctx = (AVHWFramesContext*)frame->hw_frames_ctx->data; + if (ctx->internal->hw_type->frames_unref_buffer) + ctx->internal->hw_type->frames_unref_buffer(frame); + } +} + void *av_hwdevice_hwconfig_alloc(AVBufferRef *ref) { AVHWDeviceContext *ctx = (AVHWDeviceContext*)ref->data; diff --git a/libavutil/hwcontext.h b/libavutil/hwcontext.h index 04d19d89c2..9dc74b9e46 100644 --- a/libavutil/hwcontext.h +++ b/libavutil/hwcontext.h @@ -381,6 +381,21 @@ int av_hwframe_ctx_init(AVBufferRef *ref); */ int av_hwframe_get_buffer(AVBufferRef *hwframe_ctx, AVFrame *frame, int flags); +/** + * Allow hardware to add a reference on a buffer + * + * @param frame Hardware to add reference to the frame + * @return 0 on success, a negative AVERROR code on failure + */ +int av_hwframe_ref_buffer(AVFrame *frame); + +/** + * Allow to release hardware specific attached to the frame + * + * @param frame Hardware to release reference on the frame + */ +void av_hwframe_unref_buffer(AVFrame *frame); + /** * Copy data to or from a hw surface. At least one of dst/src must have an * AVHWFramesContext attached. diff --git a/libavutil/hwcontext_internal.h b/libavutil/hwcontext_internal.h index e6266494ac..fde34932c6 100644 --- a/libavutil/hwcontext_internal.h +++ b/libavutil/hwcontext_internal.h @@ -81,6 +81,8 @@ typedef struct HWContextType { void (*frames_uninit)(AVHWFramesContext *ctx); int (*frames_get_buffer)(AVHWFramesContext *ctx, AVFrame *frame); + int (*frames_ref_buffer)(AVFrame *frame); + void (*frames_unref_buffer)(AVFrame *frame); int (*transfer_get_formats)(AVHWFramesContext *ctx, enum AVHWFrameTransferDirection dir, enum AVPixelFormat **formats);