On 30/05/2024 14:08, Dmitrii Ovchinnikov wrote:
> From: Evgeny Pavlov <lucenti...@gmail.com>
>
> Added AMF based h264, hevc, av1 decoders.
> Co-authored-by: Dmitrii Ovchinnikov <ovchinnikov.dmit...@gmail.com>
> 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-requ...@ffmpeg.org with subject "unsubscribe".
