diff mbox series

[FFmpeg-devel,3/5] avcodec/av1dec: Use ProgressFrames

Message ID AS8P250MB0744DEFE4DC7B0B12D4B40FD8F0D2@AS8P250MB0744.EURP250.PROD.OUTLOOK.COM
State Accepted
Commit 0f8763fbea4e8816cd54c2a481d4c048fec58394
Headers show
Series [FFmpeg-devel,1/5] avcodec/progressframe: Explain how unnamed union can simplify accesses | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Andreas Rheinhardt April 19, 2024, 4:07 p.m. UTC
AV1 can put a frame into multiple reference slots;
up until now, this involved creating a new reference
to the underlying AVFrame; therefore av1_frame_ref()
could fail.
This commit changes this by using the ProgressFrame API
to share the underlying AVFrames.

(Hint: vaapi_av1_surface_id() checked whether the AV1Frames
contained in the AV1DecContext were NULL or not (of course
they were not); this has been changed to actually check for
whether said AV1Frame is blank or not.)

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
1. The AV1 patches are basically untested (apart from FATE and
compilation). Could someone please test them?
2. An alternative is to use an array of AV1Frames and an array of
pointers to AV1Frames. The former have refcounts where each pointer
to an AV1Frame is considered to own a reference. av1_frame_ref()
would boil down to incrementing a refcount and setting a pointer.
3. Is there a specific reason that vulkan checked pict_type
instead of setting whether e.g. buf[0] or data[0] was set
(as is customarily done)?

 libavcodec/av1dec.c     | 92 +++++++++++------------------------------
 libavcodec/av1dec.h     |  8 +++-
 libavcodec/dxva2_av1.c  |  8 ++--
 libavcodec/nvdec_av1.c  |  4 +-
 libavcodec/vaapi_av1.c  |  6 +--
 libavcodec/vdpau_av1.c  |  7 ++--
 libavcodec/vulkan_av1.c |  4 +-
 7 files changed, 47 insertions(+), 82 deletions(-)

Comments

James Almer April 20, 2024, 8:22 p.m. UTC | #1
On 4/19/2024 1:07 PM, Andreas Rheinhardt wrote:
> AV1 can put a frame into multiple reference slots;
> up until now, this involved creating a new reference
> to the underlying AVFrame; therefore av1_frame_ref()
> could fail.
> This commit changes this by using the ProgressFrame API
> to share the underlying AVFrames.
> 
> (Hint: vaapi_av1_surface_id() checked whether the AV1Frames
> contained in the AV1DecContext were NULL or not (of course
> they were not); this has been changed to actually check for
> whether said AV1Frame is blank or not.)
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
> 1. The AV1 patches are basically untested (apart from FATE and
> compilation). Could someone please test them?

I tested this and patch 4/5 with d3d11/12, cuda and vulkan. No issues.
Can't test vaapi.
diff mbox series

Patch

diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c
index b4b741054a..2a1a249bc4 100644
--- a/libavcodec/av1dec.c
+++ b/libavcodec/av1dec.c
@@ -38,8 +38,8 @@ 
 #include "itut35.h"
 #include "hwconfig.h"
 #include "profiles.h"
+#include "progressframe.h"
 #include "refstruct.h"
