[FFmpeg-devel] [PATCH] lavc/amfenc: Retain a reference to D3D frames used as input during the encoding process
Alexander Kravchenko
akravchenko188 at gmail.com
Sun Apr 8 22:13:19 EEST 2018
> -----Original Message-----
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf
> Of Mark Thompson
> Sent: Sunday, April 8, 2018 8:25 PM
> To: ffmpeg-devel at ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH] lavc/amfenc: Retain a reference to D3D
> frames used as input during the encoding process
>
> On 06/04/18 11:25, Alexander Kravchenko wrote:
> >>
> >> This breaks the testcase described in
> >> https://trac.ffmpeg.org/ticket/6990 which is basically the same as
> >> the one you described in this patch.
> >>
> >> I get the following spammed repeatedly:
> >>
> >> [AVHWFramesContext @ 000000000502d340] Static surface pool size
> exceeded.
> >> [mpeg2video @ 0000000002f8d400] get_buffer() failed [mpeg2video @
> >> 0000000002f8d400] thread_get_buffer() failed [mpeg2video @
> >> 0000000002f8d400] get_buffer() failed (-12 0000000000000000) Error
> >> while decoding stream #0:1: Operation not permitted
> >
> > Hi, could you please review the following updated patch I added queue
> > limit according initial pool size of incoming frame context.
> > This solves the problem running this pipeline without -extra_hw_frames 16
> option, but -extra_hw_frames option can be used to have bigger queue for
> encoder.
>
> I think you've misunderstood /why/ the decoder has the pool size allocation
> that it does. The decoder expects to use all of the surfaces it has allocated in
> the worst case - the difference between MPEG-2 and H.264 is that MPEG-2
> can store at most two reference frames (the forward and backward
> references for B-frames), while H.264 can store up to sixteen. Most H.264
> streams don't use all sixteen references, but in theory they could (excepting
> level restrictions, but they are generally quite iffy) so the decoder allocates
> space for all of those references even if they aren't used.
>
> I can believe that this patch happens to work if you have a simple stream
> with limited references (streams rarely use more than two or three), but it
> will certainly fail exactly as before for complex streams.
>
> If you want to hold onto more than one frame in the encoder then currently
> you need to use the -extra_hw_frames option on the source (whether
> decoder or filter) - that is exactly what it's there for. Some sort of automatic
> negotiation is suggested (there was some discussion on libav-devel a while
> ago), but the requirement that it works through libavfilter is a difficult one
> with the current structure so nothing concrete is yet proposed. (That was
> mostly considering libmfx, where it's even more of a problem because the
> lookahead options can make the encoder queue over a hundred frames
> internally.)
>
> > ---
> > libavcodec/amfenc.c | 97
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++-
> > libavcodec/amfenc.h | 3 ++
> > 2 files changed, 99 insertions(+), 1 deletion(-)
> >
> > diff --git a/libavcodec/amfenc.c b/libavcodec/amfenc.c index
> > 89a10ff253..eb7b20c252 100644
> > --- a/libavcodec/amfenc.c
> > +++ b/libavcodec/amfenc.c
> > @@ -157,6 +157,9 @@ static int amf_init_context(AVCodecContext *avctx)
> > AmfContext *ctx = avctx->priv_data;
> > AMF_RESULT res = AMF_OK;
> >
> > + ctx->hwsurfaces_in_queue = 0;
> > + ctx->hwsurfaces_in_queue_max = 16;
> > +
> > // configure AMF logger
> > // the return of these functions indicates old state and do not affect
> behaviour
> > ctx->trace->pVtbl->EnableWriter(ctx->trace,
> > AMF_TRACE_WRITER_DEBUG_OUTPUT, ctx->log_to_dbg != 0 ); @@ -187,6
> +190,7 @@ static int amf_init_context(AVCodecContext *avctx)
> > if (!ctx->hw_frames_ctx) {
> > return AVERROR(ENOMEM);
> > }
> > + ctx->hwsurfaces_in_queue_max =
> > + device_ctx->initial_pool_size - 1;
> > } else {
> > if(res == AMF_NOT_SUPPORTED)
> > av_log(avctx, AV_LOG_INFO,
> > "avctx->hw_frames_ctx has D3D11 device which doesn't have D3D11VA
> interface, switching to default\n"); @@ -443,6 +447,73 @@ int
> ff_amf_encode_init(AVCodecContext *avctx)
> > return ret;
> > }
> >
> > +#define AMF_AV_QUERY_INTERFACE(res, from, InterfaceTypeTo, to) \
> > + { \
> > + AMFGuid guid_##InterfaceTypeTo = IID_##InterfaceTypeTo(); \
> > + res = from->pVtbl->QueryInterface(from, &guid_##InterfaceTypeTo,
> (void**)&to); \
> > + }
> > +
> > +#define AMF_AV_ASSIGN_PROPERTY_INTERFACE(res, pThis, name, val) \
> > + { \
> > + AMFInterface *amf_interface; \
> > + AMFVariantStruct var; \
> > + res = AMFVariantInit(&var); \
> > + if (res == AMF_OK) { \
> > + AMF_AV_QUERY_INTERFACE(res, val, AMFInterface,
> amf_interface)\
> > + if (res == AMF_OK) { \
> > + res = AMFVariantAssignInterface(&var, amf_interface); \
> > + amf_interface->pVtbl->Release(amf_interface); \
> > + } \
> > + if (res == AMF_OK) { \
> > + res = pThis->pVtbl->SetProperty(pThis, name, var); \
> > + } \
> > + AMFVariantClear(&var); \
> > + }\
> > + }
> > +
> > +#define AMF_AV_GET_PROPERTY_INTERFACE(res, pThis, name,
> TargetType, val) \
> > + { \
> > + AMFVariantStruct var; \
> > + res = AMFVariantInit(&var); \
> > + if (res == AMF_OK) { \
> > + res = pThis->pVtbl->GetProperty(pThis, name, &var); \
> > + if (res == AMF_OK) { \
> > + if (var.type == AMF_VARIANT_INTERFACE &&
> AMFVariantInterface(&var)) { \
> > + AMF_AV_QUERY_INTERFACE(res, AMFVariantInterface(&var),
> TargetType, val); \
> > + } else { \
> > + res = AMF_INVALID_DATA_TYPE; \
> > + } \
> > + } \
> > + AMFVariantClear(&var); \
> > + }\
> > + }
>
> These macros are only expanded once each and with a single type. Do you
> intend to use them again in future patches with different types? If not, it
> might be easier not to have them at all, or turn them into functions.
>
> > +
> > +static AMFBuffer* amf_create_buffer_with_frame_ref(const AVFrame*
> > +frame, AMFContext *context) {
> > + AVFrame *frame_ref;
> > + AMFBuffer *frame_ref_storage_buffer = NULL;
> > + AMF_RESULT res;
> > +
> > + res = context->pVtbl->AllocBuffer(context, AMF_MEMORY_HOST,
> sizeof(frame_ref), &frame_ref_storage_buffer);
> > + if (res == AMF_OK) {
> > + frame_ref = av_frame_clone(frame);
> > + if (frame_ref) {
> > + memcpy(frame_ref_storage_buffer->pVtbl-
> >GetNative(frame_ref_storage_buffer), &frame_ref, sizeof(frame_ref));
> > + } else {
> > + frame_ref_storage_buffer->pVtbl-
> >Release(frame_ref_storage_buffer);
> > + frame_ref_storage_buffer = NULL;
> > + }
> > + }
> > + return frame_ref_storage_buffer;
> > +}
> > +
> > +static void amf_release_buffer_with_frame_ref(AMFBuffer
> > +*frame_ref_storage_buffer) {
> > + AVFrame *av_frame_ref;
> > + memcpy(&av_frame_ref, frame_ref_storage_buffer->pVtbl-
> >GetNative(frame_ref_storage_buffer), sizeof(av_frame_ref));
> > + av_frame_free(&av_frame_ref);
> > +
> > +frame_ref_storage_buffer->pVtbl->Release(frame_ref_storage_buffer);
> > +}
> >
> > int ff_amf_send_frame(AVCodecContext *avctx, const AVFrame *frame)
> {
> > @@ -484,6 +555,7 @@ int ff_amf_send_frame(AVCodecContext *avctx,
> const AVFrame *frame)
> > (ctx->hw_device_ctx && ((AVHWFramesContext*)frame-
> >hw_frames_ctx->data)->device_ctx ==
> > (AVHWDeviceContext*)ctx->hw_device_ctx->data)
> > )) {
> > + AMFBuffer *frame_ref_storage_buffer;
> > #if CONFIG_D3D11VA
> > static const GUID AMFTextureArrayIndexGUID = { 0x28115527,
> 0xe7c3, 0x4b66, { 0x99, 0xd3, 0x4f, 0x2a, 0xe6, 0xb4, 0x7f, 0xaf } };
> > ID3D11Texture2D *texture =
> > (ID3D11Texture2D*)frame->data[0]; // actual texture @@ -496,6 +568,17
> @@ int ff_amf_send_frame(AVCodecContext *avctx, const AVFrame
> *frame)
> > // input HW surfaces can be vertically aligned by 16; tell AMF the real
> size
> > surface->pVtbl->SetCrop(surface, 0, 0, frame->width,
> > frame->height); #endif
> > +
> > + frame_ref_storage_buffer =
> amf_create_buffer_with_frame_ref(frame, ctx->context);
> > + AMF_RETURN_IF_FALSE(ctx, frame_ref_storage_buffer !=
> > + NULL, AVERROR(ENOMEM), "create_buffer_with_frame_ref() returned
> > + NULL\n");
> > +
> > + AMF_AV_ASSIGN_PROPERTY_INTERFACE(res, surface,
> L"av_frame_ref", frame_ref_storage_buffer);
> > + if (res != AMF_OK) {
> > + surface->pVtbl->Release(surface);
> > + }
> > + ctx->hwsurfaces_in_queue++;
> > + frame_ref_storage_buffer->pVtbl-
> >Release(frame_ref_storage_buffer);
> > + AMF_RETURN_IF_FALSE(ctx, res == AMF_OK,
> AVERROR(ENOMEM),
> > + "SetProperty failed for \"av_frame_ref\" with error %d\n", res);
> > } else {
> > res = ctx->context->pVtbl->AllocSurface(ctx->context,
> AMF_MEMORY_HOST, ctx->format, avctx->width, avctx->height, &surface);
> > AMF_RETURN_IF_FALSE(ctx, res == AMF_OK, AVERROR(ENOMEM),
> > "AllocSurface() failed with error %d\n", res); @@ -560,6 +643,18 @@ int
> ff_amf_receive_packet(AVCodecContext *avctx, AVPacket *avpkt)
> > ret = amf_copy_buffer(avctx, avpkt, buffer);
> >
> > buffer->pVtbl->Release(buffer);
> > +
> > + if (data->pVtbl->HasProperty(data, L"av_frame_ref")) {
> > + AMFBuffer *frame_ref_storage_buffer;
> > + AMF_AV_GET_PROPERTY_INTERFACE(res, data,
> L"av_frame_ref", AMFBuffer, frame_ref_storage_buffer);
> > + if (res == AMF_OK) {
> > +
> amf_release_buffer_with_frame_ref(frame_ref_storage_buffer);
> > + ctx->hwsurfaces_in_queue--;
> > + } else {
> > + av_log(avctx, AV_LOG_WARNING, "GetProperty failed
> > + for \"av_frame_ref\" with error %d\n", res);
>
> Can you get this warning in normal use? It seems like it should be fatal, since
> a frame reference has been completely lost.
>
> > + }
> > + }
> > +
> > data->pVtbl->Release(data);
> >
> > AMF_RETURN_IF_FALSE(ctx, ret >= 0, ret,
> > "amf_copy_buffer() failed with error %d\n", ret); @@ -589,7 +684,7 @@
> int ff_amf_receive_packet(AVCodecContext *avctx, AVPacket *avpkt)
> > av_log(avctx, AV_LOG_WARNING, "Data acquired but delayed
> drain submission got AMF_INPUT_FULL- should not happen\n");
> > }
> > }
> > - } else if (ctx->delayed_surface != NULL || ctx->delayed_drain || (ctx-
> >eof && res_query != AMF_EOF)) {
> > + } else if (ctx->delayed_surface != NULL || ctx->delayed_drain
> > + || (ctx->eof && res_query != AMF_EOF) || (ctx->hwsurfaces_in_queue
> > + >= ctx->hwsurfaces_in_queue_max)) {
> > block_and_wait = 1;
> > av_usleep(1000); // wait and poll again
> > }
> > diff --git a/libavcodec/amfenc.h b/libavcodec/amfenc.h index
> > 84f0aad2fa..b1361842bd 100644
> > --- a/libavcodec/amfenc.h
> > +++ b/libavcodec/amfenc.h
> > @@ -62,6 +62,9 @@ typedef struct AmfContext {
> > AVBufferRef *hw_device_ctx; ///< pointer to HW accelerator
> (decoder)
> > AVBufferRef *hw_frames_ctx; ///< pointer to HW accelerator (frame
> allocator)
> >
> > + int hwsurfaces_in_queue;
> > + int hwsurfaces_in_queue_max;
> > +
> > // helpers to handle async calls
> > int delayed_drain;
> > AMFSurface *delayed_surface;
> >
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Hi Mark, thanks for your feedback
I just tried to propose solution which solves the problem in many cases (as short term solution to have this patch simple)
What could you recommend for this fix (patch)?
1) Check if "extra_hw_frames" value is set and enough for encoding, then use incoming frames as input for decoder, otherwise use copy routine
2) Check if "extra_hw_frames" value is set and enough, otherwise show error
3) Try to find decoder/encoder negotiation solution
4) ?
> These macros are only expanded once each and with a single type. Do you
> intend to use them again in future patches with different types? If not, it
> might be easier not to have them at all, or turn them into functions.
I will change them to functions, no problem. In near future macroses like this will appear in AMF itself, so I will simplify this later in separate patch.
> Can you get this warning in normal use? It seems like it should be fatal, since
> a frame reference has been completely lost.
Agree, will be fixed
More information about the ffmpeg-devel
mailing list