diff mbox series

[FFmpeg-devel,20/42] avcodec/nvdec: Use RefStruct-pool API for decoder pool

Message ID AS8P250MB074433F9BB500D92E2D747228FFAA@AS8P250MB0744.EURP250.PROD.OUTLOOK.COM
State Accepted
Commit e01e30ede1d0a06df90c0c7816cb51941bcd3bc9
Headers show
Series New API for reference counting and ThreadFrames | expand

Commit Message

Andreas Rheinhardt Sept. 19, 2023, 7:57 p.m. UTC 
It involves less allocations, in particular no allocations
after the entry has been created. Therefore creating a new
reference from an existing one can't fail and therefore
need not be checked. It also avoids indirections and casts.

Also note that nvdec_decoder_frame_init() (the callback
to initialize new entries from the pool) does not use
atomics to read and replace the number of entries
currently used by the pool. This relies on nvdec (like
most other hwaccels) not being run in a truely frame-threaded
way.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
Notice that the AVBufferPool API serializes creating new entries
as well as getting an already existing entry from the pool,
so not using atomics here would be fine for it even if nvdec
were run in a truely multithreaded way.

 libavcodec/nvdec.c | 49 +++++++++++++++++++++++-----------------------
 libavcodec/nvdec.h |  6 +++---
 2 files changed, 27 insertions(+), 28 deletions(-)

Comments

Anton Khirnov Oct. 4, 2023, 2:28 p.m. UTC | #1 
Quoting Andreas Rheinhardt (2023-09-19 21:57:12)
> It involves less allocations, in particular no allocations
> after the entry has been created. Therefore creating a new
> reference from an existing one can't fail and therefore
> need not be checked. It also avoids indirections and casts.
> 
> Also note that nvdec_decoder_frame_init() (the callback
> to initialize new entries from the pool) does not use
> atomics to read and replace the number of entries
> currently used by the pool. This relies on nvdec (like
> most other hwaccels) not being run in a truely frame-threaded
> way.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
> Notice that the AVBufferPool API serializes creating new entries
> as well as getting an already existing entry from the pool,
> so not using atomics here would be fine for it even if nvdec
> were run in a truely multithreaded way.

AFAIK the API does not allow it, so there's nothing we can do about it.

Patch LGTM
diff mbox series

Patch

diff --git a/libavcodec/nvdec.c b/libavcodec/nvdec.c
index 0ec8e896a6..27be644356 100644
--- a/libavcodec/nvdec.c
+++ b/libavcodec/nvdec.c
@@ -237,21 +237,22 @@  fail:
     return ret;
 }
 
-static AVBufferRef *nvdec_decoder_frame_alloc(void *opaque, size_t size)
+static int nvdec_decoder_frame_init(FFRefStructOpaque opaque, void *obj)
 {
-    NVDECFramePool *pool = opaque;
-    AVBufferRef *ret;
+    NVDECFramePool *pool = opaque.nc;
+    unsigned int *intp = obj;
 
     if (pool->nb_allocated >= pool->dpb_size)
-        return NULL;
+        return AVERROR(ENOMEM);
 
-    ret = av_buffer_alloc(sizeof(unsigned int));
-    if (!ret)
-        return NULL;
+    *intp = pool->nb_allocated++;
 
-    *(unsigned int*)ret->data = pool->nb_allocated++;
+    return 0;
+}
 
-    return ret;
+static void nvdec_decoder_frame_pool_free(FFRefStructOpaque opaque)
+{
+    av_free(opaque.nc);
 }
 
 int ff_nvdec_decode_uninit(AVCodecContext *avctx)
@@ -268,7 +269,7 @@  int ff_nvdec_decode_uninit(AVCodecContext *avctx)
     ctx->slice_offsets_allocated = 0;
 
     ff_refstruct_unref(&ctx->decoder);
-    av_buffer_pool_uninit(&ctx->decoder_pool);
+    ff_refstruct_pool_uninit(&ctx->decoder_pool);
 
     return 0;
 }
@@ -424,8 +425,9 @@  int ff_nvdec_decode_init(AVCodecContext *avctx)
     }
     pool->dpb_size = frames_ctx->initial_pool_size;
 
