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 <akravchenko...@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
