[FFmpeg-devel] [PATCH 01/10, v3] avutil: add hwcontext_amf.

[Mark Thompson]

On 30/05/2024 14:08, Dmitrii Ovchinnikov wrote:
> Adds hwcontext_amf, which allows to use shared AMF
> context for the encoder, decoder and AMF-based filters,
> without copy to the host memory.
> It will also allow you to use some optimisations in
> the interaction of components (for example, SAV) and make a more
> manageable and optimal setup for using GPU devices with AMF
> in the case of a fully AMF pipeline.
> It will be a significant performance uplift when full AMF pipeline
> with filters is used.
> 
> We also plan to add Compression artefact removal filter in near feature.
> v2: cleanup header files
> v3: an unnecessary class has been removed.
> ---
>  libavutil/Makefile                 |   4 +
>  libavutil/hwcontext.c              |   4 +
>  libavutil/hwcontext.h              |   1 +
>  libavutil/hwcontext_amf.c          | 585 +++++++++++++++++++++++++++++
>  libavutil/hwcontext_amf.h          |  64 ++++
>  libavutil/hwcontext_amf_internal.h |  44 +++
>  libavutil/hwcontext_internal.h     |   1 +
>  libavutil/pixdesc.c                |   4 +
>  libavutil/pixfmt.h                 |   5 +
>  9 files changed, 712 insertions(+)
>  create mode 100644 libavutil/hwcontext_amf.c
>  create mode 100644 libavutil/hwcontext_amf.h
>  create mode 100644 libavutil/hwcontext_amf_internal.h
> 
> ...
> +
> +static void amf_dummy_free(void *opaque, uint8_t *data)
> +{
> +
> +}
> +
> +static AVBufferRef *amf_pool_alloc(void *opaque, size_t size)
> +{
> +    AVHWFramesContext *hwfc = (AVHWFramesContext *)opaque;
> +    AVBufferRef *buf;
> +
> +    buf = av_buffer_create(NULL, NULL, amf_dummy_free, hwfc, AV_BUFFER_FLAG_READONLY);
> +    if (!buf) {
> +        av_log(hwfc, AV_LOG_ERROR, "Failed to create buffer for AMF context.\n");
> +        return NULL;
> +    }
> +    return buf;
> +}

You're still allocating nothing here?

I think what this means is that you don't actually want to implement frames context creation at all because it doesn't do anything.

If it is not possible to make an AMFSurface as anything other than an output from an AMF component then this would make sense, the decoder can allocate the internals.

Look at the DRM hwcontext for an example that works like this - the DRM objects can only be made as outputs from devices or by mapping, so there is no frame context implementation.

> +
> ...
> diff --git a/libavutil/hwcontext_amf.h b/libavutil/hwcontext_amf.h
> new file mode 100644
> index 0000000000..ef2118dd4e
> --- /dev/null
> +++ b/libavutil/hwcontext_amf.h
> @@ -0,0 +1,64 @@
> +/*
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +
> +#ifndef AVUTIL_HWCONTEXT_AMF_H
> +#define AVUTIL_HWCONTEXT_AMF_H
> +
> +#include "pixfmt.h"
> +#include "hwcontext.h"
> +#include <AMF/core/Factory.h>
> +#include <AMF/core/Context.h>
> +#include <AMF/core/Trace.h>
> +#include <AMF/core/Debug.h>
> +
> +/**
> + * This struct is allocated as AVHWDeviceContext.hwctx
> + */
> +typedef struct AVAMFDeviceContext {
> +    HMODULE            library;

What is this type?  (It looks like a Windows type, but I thought this was cross-platform.)

> +    AMFFactory         *factory;
> +    AMFDebug           *debug;
> +    AMFTrace           *trace;
> +    void               *trace_writer;

Are all of these objects necessary to operation of the AMF device?  Please remove elements which are not necessary and add them to the private context if they are otherwise useful.

> +
> +    int64_t            version; ///< version of AMF runtime

Why is the version necessary to expose in the public API?  Is it not possible to call the QueryVersion function after starting?

> +    AMFContext         *context;
> +    int                mem_type;
Is mem_type really necessary to expose in the piblic API?  Can the user not determine this by some API call?

> +} AVAMFDeviceContext;
> +
> +enum AMF_SURFACE_FORMAT av_amf_av_to_amf_format(enum AVPixelFormat fmt);
> +enum AVPixelFormat av_amf_to_av_format(enum AMF_SURFACE_FORMAT fmt);
> +

All of the following functions should not be public symbols.  You want to implement the hwcontext functions so that these all work without needing special implementation for AMF, they should not be individually callable because that is not useful.

> +int av_amf_context_create(AVAMFDeviceContext * context,
> +                          void* avcl,
> +                          const char *device,
> +                          AVDictionary *opts, int flags);

Use device_create.

> +int av_amf_context_init(AVAMFDeviceContext* internal, void* avcl);

Use device_init.

> +void av_amf_context_free(void *opaque, uint8_t *data);

Use device_uninit (or maybe an AVBuffer destructor?).

> +int av_amf_context_derive(AVAMFDeviceContext * internal,
> +                          AVHWDeviceContext *child_device_ctx, AVDictionary *opts,
> +                          int flags);

Use device_derive.

> +
> +int av_amf_transfer_data_from(AVHWFramesContext *ctx, AVFrame *dst,
> +                                    const AVFrame *src);

Use transfer_data_from.

> +
> +int av_amf_transfer_data_to(AVHWFramesContext *ctx, AVFrame *dst,
> +                                 const AVFrame *src);

Use transfer_data_to.

> +
> +#endif /* AVUTIL_HWCONTEXT_AMF_H */
> ...

Thanks,

- Mark