-#include "thread.h"
 
 /**< same with Div_Lut defined in spec 7.11.3.7 */
 static const uint16_t div_lut[AV1_DIV_LUT_NUM] = {
@@ -672,7 +672,7 @@  static int get_pixel_format(AVCodecContext *avctx)
 
 static void av1_frame_unref(AV1Frame *f)
 {
-    av_frame_unref(f->f);
+    ff_progress_frame_unref(&f->pf);
     ff_refstruct_unref(&f->hwaccel_picture_private);
     ff_refstruct_unref(&f->header_ref);
     f->raw_frame_header = NULL;
@@ -683,20 +683,16 @@  static void av1_frame_unref(AV1Frame *f)
     f->coded_lossless = 0;
 }
 
-static int av1_frame_ref(AVCodecContext *avctx, AV1Frame *dst, const AV1Frame *src)
+static void av1_frame_ref(AVCodecContext *avctx, AV1Frame *dst, const AV1Frame *src)
 {
-    int ret;
-
     ff_refstruct_replace(&dst->header_ref, src->header_ref);
 
     dst->raw_frame_header = src->raw_frame_header;
 
-    if (!src->f->buf[0])
-        return 0;
+    if (!src->f)
+        return;
 
-    ret = av_frame_ref(dst->f, src->f);
-    if (ret < 0)
-        goto fail;
+    ff_progress_frame_ref(&dst->pf, &src->pf);
 
     ff_refstruct_replace(&dst->hwaccel_picture_private,
                           src->hwaccel_picture_private);
@@ -725,12 +721,6 @@  static int av1_frame_ref(AVCodecContext *avctx, AV1Frame *dst, const AV1Frame *s
            sizeof(dst->ref_frame_sign_bias));
     memcpy(dst->order_hints, src->order_hints,
            sizeof(dst->order_hints));
-
-    return 0;
-
-fail:
-    av1_frame_unref(dst);
-    return AVERROR(ENOMEM);
 }
 
 static av_cold int av1_decode_free(AVCodecContext *avctx)
@@ -738,16 +728,9 @@  static av_cold int av1_decode_free(AVCodecContext *avctx)
     AV1DecContext *s = avctx->priv_data;
     AV1RawMetadataITUTT35 itut_t35;
 
-    for (int i = 0; i < FF_ARRAY_ELEMS(s->ref); i++) {
-        if (s->ref[i].f) {
-            av1_frame_unref(&s->ref[i]);
-            av_frame_free(&s->ref[i].f);
-        }
-    }
-    if (s->cur_frame.f) {
-        av1_frame_unref(&s->cur_frame);
-        av_frame_free(&s->cur_frame.f);
-    }
+    for (int i = 0; i < FF_ARRAY_ELEMS(s->ref); i++)
+        av1_frame_unref(&s->ref[i]);
+    av1_frame_unref(&s->cur_frame);
     av_buffer_unref(&s->seq_data_ref);
     ff_refstruct_unref(&s->seq_ref);
     ff_refstruct_unref(&s->header_ref);
@@ -863,16 +846,6 @@  static av_cold int av1_decode_init(AVCodecContext *avctx)
     s->pkt = avctx->internal->in_pkt;
     s->pix_fmt = AV_PIX_FMT_NONE;
 
-    for (int i = 0; i < FF_ARRAY_ELEMS(s->ref); i++) {
-        s->ref[i].f = av_frame_alloc();
-        if (!s->ref[i].f)
-            return AVERROR(ENOMEM);
-    }
-
-    s->cur_frame.f = av_frame_alloc();
-    if (!s->cur_frame.f)
-        return AVERROR(ENOMEM);
-
     ret = ff_cbs_init(&s->cbc, AV_CODEC_ID_AV1, avctx);
     if (ret < 0)
         return ret;
@@ -934,7 +907,8 @@  static int av1_frame_alloc(AVCodecContext *avctx, AV1Frame *f)
         return ret;
     }
 
-    if ((ret = ff_thread_get_buffer(avctx, f->f, AV_GET_BUFFER_FLAG_REF)) < 0)
+    ret = ff_progress_frame_get_buffer(avctx, &f->pf, AV_GET_BUFFER_FLAG_REF);
+    if (ret < 0)
         goto fail;
 
     frame = f->f;
@@ -1211,23 +1185,17 @@  FF_ENABLE_DEPRECATION_WARNINGS
     return 0;
 }
 
-static int update_reference_list(AVCodecContext *avctx)
+static void update_reference_list(AVCodecContext *avctx)
 {
     AV1DecContext *s = avctx->priv_data;
     const AV1RawFrameHeader *header = s->raw_frame_header;
-    int ret;
 
     for (int i = 0; i < AV1_NUM_REF_FRAMES; i++) {
         if (header->refresh_frame_flags & (1 << i)) {
             av1_frame_unref(&s->ref[i]);
-            if ((ret = av1_frame_ref(avctx, &s->ref[i], &s->cur_frame)) < 0) {
-                av_log(avctx, AV_LOG_ERROR,
-                       "Failed to update frame %d in reference list\n", i);
-                return ret;
-            }
+            av1_frame_ref(avctx, &s->ref[i], &s->cur_frame);
         }
     }
-    return 0;
 }
 
 static int get_current_frame(AVCodecContext *avctx)
