| Message ID | 002401d3c4ee$fada2fe0$f08e8fa0$@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, Mar 26, 2018 at 01:41:20PM +0300, Alexander Kravchenko wrote: > Hello, > I have fixed issues listed in previous patch. > > > > Say what the change is in the title. Something like "amfenc: Retain a reference to D3D11 frames used as input during the encoding > > process", maybe? > Sure, but I am preparing next patch adding DX9 support, so probably better to write D3D instead D3D11 > > > > > How many frames can end up queued inside the encoder here? > 16 > > > > > Is there always an exact 1->1 correspondence between input frames and output packets? That is, is it guaranteed that no frames are > > ever dropped, even in the low-latency modes? > yes > > > Put the * in the right place - it's part of the declarator, not the declaration-specifiers. > > "if (", and in all places below too. > I have fixed these issues in whole file (Hopefully you don’t mind if it put to same commit. There aren't many pf them) > > > From: Alexander Kravchenko <akravchenko188@gmail.com> > --- > libavcodec/amfenc.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 81 insertions(+), 8 deletions(-) > > diff --git a/libavcodec/amfenc.c b/libavcodec/amfenc.c > index 89a10ff253..f532a32b7b 100644 > --- a/libavcodec/amfenc.c > +++ b/libavcodec/amfenc.c > @@ -188,7 +188,7 @@ static int amf_init_context(AVCodecContext *avctx) > return AVERROR(ENOMEM); > } > } else { > - if(res == AMF_NOT_SUPPORTED) > + 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"); > else > av_log(avctx, AV_LOG_INFO, "avctx->hw_frames_ctx has non-AMD device, switching to default\n"); > @@ -298,7 +298,7 @@ int av_cold ff_amf_encode_close(AVCodecContext *avctx) > } > > static int amf_copy_surface(AVCodecContext *avctx, const AVFrame *frame, > - AMFSurface* surface) > + AMFSurface *surface) > { > AVFrame *sw_frame = NULL; > AMFPlane *plane = NULL; These changes are unrelated and should not be in this patch [...]
On 3/26/2018 7:41 AM, Alexander Kravchenko wrote: > Hello, > I have fixed issues listed in previous patch. > > >> Say what the change is in the title. Something like "amfenc: Retain a reference to D3D11 frames used as input during the encoding >> process", maybe? > Sure, but I am preparing next patch adding DX9 support, so probably better to write D3D instead D3D11 > >> >> How many frames can end up queued inside the encoder here? > 16 > >> >> Is there always an exact 1->1 correspondence between input frames and output packets? That is, is it guaranteed that no frames are >> ever dropped, even in the low-latency modes? > yes > >> Put the * in the right place - it's part of the declarator, not the declaration-specifiers. >> "if (", and in all places below too. > I have fixed these issues in whole file (Hopefully you don’t mind if it put to same commit. There aren't many pf them) Usually no, cosmetic changes on existing code go in their own separate commits. Also, if this is fixing the issue described in https://trac.ffmpeg.org/ticket/6990 then please add a "Fixes ticket #6990" to the commit message.
On 26/03/18 11:41, Alexander Kravchenko wrote: Put email comments on a patch below the "---" line (otherwise they end up in the commit message when applied). > Hello, > I have fixed issues listed in previous patch. > > >> Say what the change is in the title. Something like "amfenc: Retain a reference to D3D11 frames used as input during the encoding >> process", maybe? > Sure, but I am preparing next patch adding DX9 support, so probably better to write D3D instead D3D11 Well, it currently means D3D11 only, unless you are posting a D3D9 patch ahead of it? Doesn't really matter, though. >> >> How many frames can end up queued inside the encoder here? > 16 So a transcode from a file with a lot of references will fall over without extra_hw_frames being set on the decoder? That probably wants more documentation somewhere, but it shouldn't block this patch. >> >> Is there always an exact 1->1 correspondence between input frames and output packets? That is, is it guaranteed that no frames are >> ever dropped, even in the low-latency modes? > yes Ok, good. >> Put the * in the right place - it's part of the declarator, not the declaration-specifiers. >> "if (", and in all places below too. > I have fixed these issues in whole file (Hopefully you don’t mind if it put to same commit. There aren't many pf them) I was meaning only in the code you are adding. The others should be fixed, but it should be separate patch with no functional content (saying something like "fix cosmetic issues"). > > From: Alexander Kravchenko <akravchenko188@gmail.com> > --- > libavcodec/amfenc.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 81 insertions(+), 8 deletions(-) > > diff --git a/libavcodec/amfenc.c b/libavcodec/amfenc.c > index 89a10ff253..f532a32b7b 100644 > --- a/libavcodec/amfenc.c > +++ b/libavcodec/amfenc.c > @@ -188,7 +188,7 @@ static int amf_init_context(AVCodecContext *avctx) > return AVERROR(ENOMEM); > } > } else { > - if(res == AMF_NOT_SUPPORTED) > + 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"); > else > av_log(avctx, AV_LOG_INFO, "avctx->hw_frames_ctx has non-AMD device, switching to default\n"); > @@ -298,7 +298,7 @@ int av_cold ff_amf_encode_close(AVCodecContext *avctx) > } > > static int amf_copy_surface(AVCodecContext *avctx, const AVFrame *frame, > - AMFSurface* surface) > + AMFSurface *surface) > { > AVFrame *sw_frame = NULL; > AMFPlane *plane = NULL; > @@ -371,7 +371,7 @@ static int amf_copy_buffer(AVCodecContext *avctx, AVPacket *pkt, AMFBuffer *buff > switch (avctx->codec->id) { > case AV_CODEC_ID_H264: > buffer->pVtbl->GetProperty(buffer, AMF_VIDEO_ENCODER_OUTPUT_DATA_TYPE, &var); > - if(var.int64Value == AMF_VIDEO_ENCODER_OUTPUT_DATA_TYPE_IDR) { > + if (var.int64Value == AMF_VIDEO_ENCODER_OUTPUT_DATA_TYPE_IDR) { > pkt->flags = AV_PKT_FLAG_KEY; > } > break; > @@ -443,6 +443,48 @@ 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) \ > + return res; \ > + 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); \ > + } \ > + res = AMFVariantClear(&var); \ > + } > + > +#define AMF_AV_GET_PROPERTY_INTERFACE(res, pThis, name, TargetType, val) \ > + { \ > + AMFVariantStruct var; \ > + res = AMFVariantInit(&var); \ > + if (res != AMF_OK) \ > + return res; \ > + 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 look even more like they should be functions rather than macros now? In particular, the returns in them interact badly with the code below. > > int ff_amf_send_frame(AVCodecContext *avctx, const AVFrame *frame) > { > @@ -458,7 +500,7 @@ int ff_amf_send_frame(AVCodecContext *avctx, const AVFrame *frame) > if (!ctx->eof) { // submit drain one time only > if (ctx->delayed_surface != NULL) { > ctx->delayed_drain = 1; // input queue is full: resubmit Drain() in ff_amf_receive_packet > - } else if(!ctx->delayed_drain) { > + } else if (!ctx->delayed_drain) { > res = ctx->encoder->pVtbl->Drain(ctx->encoder); > if (res == AMF_INPUT_FULL) { > ctx->delayed_drain = 1; // input queue is full: resubmit Drain() in ff_amf_receive_packet > @@ -484,6 +526,8 @@ 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) > )) { > + AVFrame *frame_ref; > + 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 +540,20 @@ 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 > + res = ctx->context->pVtbl->AllocBuffer(ctx->context, AMF_MEMORY_HOST, sizeof(frame_ref), &frame_ref_storage_buffer); > + AMF_RETURN_IF_FALSE(ctx, res == AMF_OK, AVERROR(ENOMEM), "AllocBuffer() failed with error %d\n", res); > + frame_ref = av_frame_clone(frame); > + AMF_RETURN_IF_FALSE(ctx, frame_ref != NULL, AVERROR(ENOMEM), "av_frame_clone() returned NULL\n"); > + memcpy(frame_ref_storage_buffer->pVtbl->GetNative(frame_ref_storage_buffer), &frame_ref, sizeof(frame_ref)); > + AMF_AV_ASSIGN_PROPERTY_INTERFACE(res, surface, L"av_frame_ref", frame_ref_storage_buffer); > + if (res != AMF_OK) > + { { on the previous line. > + av_log(avctx, AV_LOG_WARNING, "failed to attach av_frame_ref to surface\n"); And keep going anyway, corrupting the output? > + av_frame_free(&frame_ref); > + surface->pVtbl->Release(surface); > + } > + frame_ref_storage_buffer->pVtbl->Release(frame_ref_storage_buffer); > + AMF_RETURN_IF_FALSE(ctx, res == AMF_OK, AVERROR(ENOMEM), "SetProperty failed for \"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); > @@ -554,12 +612,27 @@ int ff_amf_receive_packet(AVCodecContext *avctx, AVPacket *avpkt) > res_query = ctx->encoder->pVtbl->QueryOutput(ctx->encoder, &data); > if (data) { > // copy data to packet > - AMFBuffer* buffer; > - AMFGuid guid = IID_AMFBuffer(); > - data->pVtbl->QueryInterface(data, &guid, (void**)&buffer); // query for buffer interface > + AMFBuffer *buffer; > + AMF_AV_QUERY_INTERFACE(res, data, AMFBuffer, buffer); > + AMF_RETURN_IF_FALSE(ctx, res == AMF_OK, AVERROR_UNKNOWN, "Invalid data type from encoder->QueryOutput, should be AMFBuffer, error %d\n", res); > ret = amf_copy_buffer(avctx, avpkt, buffer); > - > buffer->pVtbl->Release(buffer); > + > + //try to get attached av_frame_ref and unref > + if (data->pVtbl->HasProperty(data, L"av_frame_ref")) { > + AMFBuffer *frame_ref_storage_buffer = NULL; > + AVFrame *av_frame_ref; > + > + AMF_AV_GET_PROPERTY_INTERFACE(res, data, L"av_frame_ref", AMFBuffer, frame_ref_storage_buffer); > + if (res == AMF_OK) { > + 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); > + } else { > + av_log(avctx, AV_LOG_WARNING, "av_frame_ref data attached to frame is corrupted\n"); > + } > + } > + > data->pVtbl->Release(data); > > AMF_RETURN_IF_FALSE(ctx, ret >= 0, ret, "amf_copy_buffer() failed with error %d\n", ret); > Thanks, - Mark
> > Put email comments on a patch below the "---" line (otherwise they end up in the commit message when applied). > Sorry, I am not sure I understand you correctly here > > +#define AMF_AV_GET_PROPERTY_INTERFACE(res, pThis, name, TargetType, val) \ > > + { \ > > + AMFVariantStruct var; \ > > + res = AMFVariantInit(&var); \ > > + if (res != AMF_OK) \ > > + return res; \ > > + 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 look even more like they should be functions rather than macros now? In particular, the returns in them interact badly with the > code below. I'd also prefer function here, but it works with different types (TargetType parameter is type name) I am going to discuss propose such macros to AMF headers near future. > > + if (res != AMF_OK) > > + { > > { on the previous line. > > > + av_log(avctx, AV_LOG_WARNING, "failed to attach av_frame_ref to surface\n"); > > And keep going anyway, corrupting the output? > Ok, agree, I will return error here Thanks, Alexander ________________________________ This e-mail and any attachment(s) are intended only for the recipient(s) named above and others who have been specifically authorized to receive them. They may contain confidential information. If you are not the intended recipient, please do not read this email or its attachment(s). Furthermore, you are hereby notified that any dissemination, distribution or copying of this e-mail and any attachment(s) is strictly prohibited. If you have received this e-mail in error, please immediately notify the sender by replying to this e-mail and then delete this e-mail and any attachment(s) or copies thereof from your system. Thank you.
Hello, Mark, Did you have a chance to review the latest version patch? Thanks, Alexander
diff --git a/libavcodec/amfenc.c b/libavcodec/amfenc.c index 89a10ff253..f532a32b7b 100644 --- a/libavcodec/amfenc.c +++ b/libavcodec/amfenc.c @@ -188,7 +188,7 @@ static int amf_init_context(AVCodecContext *avctx) return AVERROR(ENOMEM); } } else { - if(res == AMF_NOT_SUPPORTED) + 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"); else av_log(avctx, AV_LOG_INFO, "avctx->hw_frames_ctx has non-AMD device, switching to default\n"); @@ -298,7 +298,7 @@ int av_cold ff_amf_encode_close(AVCodecContext *avctx) } static int amf_copy_surface(AVCodecContext *avctx, const AVFrame *frame, - AMFSurface* surface) + AMFSurface *surface) { AVFrame *sw_frame = NULL; AMFPlane *plane = NULL; @@ -371,7 +371,7 @@ static int amf_copy_buffer(AVCodecContext *avctx, AVPacket *pkt, AMFBuffer *buff switch (avctx->codec->id) { case AV_CODEC_ID_H264: buffer->pVtbl->GetProperty(buffer, AMF_VIDEO_ENCODER_OUTPUT_DATA_TYPE, &var); - if(var.int64Value == AMF_VIDEO_ENCODER_OUTPUT_DATA_TYPE_IDR) { + if (var.int64Value == AMF_VIDEO_ENCODER_OUTPUT_DATA_TYPE_IDR) { pkt->flags = AV_PKT_FLAG_KEY; } break; @@ -443,6 +443,48 @@ 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) \ + return res; \ + 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); \ + } \ + res = AMFVariantClear(&var); \ + } + +#define AMF_AV_GET_PROPERTY_INTERFACE(res, pThis, name, TargetType, val) \ + { \ + AMFVariantStruct var; \ + res = AMFVariantInit(&var); \ + if (res != AMF_OK) \ + return res; \ + 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); \ + } int ff_amf_send_frame(AVCodecContext *avctx, const AVFrame *frame) { @@ -458,7 +500,7 @@ int ff_amf_send_frame(AVCodecContext *avctx, const AVFrame *frame) if (!ctx->eof) { // submit drain one time only if (ctx->delayed_surface != NULL) { ctx->delayed_drain = 1; // input queue is full: resubmit Drain() in ff_amf_receive_packet - } else if(!ctx->delayed_drain) { + } else if (!ctx->delayed_drain) { res = ctx->encoder->pVtbl->Drain(ctx->encoder); if (res == AMF_INPUT_FULL) { ctx->delayed_drain = 1; // input queue is full: resubmit Drain() in ff_amf_receive_packet @@ -484,6 +526,8 @@ 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) )) { + AVFrame *frame_ref; + 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 +540,20 @@ 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 + res = ctx->context->pVtbl->AllocBuffer(ctx->context, AMF_MEMORY_HOST, sizeof(frame_ref), &frame_ref_storage_buffer); + AMF_RETURN_IF_FALSE(ctx, res == AMF_OK, AVERROR(ENOMEM), "AllocBuffer() failed with error %d\n", res); + frame_ref = av_frame_clone(frame); + AMF_RETURN_IF_FALSE(ctx, frame_ref != NULL, AVERROR(ENOMEM), "av_frame_clone() returned NULL\n"); + memcpy(frame_ref_storage_buffer->pVtbl->GetNative(frame_ref_storage_buffer), &frame_ref, sizeof(frame_ref)); + AMF_AV_ASSIGN_PROPERTY_INTERFACE(res, surface, L"av_frame_ref", frame_ref_storage_buffer); + if (res != AMF_OK) + { + av_log(avctx, AV_LOG_WARNING, "failed to attach av_frame_ref to surface\n"); + av_frame_free(&frame_ref); + surface->pVtbl->Release(surface); + } + frame_ref_storage_buffer->pVtbl->Release(frame_ref_storage_buffer); + AMF_RETURN_IF_FALSE(ctx, res == AMF_OK, AVERROR(ENOMEM), "SetProperty failed for \"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); @@ -554,12 +612,27 @@ int ff_amf_receive_packet(AVCodecContext *avctx, AVPacket *avpkt) res_query = ctx->encoder->pVtbl->QueryOutput(ctx->encoder, &data); if (data) { // copy data to packet - AMFBuffer* buffer; - AMFGuid guid = IID_AMFBuffer(); - data->pVtbl->QueryInterface(data, &guid, (void**)&buffer); // query for buffer interface + AMFBuffer *buffer; + AMF_AV_QUERY_INTERFACE(res, data, AMFBuffer, buffer); + AMF_RETURN_IF_FALSE(ctx, res == AMF_OK, AVERROR_UNKNOWN, "Invalid data type from encoder->QueryOutput, should be AMFBuffer, error %d\n", res); ret = amf_copy_buffer(avctx, avpkt, buffer); - buffer->pVtbl->Release(buffer); + + //try to get attached av_frame_ref and unref + if (data->pVtbl->HasProperty(data, L"av_frame_ref")) { + AMFBuffer *frame_ref_storage_buffer = NULL; + AVFrame *av_frame_ref; + + AMF_AV_GET_PROPERTY_INTERFACE(res, data, L"av_frame_ref", AMFBuffer, frame_ref_storage_buffer); + if (res == AMF_OK) { + 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); + } else { + av_log(avctx, AV_LOG_WARNING, "av_frame_ref data attached to frame is corrupted\n"); + } + } + data->pVtbl->Release(data); AMF_RETURN_IF_FALSE(ctx, ret >= 0, ret, "amf_copy_buffer() failed with error %d\n", ret);