-    ctx->decoder_pool = av_buffer_pool_init2(sizeof(int), pool,
-                                             nvdec_decoder_frame_alloc, av_free);
+    ctx->decoder_pool = ff_refstruct_pool_alloc_ext(sizeof(unsigned int), 0, pool,
+                                                    nvdec_decoder_frame_init,
+                                                    NULL, NULL, nvdec_decoder_frame_pool_free);
     if (!ctx->decoder_pool) {
         ret = AVERROR(ENOMEM);
         goto fail;
@@ -444,8 +446,8 @@  static void nvdec_fdd_priv_free(void *priv)
     if (!cf)
         return;
 
-    av_buffer_unref(&cf->idx_ref);
-    av_buffer_unref(&cf->ref_idx_ref);
+    ff_refstruct_unref(&cf->idx_ref);
+    ff_refstruct_unref(&cf->ref_idx_ref);
     ff_refstruct_unref(&cf->decoder);
 
     av_freep(&priv);
@@ -469,8 +471,8 @@  static void nvdec_unmap_mapped_frame(void *opaque, uint8_t *data)
     CHECK_CU(decoder->cudl->cuCtxPopCurrent(&dummy));
 
 finish:
-    av_buffer_unref(&unmap_data->idx_ref);
-    av_buffer_unref(&unmap_data->ref_idx_ref);
+    ff_refstruct_unref(&unmap_data->idx_ref);
+    ff_refstruct_unref(&unmap_data->ref_idx_ref);
     ff_refstruct_unref(&unmap_data->decoder);
     av_free(unmap_data);
 }
@@ -526,10 +528,7 @@  static int nvdec_retrieve_data(void *logctx, AVFrame *frame)
         goto copy_fail;
 
     unmap_data->idx = cf->idx;
-    if (!(unmap_data->idx_ref     = av_buffer_ref(cf->idx_ref))) {
-        ret = AVERROR(ENOMEM);
-        goto copy_fail;
-    }
+    unmap_data->idx_ref = ff_refstruct_ref(cf->idx_ref);
     unmap_data->decoder = ff_refstruct_ref(cf->decoder);
 
     av_pix_fmt_get_chroma_sub_sample(hwctx->sw_format, &shift_h, &shift_v);
@@ -577,13 +576,13 @@  int ff_nvdec_start_frame(AVCodecContext *avctx, AVFrame *frame)
 
     cf->decoder = ff_refstruct_ref(ctx->decoder);
 
-    cf->idx_ref = av_buffer_pool_get(ctx->decoder_pool);
+    cf->idx_ref = ff_refstruct_pool_get(ctx->decoder_pool);
     if (!cf->idx_ref) {
         av_log(avctx, AV_LOG_ERROR, "No decoder surfaces left\n");
         ret = AVERROR(ENOMEM);
         goto fail;
     }
-    cf->ref_idx = cf->idx = *(unsigned int*)cf->idx_ref->data;
+    cf->ref_idx = cf->idx = *cf->idx_ref;
 
     fdd->hwaccel_priv      = cf;
     fdd->hwaccel_priv_free = nvdec_fdd_priv_free;
@@ -611,16 +610,16 @@  int ff_nvdec_start_frame_sep_ref(AVCodecContext *avctx, AVFrame *frame, int has_
 
     if (has_sep_ref) {
         if (!cf->ref_idx_ref) {
-            cf->ref_idx_ref = av_buffer_pool_get(ctx->decoder_pool);
+            cf->ref_idx_ref = ff_refstruct_pool_get(ctx->decoder_pool);
             if (!cf->ref_idx_ref) {
                 av_log(avctx, AV_LOG_ERROR, "No decoder surfaces left\n");
                 ret = AVERROR(ENOMEM);
                 goto fail;
             }
         }
-        cf->ref_idx = *(unsigned int*)cf->ref_idx_ref->data;
+        cf->ref_idx = *cf->ref_idx_ref;
     } else {
-        av_buffer_unref(&cf->ref_idx_ref);
+        ff_refstruct_unref(&cf->ref_idx_ref);
         cf->ref_idx = cf->idx;
     }
 
diff --git a/libavcodec/nvdec.h b/libavcodec/nvdec.h
index efacb5b691..353e95bf42 100644
--- a/libavcodec/nvdec.h
+++ b/libavcodec/nvdec.h
@@ -44,15 +44,15 @@ 
 typedef struct NVDECFrame {
     unsigned int idx;
     unsigned int ref_idx;
-    AVBufferRef *idx_ref;
-    AVBufferRef *ref_idx_ref;
+    unsigned int *idx_ref;         ///< RefStruct reference
+    unsigned int *ref_idx_ref;     ///< RefStruct reference
     struct NVDECDecoder  *decoder; ///< RefStruct reference
 } NVDECFrame;
 
 typedef struct NVDECContext {
     CUVIDPICPARAMS pic_params;
 
-    AVBufferPool *decoder_pool;
+    struct FFRefStructPool *decoder_pool;
 
     struct NVDECDecoder  *decoder; ///< RefStruct reference