@@ -1358,20 +1326,12 @@  static int av1_receive_frame_internal(AVCodecContext *avctx, AVFrame *frame)
             if (s->raw_frame_header->show_existing_frame) {
                 av1_frame_unref(&s->cur_frame);
 
-                ret = av1_frame_ref(avctx, &s->cur_frame,
-                                    &s->ref[s->raw_frame_header->frame_to_show_map_idx]);
-                if (ret < 0) {
-                    av_log(avctx, AV_LOG_ERROR, "Failed to get reference frame.\n");
-                    goto end;
-                }
+                av1_frame_ref(avctx, &s->cur_frame,
+                              &s->ref[s->raw_frame_header->frame_to_show_map_idx]);
 
-                ret = update_reference_list(avctx);
-                if (ret < 0) {
-                    av_log(avctx, AV_LOG_ERROR, "Failed to update reference list.\n");
-                    goto end;
-                }
+                update_reference_list(avctx);
 
-                if (s->cur_frame.f->buf[0]) {
+                if (s->cur_frame.f) {
                     ret = set_output_frame(avctx, frame);
                     if (ret < 0)
                         av_log(avctx, AV_LOG_ERROR, "Set output frame error.\n");
@@ -1392,7 +1352,7 @@  static int av1_receive_frame_internal(AVCodecContext *avctx, AVFrame *frame)
             s->cur_frame.spatial_id  = header->spatial_id;
             s->cur_frame.temporal_id = header->temporal_id;
 
-            if (avctx->hwaccel && s->cur_frame.f->buf[0]) {
+            if (avctx->hwaccel && s->cur_frame.f) {
                 ret = FF_HW_CALL(avctx, start_frame, unit->data, unit->data_size);
                 if (ret < 0) {
                     av_log(avctx, AV_LOG_ERROR, "HW accel start frame fail.\n");
@@ -1418,7 +1378,7 @@  static int av1_receive_frame_internal(AVCodecContext *avctx, AVFrame *frame)
             if (ret < 0)
                 goto end;
 
-            if (avctx->hwaccel && s->cur_frame.f->buf[0]) {
+            if (avctx->hwaccel && s->cur_frame.f) {
                 ret = FF_HW_CALL(avctx, decode_slice, raw_tile_group->tile_data.data,
                                  raw_tile_group->tile_data.data_size);
                 if (ret < 0) {
@@ -1469,7 +1429,7 @@  static int av1_receive_frame_internal(AVCodecContext *avctx, AVFrame *frame)
 
         if (raw_tile_group && (s->tile_num == raw_tile_group->tg_end + 1)) {
             int show_frame = s->raw_frame_header->show_frame;
-            if (avctx->hwaccel && s->cur_frame.f->buf[0]) {
+            if (avctx->hwaccel && s->cur_frame.f) {
                 ret = FF_HW_SIMPLE_CALL(avctx, end_frame);
                 if (ret < 0) {
                     av_log(avctx, AV_LOG_ERROR, "HW accel end frame fail.\n");
@@ -1477,13 +1437,9 @@  static int av1_receive_frame_internal(AVCodecContext *avctx, AVFrame *frame)
                 }
             }
 
-            ret = update_reference_list(avctx);
-            if (ret < 0) {
-                av_log(avctx, AV_LOG_ERROR, "Failed to update reference list.\n");
-                goto end;
-            }
+            update_reference_list(avctx);
 
-            if (s->raw_frame_header->show_frame && s->cur_frame.f->buf[0]) {
+            if (s->raw_frame_header->show_frame && s->cur_frame.f) {
                 ret = set_output_frame(avctx, frame);
                 if (ret < 0) {
                     av_log(avctx, AV_LOG_ERROR, "Set output frame error\n");
@@ -1597,7 +1553,9 @@  const FFCodec ff_av1_decoder = {
     .close                 = av1_decode_free,
     FF_CODEC_RECEIVE_FRAME_CB(av1_receive_frame),
     .p.capabilities        = AV_CODEC_CAP_DR1,
-    .caps_internal         = FF_CODEC_CAP_INIT_CLEANUP | FF_CODEC_CAP_SKIP_FRAME_FILL_PARAM,
+    .caps_internal         = FF_CODEC_CAP_INIT_CLEANUP |
+                             FF_CODEC_CAP_SKIP_FRAME_FILL_PARAM |
+                             FF_CODEC_CAP_USES_PROGRESSFRAMES,
     .flush                 = av1_decode_flush,
     .p.profiles            = NULL_IF_CONFIG_SMALL(ff_av1_profiles),
     .p.priv_class          = &av1_class,
diff --git a/libavcodec/av1dec.h b/libavcodec/av1dec.h
index 79a0be510b..66a732d781 100644
--- a/libavcodec/av1dec.h
+++ b/libavcodec/av1dec.h
@@ -32,9 +32,15 @@ 
 #include "cbs.h"
 #include "cbs_av1.h"
 #include "dovi_rpu.h"
+#include "progressframe.h"
 
 typedef struct AV1Frame {
-    AVFrame *f;
+    union {
+        struct {
+            struct AVFrame *f;
+        };
+        ProgressFrame pf;
+    };
 
     void *hwaccel_picture_private; ///< RefStruct reference
 
diff --git a/libavcodec/dxva2_av1.c b/libavcodec/dxva2_av1.c
index 184a922fd8..5b95f99c9b 100644
--- a/libavcodec/dxva2_av1.c
+++ b/libavcodec/dxva2_av1.c
@@ -138,9 +138,9 @@  int ff_dxva2_av1_fill_picture_parameters(const AVCodecContext *avctx, AVDXVACont
         int8_t ref_idx = frame_header->ref_frame_idx[i];
         AVFrame *ref_frame = h->ref[ref_idx].f;
 
-        pp->frame_refs[i].width  = ref_frame->width;
-        pp->frame_refs[i].height = ref_frame->height;
-        pp->frame_refs[i].Index  = ref_frame->buf[0] ? ref_idx : 0xFF;
+        pp->frame_refs[i].width  = ref_frame ? ref_frame->width  : 0;
+        pp->frame_refs[i].height = ref_frame ? ref_frame->height : 0;
+        pp->frame_refs[i].Index  = ref_frame ? ref_idx : 0xFF;
 
         /* Global Motion */
         pp->frame_refs[i].wminvalid = h->cur_frame.gm_invalid[AV1_REF_FRAME_LAST + i];
@@ -151,7 +151,7 @@  int ff_dxva2_av1_fill_picture_parameters(const AVCodecContext *avctx, AVDXVACont
     }
     for (i = 0; i < AV1_NUM_REF_FRAMES; i++) {
         AVFrame *ref_frame = h->ref[i].f;
-        if (ref_frame->buf[0])
+        if (ref_frame)
             pp->RefFrameMapTextureIndex[i] = ff_dxva2_get_surface_index(avctx, ctx, ref_frame, 0);
     }
 
diff --git a/libavcodec/nvdec_av1.c b/libavcodec/nvdec_av1.c
index b0b013846e..4efa420e66 100644
--- a/libavcodec/nvdec_av1.c
+++ b/libavcodec/nvdec_av1.c
@@ -251,8 +251,8 @@  static int nvdec_av1_start_frame(AVCodecContext *avctx, const uint8_t *buffer, u
         AVFrame *ref_frame = s->ref[ref_idx].f;
 
         ppc->ref_frame[i].index = ppc->ref_frame_map[ref_idx];
-        ppc->ref_frame[i].width = ref_frame->width;
-        ppc->ref_frame[i].height = ref_frame->height;
+        ppc->ref_frame[i].width  = ref_frame ? ref_frame->width  : 0;
+        ppc->ref_frame[i].height = ref_frame ? ref_frame->height : 0;
 
         /* Global Motion */
         ppc->global_motion[i].invalid = !frame_header->is_global[AV1_REF_FRAME_LAST + i];
diff --git a/libavcodec/vaapi_av1.c b/libavcodec/vaapi_av1.c
index 1f9a6071ba..1f563483b9 100644
--- a/libavcodec/vaapi_av1.c
+++ b/libavcodec/vaapi_av1.c
@@ -46,7 +46,7 @@  typedef struct VAAPIAV1DecContext {
 
 static VASurfaceID vaapi_av1_surface_id(AV1Frame *vf)
 {
-    if (vf)
+    if (vf->f)
         return ff_vaapi_get_surface_id(vf->f);
     else
         return VA_INVALID_SURFACE;
@@ -132,7 +132,7 @@  static int vaapi_av1_start_frame(AVCodecContext *avctx,
             goto fail;
         pic->output_surface = ff_vaapi_get_surface_id(ctx->tmp_frame);
     } else {
-        pic->output_surface = vaapi_av1_surface_id(&s->cur_frame);
+        pic->output_surface = ff_vaapi_get_surface_id(s->cur_frame.f);
     }
 
     memset(&pic_param, 0, sizeof(VADecPictureParameterBufferAV1));
@@ -142,7 +142,7 @@  static int vaapi_av1_start_frame(AVCodecContext *avctx,
         .bit_depth_idx              = bit_depth_idx,
         .matrix_coefficients        = seq->color_config.matrix_coefficients,
         .current_frame              = pic->output_surface,
-        .current_display_picture    = vaapi_av1_surface_id(&s->cur_frame),
+        .current_display_picture    = ff_vaapi_get_surface_id(s->cur_frame.f),
         .frame_width_minus1         = frame_header->frame_width_minus_1,
         .frame_height_minus1        = frame_header->frame_height_minus_1,
         .primary_ref_frame          = frame_header->primary_ref_frame,
diff --git a/libavcodec/vdpau_av1.c b/libavcodec/vdpau_av1.c
index 80923092b9..b74ea6aa0c 100644
--- a/libavcodec/vdpau_av1.c
+++ b/libavcodec/vdpau_av1.c
@@ -219,7 +219,8 @@  static int vdpau_av1_start_frame(AVCodecContext *avctx,
         info->loop_filter_ref_deltas[i] = frame_header->loop_filter_ref_deltas[i];
 
         /* Reference Frames */
-        info->ref_frame_map[i] = ff_vdpau_get_surface_id(s->ref[i].f) ? ff_vdpau_get_surface_id(s->ref[i].f) : VDP_INVALID_HANDLE;
+        info->ref_frame_map[i] = s->ref[i].f && ff_vdpau_get_surface_id(s->ref[i].f) ?
+                                     ff_vdpau_get_surface_id(s->ref[i].f) : VDP_INVALID_HANDLE;
     }
 
     if (frame_header->primary_ref_frame == AV1_PRIMARY_REF_NONE) {
@@ -235,8 +236,8 @@  static int vdpau_av1_start_frame(AVCodecContext *avctx,
         AVFrame *ref_frame = s->ref[ref_idx].f;
 
         info->ref_frame[i].index = info->ref_frame_map[ref_idx];
-        info->ref_frame[i].width = ref_frame->width;
-        info->ref_frame[i].height = ref_frame->height;
+        info->ref_frame[i].width  = ref_frame ? ref_frame->width  : 0;
+        info->ref_frame[i].height = ref_frame ? ref_frame->height : 0;
 
         /* Global Motion */
         info->global_motion[i].invalid = !frame_header->is_global[AV1_REF_FRAME_LAST + i];
diff --git a/libavcodec/vulkan_av1.c b/libavcodec/vulkan_av1.c
index 49cd69d051..25ab4ecc70 100644
--- a/libavcodec/vulkan_av1.c
+++ b/libavcodec/vulkan_av1.c
@@ -284,7 +284,7 @@  static int vk_av1_start_frame(AVCodecContext          *avctx,
         AV1VulkanDecodePicture *hp = ref_frame->hwaccel_picture_private;
         int found = 0;
 
-        if (ref_frame->f->pict_type == AV_PICTURE_TYPE_NONE)
+        if (!ref_frame->f)
             continue;
 
         for (int j = 0; j < ref_count; j++) {
@@ -326,7 +326,7 @@  static int vk_av1_start_frame(AVCodecContext          *avctx,
         const AV1Frame *ref_frame = &s->ref[idx];
         AV1VulkanDecodePicture *hp = ref_frame->hwaccel_picture_private;
 
-        if (ref_frame->f->pict_type == AV_PICTURE_TYPE_NONE)
+        if (!ref_frame->f)
             ap->av1_pic_info.referenceNameSlotIndices[i] = AV1_REF_FRAME_NONE;
         else
             ap->av1_pic_info.referenceNameSlotIndices[i] = hp->frame_id;