diff mbox series

[FFmpeg-devel] avutil/hwcontext: add support to allow hardware to ref/unref frame

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
Related show

Checks

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

Commit Message

Suji Velupillai March 4, 2021, 6:19 a.m. UTC
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(-)

Comments

Hendrik Leppkes March 4, 2021, 8:06 a.m. UTC | #1
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
Suji Velupillai March 4, 2021, 6:59 p.m. UTC | #2
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".
James Almer March 4, 2021, 7:13 p.m. UTC | #3
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 mbox series

Patch

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);