PR #25660 opened by JuliusBairaktaris URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/25660 Patch URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/25660.patch
For hardware input, the AMF encoders attach a clone of each frame to its surface and count the surface as queued; both are undone only when the matching output arrives. On error paths and at an early close they were never undone: - a failed HDR metadata property left the metadata buffer, the surface, the clone and the count; - a failed `SubmitInput()`, first or resubmitted, left the clone and the count; - a failed output-list write left the surface and the output buffer; - frames still inside the encoder at close kept their clones; - a surface whose clone could not be attached was still counted. This keeps the attached clones in a list, counts a surface only once its clone is attached, unwinds both on every failure, and frees the remaining clones at close. Found while following up the Forgejo_Fairy review of #25458. Tested on Windows 11, RX 9070 XT, driver 32.0.32015.7029, with injected failures: a failed HDR property on d3d11va HDR10 input left one clone and one queued surface (none now); a failed `SubmitInput()` on d3d11va or AMF surface input left two (now one is unwound immediately and the frame still in the encoder is freed at close). h264_amf, hevc_amf and av1_amf give bit-identical output for software, d3d11va and AMF surface input, as does hevc_amf for HDR10 input. Conflicts textually in `amf_submit_frame()` with my #25458 and #25488 and with #24055 and #24696; I'll rebase this or my PRs, whichever lands later. Written with Claude Opus 5.5 in Claude Code; `Assisted-by` trailer on the commit. >From ad3c7ef3c5bce54700ef52b1c0666520152e6a54 Mon Sep 17 00:00:00 2001 From: Julius Bairaktaris <[email protected]> Date: Sun, 1 Nov 2026 17:49:17 +0100 Subject: [PATCH] avcodec/amfenc: release input surfaces and attached frames on errors For hardware input, amf_submit_frame() attaches a clone of the frame to the surface as an integer property and counts the surface in hwsurfaces_in_queue; both are undone only when the matching output arrives. On several paths that never happens: - When setting the HDR metadata property fails, the function returns without releasing the metadata buffer, the surface, the clone or the count. - When SubmitInput() fails, the surface is released, but the clone and the count are not; the same applies to a failed resubmission in ff_amf_receive_packet(), which also leaks the surface and the output buffer when the output list cannot be grown. - When the encoder is closed with frames still inside it, Terminate() drops their surfaces, and the clones attached to them are never freed. - When attaching the clone fails, the surface is still counted, but no output will ever decrement the count. Keep the attached clones in a list, count a surface only when its clone is attached, undo both on every failure after attaching, and free the remaining clones at close. Reported by the Forgejo_Fairy review of #25458. Tested on Windows 11, RX 9070 XT, driver 32.0.32015.7029, by injecting failures: after a failed HDR metadata property on d3d11va HDR10 input, one clone and one queued surface were left at close (none now); after a failed SubmitInput() on d3d11va or AMF surface input, two (one is now undone on the error path and the one still inside the encoder is freed at close). h264_amf, hevc_amf and av1_amf give bit-identical output for software, d3d11va and AMF surface input, as does hevc_amf for HDR10 input. Assisted-by: Claude Opus 5.5 Signed-off-by: Julius Bairaktaris <[email protected]> --- libavcodec/amfenc.c | 73 ++++++++++++++++++++++++++++++++++++++------- libavcodec/amfenc.h | 3 ++ 2 files changed, 66 insertions(+), 10 deletions(-) diff --git a/libavcodec/amfenc.c b/libavcodec/amfenc.c index 20bc22d98c5..964f75a4f51 100644 --- a/libavcodec/amfenc.c +++ b/libavcodec/amfenc.c @@ -147,6 +147,12 @@ int av_cold ff_amf_encode_close(AVCodecContext *avctx) ctx->encoder = NULL; } + for (int i = 0; i < ctx->nb_attached_frames; i++) + av_frame_free(&ctx->attached_frames[i]); + av_freep(&ctx->attached_frames); + ctx->nb_attached_frames = 0; + ctx->hwsurfaces_in_queue = 0; + av_buffer_unref(&ctx->device_ctx_ref); av_fifo_freep2(&ctx->timestamp_list); @@ -369,26 +375,53 @@ static AMF_RESULT amf_store_attached_frame_ref(AMFEncoderContext *ctx, const AVF { AMF_RESULT res = AMF_FAIL; int64_t data; - AVFrame *frame_ref = av_frame_clone(frame); + AVFrame **attached; + AVFrame *frame_ref; + + attached = av_fast_realloc(ctx->attached_frames, &ctx->attached_frames_size, + (ctx->nb_attached_frames + 1) * sizeof(*attached)); + if (!attached) + return AMF_OUT_OF_MEMORY; + ctx->attached_frames = attached; + + frame_ref = av_frame_clone(frame); if (frame_ref) { memcpy(&data, &frame_ref, sizeof(frame_ref)); // store pointer in 8 bytes AMF_ASSIGN_PROPERTY_INT64(res, surface, ctx->av_frame_property_name, data); + if (res == AMF_OK) + ctx->attached_frames[ctx->nb_attached_frames++] = frame_ref; + else + av_frame_free(&frame_ref); } return res; } -static AMF_RESULT amf_release_attached_frame_ref(AMFEncoderContext *ctx, AMFBuffer *buffer) +static AMF_RESULT amf_release_attached_frame_ref(AMFEncoderContext *ctx, AMFData *data) { AMFVariantStruct var = {0}; - AMF_RESULT res = buffer->pVtbl->GetProperty(buffer, ctx->av_frame_property_name, &var); + AMF_RESULT res = data->pVtbl->GetProperty(data, ctx->av_frame_property_name, &var); if(res == AMF_OK && var.int64Value){ AVFrame *frame_ref; + int i; memcpy(&frame_ref, &var.int64Value, sizeof(frame_ref)); + for (i = 0; i < ctx->nb_attached_frames; i++) + if (ctx->attached_frames[i] == frame_ref) + break; + if (i == ctx->nb_attached_frames) + return AMF_NOT_FOUND; + ctx->attached_frames[i] = ctx->attached_frames[--ctx->nb_attached_frames]; av_frame_free(&frame_ref); } return res; } +static void amf_discard_input_surface(AMFEncoderContext *ctx, AMFSurface *surface) +{ + if (amf_release_attached_frame_ref(ctx, (AMFData *)surface) == AMF_OK) + ctx->hwsurfaces_in_queue--; + surface->pVtbl->Release(surface); +} + static int amf_submit_frame(AVCodecContext *avctx, AVFrame *frame, AMFSurface **surface_resubmit) { AMFEncoderContext *ctx = avctx->priv_data; @@ -443,8 +476,8 @@ static int amf_submit_frame(AVCodecContext *avctx, AVFrame *frame, AMFSurface break; } if (hw_surface) { - amf_store_attached_frame_ref(ctx, frame, surface); - ctx->hwsurfaces_in_queue++; + if (amf_store_attached_frame_ref(ctx, frame, surface) == AMF_OK) + ctx->hwsurfaces_in_queue++; // input HW surfaces can be vertically aligned by 16; tell AMF the real size surface->pVtbl->SetCrop(surface, 0, 0, frame->width, frame->height); } @@ -464,7 +497,12 @@ static int amf_submit_frame(AVCodecContext *avctx, AVFrame *frame, AMFSurface AMF_ASSIGN_PROPERTY_INTERFACE(res, ctx->encoder, AMF_VIDEO_ENCODER_AV1_INPUT_HDR_METADATA, hdrmeta_buffer); break; } res = amf_set_property_buffer(surface, L"av_frame_hdrmeta", hdrmeta_buffer); - AMF_RETURN_IF_FALSE(ctx, res == AMF_OK, AVERROR_UNKNOWN, "SetProperty failed for \"av_frame_hdrmeta\" with error %d\n", res); + if (res != AMF_OK) { + av_log(ctx, AV_LOG_ERROR, "SetProperty failed for \"av_frame_hdrmeta\" with error %d\n", res); + hdrmeta_buffer->pVtbl->Release(hdrmeta_buffer); + ret = AVERROR_UNKNOWN; + goto fail; + } } hdrmeta_buffer->pVtbl->Release(hdrmeta_buffer); } @@ -538,8 +576,12 @@ static int amf_submit_frame(AVCodecContext *avctx, AVFrame *frame, AMFSurface //store surface for later submission *surface_resubmit = surface; } else { + if (res != AMF_OK) { + av_log(ctx, AV_LOG_ERROR, "SubmitInput() failed with error %d\n", res); + ret = AVERROR_UNKNOWN; + goto fail; + } surface->pVtbl->Release(surface); - AMF_RETURN_IF_FALSE(ctx, res == AMF_OK, AVERROR_UNKNOWN, "SubmitInput() failed with error %d\n", res); ctx->submitted_frame++; ret = av_fifo_write(ctx->timestamp_list, &frame->pts, 1); if (ret < 0) @@ -548,6 +590,10 @@ static int amf_submit_frame(AVCodecContext *avctx, AVFrame *frame, AMFSurface return AVERROR(EAGAIN); // too soon to poll or wait } return 0; + +fail: + amf_discard_input_surface(ctx, surface); + return ret; } static int amf_submit_frame_locked(AVCodecContext *avctx, AVFrame *frame, AMFSurface **surface_resubmit) @@ -575,7 +621,7 @@ static AMF_RESULT amf_query_output(AVCodecContext *avctx, AMFBuffer **buffer) AMFGuid guid = IID_AMFBuffer(); data->pVtbl->QueryInterface(data, &guid, (void**)buffer); // query for buffer interface data->pVtbl->Release(data); - if (amf_release_attached_frame_ref(ctx, *buffer) == AMF_OK) + if (amf_release_attached_frame_ref(ctx, (AMFData *)*buffer) == AMF_OK) ctx->hwsurfaces_in_queue--; ctx->encoded_frame++; } @@ -693,16 +739,23 @@ int ff_amf_receive_packet(AVCodecContext *avctx, AVPacket *avpkt) amf_query_output(avctx, &buffer); if (buffer != NULL) { ret = av_fifo_write(ctx->output_list, &buffer, 1); - if (ret < 0) + if (ret < 0) { + buffer->pVtbl->Release(buffer); + amf_discard_input_surface(ctx, surface); return ret; + } } } while(res == AMF_INPUT_FULL); + if (res != AMF_OK && res != AMF_INPUT_FULL) { + amf_discard_input_surface(ctx, surface); + av_log(avctx, AV_LOG_ERROR, "SubmitInput() failed with error %d\n", res); + return AVERROR_UNKNOWN; + } surface->pVtbl->Release(surface); if (res == AMF_INPUT_FULL) { av_log(avctx, AV_LOG_WARNING, "Data acquired but delayed SubmitInput returned AMF_INPUT_FULL- should not happen\n"); } else { - AMF_RETURN_IF_FALSE(ctx, res == AMF_OK, AVERROR_UNKNOWN, "SubmitInput() failed with error %d\n", res); ret = av_fifo_write(ctx->timestamp_list, &pts, 1); diff --git a/libavcodec/amfenc.h b/libavcodec/amfenc.h index 6a380208d14..25b9e8a51c2 100644 --- a/libavcodec/amfenc.h +++ b/libavcodec/amfenc.h @@ -50,6 +50,9 @@ typedef struct AMFEncoderContext { wchar_t *av_frame_property_name; int hwsurfaces_in_queue; + AVFrame **attached_frames; + unsigned int attached_frames_size; + int nb_attached_frames; int hwsurfaces_in_queue_max; int query_timeout_supported; -- 2.54.0 _______________________________________________ ffmpeg-devel mailing list -- [email protected] To unsubscribe send an email to [email protected]
