From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id 5EC294B3AE for ; Tue, 4 Jun 2024 19:25:11 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id E9A5768D6E4; Tue, 4 Jun 2024 22:25:08 +0300 (EEST) Received: from mail-wm1-f48.google.com (mail-wm1-f48.google.com [209.85.128.48]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 71EEC68D6B0 for ; Tue, 4 Jun 2024 22:25:03 +0300 (EEST) Received: by mail-wm1-f48.google.com with SMTP id 5b1f17b1804b1-4212b4eea2eso42502315e9.2 for ; Tue, 04 Jun 2024 12:25:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=jkqxz-net.20230601.gappssmtp.com; s=20230601; t=1717529103; x=1718133903; darn=ffmpeg.org; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=whdEnjLf2I2PMU0+HaAP06ecrcJSwWkVCAfpRWuP/7E=; b=yC6rD1Sqll+XEc5BBjHBmTnt5d3feyAOSztWL38g3H4tU6PtP6xjJJ7nxX99vpJAZr l4P164faZUXf67zSwt+vC3dcnXxfJ8IQ1pH71xs712EJAUjWrNglPMZsFlHMtBQU50EL OukuGX4fzl415ubswthwP/NOTRkiMTQKdP0e21nwxe+dFaVoJl55hpimvVNQEmFGPToN 7rFvpNzy2XAkIzozsKWwwfagewc/CPxYX7IJjflIHSaxlaWD3GTGvTCLSTQR4gAAS1ue 1X85fZDDz13pn6bnneLVPeNAJhYMTASS0Ht9Hcf2xn13thvQaDBmdr5iQCV4wHAjB9V6 mGQg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1717529103; x=1718133903; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=whdEnjLf2I2PMU0+HaAP06ecrcJSwWkVCAfpRWuP/7E=; b=hIDBekDeUB5DYAtOhacoVnRyujcTcvvEFlZtazMD+n/vJ81ag6XccZIGaUCoeDego9 j0Aa6dzQsNYyjZmxeX9k+ff15WjIHZbey7LGHPQkKLZZ6eDy9MUQ/ygTSNLY8rWTlSQa IQ1xUJ3zM0Wm6St6n8xarYLKgYwc1WadJeVmUzsNFXCXS4fu8PVgL92FFB5jHn/cnpnC AZ6UJiSSLCnr48R+cngI2KsOrcx6OfbDryjdOqE1gQm42En939qqAU2aPWlBI39WE+aU gzWY7xzPP8O2oy6OWvrhPhnaFDP6Tq+vmoKQv/W0fNE2R0HPna5kGbYdta5EBMwzJGtR CoIw== X-Gm-Message-State: AOJu0YzCQU0ylD2bnmTThdRFoenSrzF/ItnSEGW6exRIVriIOlYT0gBd EIliUH/gZlVpeL+RHH5rZ6qbdNDPssJf2OCyDNucHC0TOpDuNSq58307dPIIgemoMZ7UoWBdyIX b X-Google-Smtp-Source: AGHT+IHvZ+A8VCCOynxi9Xx7cIxMihbw9CpzqnCnmzDeHBT1Bb4lVFieqYn43oijqQ9Sm7o7alU1aQ== X-Received: by 2002:a05:600c:4754:b0:416:9ba0:8f17 with SMTP id 5b1f17b1804b1-421562f21b8mr4268315e9.22.1717529102514; Tue, 04 Jun 2024 12:25:02 -0700 (PDT) Received: from [192.168.0.15] (cpc92302-cmbg19-2-0-cust1183.5-4.cable.virginm.net. [82.1.212.160]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-35e5047f75esm9492751f8f.35.2024.06.04.12.25.02 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 04 Jun 2024 12:25:02 -0700 (PDT) Message-ID: <211fab8d-21de-451e-8c41-5a39c112a84e@jkqxz.net> Date: Tue, 4 Jun 2024 20:25:36 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: ffmpeg-devel@ffmpeg.org References: <20240530130826.374-1-ovchinnikov.dmitrii@gmail.com> <20240530130826.374-2-ovchinnikov.dmitrii@gmail.com> From: Mark Thompson In-Reply-To: <20240530130826.374-2-ovchinnikov.dmitrii@gmail.com> Subject: Re: [FFmpeg-devel] [PATCH 02/10, v3] avcodec: add amfdec. X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: On 30/05/2024 14:08, Dmitrii Ovchinnikov wrote: > From: Evgeny Pavlov > > Added AMF based h264, hevc, av1 decoders. > Co-authored-by: Dmitrii Ovchinnikov > v2: added encoder reinitialisation > v3: use AMF_SURFACE_UNKNOWN to int decoder(ctx->output_format before) > --- > libavcodec/Makefile | 7 +- > libavcodec/allcodecs.c | 3 + > libavcodec/amfdec.c | 696 +++++++++++++++++++++++++++++++++++++++++ > libavcodec/amfdec.h | 63 ++++ > 4 files changed, 767 insertions(+), 2 deletions(-) > create mode 100644 libavcodec/amfdec.c > create mode 100644 libavcodec/amfdec.h > > ... > + > +const enum AVPixelFormat amf_dec_pix_fmts[] = { > + AV_PIX_FMT_YUV420P, > + AV_PIX_FMT_NV12, > + AV_PIX_FMT_BGRA, > + AV_PIX_FMT_ARGB, > + AV_PIX_FMT_RGBA, > + AV_PIX_FMT_GRAY8, > + AV_PIX_FMT_BGR0, > + AV_PIX_FMT_YUYV422, > + AV_PIX_FMT_P010, > + AV_PIX_FMT_P012, > + AV_PIX_FMT_YUV420P10, > + AV_PIX_FMT_YUV420P12, > + AV_PIX_FMT_YUV420P16, > +#if CONFIG_D3D11VA > + AV_PIX_FMT_D3D11, > +#endif > +#if CONFIG_DXVA2 > + AV_PIX_FMT_DXVA2_VLD, > +#endif > + AV_PIX_FMT_AMF_SURFACE, > + AV_PIX_FMT_NONE > +}; What is this set of formats doing? Most of them are ignored becase get_format below only ever offers two choices. > + > +static const AVCodecHWConfigInternal *const amf_hw_configs[] = { > + &(const AVCodecHWConfigInternal) { > + .public = { > + .pix_fmt = AV_PIX_FMT_AMF_SURFACE, > + .methods = AV_CODEC_HW_CONFIG_METHOD_HW_FRAMES_CTX | See below, I don't think it makes sense to have HW_FRAMES_CTX in this decoder. > + AV_CODEC_HW_CONFIG_METHOD_HW_DEVICE_CTX, > + .device_type = AV_HWDEVICE_TYPE_AMF, > + }, > + .hwaccel = NULL, > + }, > + NULL > +}; > + > ... > + > +static int amf_init_decoder(AVCodecContext *avctx) > +{ > + AMFDecoderContext *ctx = avctx->priv_data; > + AVAMFDeviceContext * internal = ctx->amf_device_ctx; > + const wchar_t *codec_id = NULL; > + AMF_RESULT res; > + AMFBuffer *buffer; > + amf_int64 color_profile; > + int pool_size = 36; > + > + ctx->drain = 0; > + ctx->resolution_changed = 0; > + > + switch (avctx->codec->id) { > + case AV_CODEC_ID_H264: > + codec_id = AMFVideoDecoderUVD_H264_AVC; > + break; > + case AV_CODEC_ID_HEVC: { > + if (avctx->profile == AV_PROFILE_HEVC_MAIN_10) You won't know profile here? It is an output field, the decoder has to set it once it determines it from the stream. > + codec_id = AMFVideoDecoderHW_H265_MAIN10; > + else > + codec_id = AMFVideoDecoderHW_H265_HEVC; > + } break; > + case AV_CODEC_ID_AV1: > + if (avctx->profile == AV_PROFILE_AV1_PROFESSIONAL) > + codec_id = AMFVideoDecoderHW_AV1_12BIT; > + else > + codec_id = AMFVideoDecoderHW_AV1; > + break; > + default: > + break; > + } > + AMF_RETURN_IF_FALSE(ctx, codec_id != NULL, AVERROR(EINVAL), "Codec %d is not supported\n", avctx->codec->id); > + > + ...> + > +static int amf_decode_init(AVCodecContext *avctx) > +{ > + AMFDecoderContext *ctx = avctx->priv_data; > + int ret; > + ctx->local_context = 0; > + ctx->in_pkt = av_packet_alloc(); > + if (!ctx->in_pkt) > + return AVERROR(ENOMEM); > + > + if (avctx->hw_frames_ctx){ This will never be set at init time because the user sets it in the get_format callback (see documentation for the field). Even ignoring that, I don't see how this would make sense ayway? The AMF frames context is a dummy shell containing nothing, so the AV_CODEC_HW_CONFIG_METHOD_HW_FRAMES_CTX can't do anything useful. (How are you testing this path?) > + AVHWFramesContext *frames_ctx = (AVHWFramesContext*)avctx->hw_frames_ctx->data; > + if (frames_ctx->device_ctx->type == AV_HWDEVICE_TYPE_AMF) { > + ctx->amf_device_ctx = frames_ctx->device_ctx->hwctx; > + } > + } > + else if (avctx->hw_device_ctx && !avctx->hw_frames_ctx) { > + AVHWDeviceContext *hwdev_ctx; > + AVHWFramesContext *hwframes_ctx; > + hwdev_ctx = (AVHWDeviceContext*)avctx->hw_device_ctx->data; > + if (hwdev_ctx->type == AV_HWDEVICE_TYPE_AMF) > + { > + ctx->amf_device_ctx = hwdev_ctx->hwctx; > + } > + > + avctx->hw_frames_ctx = av_hwframe_ctx_alloc(avctx->hw_device_ctx); > + > + if (!avctx->hw_frames_ctx) { > + av_log(avctx, AV_LOG_ERROR, "av_hwframe_ctx_alloc failed\n"); > + return AVERROR(ENOMEM); > + } > + > + hwframes_ctx = (AVHWFramesContext*)avctx->hw_frames_ctx->data; > + hwframes_ctx->width = FFALIGN(avctx->coded_width, 32); > + hwframes_ctx->height = FFALIGN(avctx->coded_height, 32); I don't see how you can ensure that you have a correct value for the sizes here? (See documentation for the fields; sometimes they are set from codecpar to help the user and I would guess that you are only testing with that in the ffmpeg utility rather than using the decoder standalone.) > + hwframes_ctx->format = AV_PIX_FMT_AMF_SURFACE; > + hwframes_ctx->sw_format = avctx->sw_pix_fmt == AV_PIX_FMT_YUV420P10 ? AV_PIX_FMT_P010 : AV_PIX_FMT_NV12; I don't see where sw_pix_fmt would have come from either. > + hwframes_ctx->initial_pool_size = ctx->surface_pool_size + 8; > + avctx->pix_fmt = AV_PIX_FMT_AMF_SURFACE; > + > + ret = av_hwframe_ctx_init(avctx->hw_frames_ctx); > + > + if (ret < 0) { > + av_log(NULL, AV_LOG_ERROR, "Error initializing a AMF frame pool\n"); > + av_buffer_unref(&avctx->hw_frames_ctx); > + return ret; > + } > + } else { > + ctx->amf_device_ctx = av_mallocz(sizeof(AVAMFDeviceContext)); sizeof(AVAMFDeviceContext) is not allowed in a different library. I think this is trying to do what making a device would do except without calling the normal functions. Just make a device normally rather than having special functions to bypass that? > + ctx->local_context = 1; > + if ((ret = av_amf_context_create(ctx->amf_device_ctx, avctx, "", NULL, 0)) != 0) { > + amf_decode_close(avctx); > + return ret; > + } > + if ((ret = amf_init_decoder_context(avctx)) != 0) { > + return ret; > + } > + } > + if ((ret = amf_init_decoder(avctx)) == 0) { > + AMFVariantStruct format_var = {0}; > + ret = ctx->decoder->pVtbl->GetProperty(ctx->decoder, AMF_VIDEO_DECODER_OUTPUT_FORMAT, &format_var); > + if (ret != AMF_OK) { > + return AVERROR(EINVAL); > + } > + enum AVPixelFormat format = av_amf_to_av_format((AMF_SURFACE_FORMAT)format_var.int64Value); > + enum AVPixelFormat pix_fmts[3] = { > + AV_PIX_FMT_AMF_SURFACE, > + format, > + AV_PIX_FMT_NONE }; > + > + > + ret = ff_get_format(avctx, pix_fmts); > + if (ret < 0) { > + avctx->pix_fmt = AV_PIX_FMT_NONE; > + } > + > + return 0; > + } > + amf_decode_close(avctx); > + return ret; > +} > + > ...> diff --git a/libavcodec/amfdec.h b/libavcodec/amfdec.h For a single-file implementation with no external functions there is no need to make a header. Thanks, - Mark _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".