Re: [FFmpeg-devel] [PATCH 02/10, v3] avcodec: add amfdec.

[Mark Thompson <sw@jkqxz.net>]

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