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

[Mark Thompson <sw@jkqxz.net>]

On 01/05/2024 19:38, 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 optimizations 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, fixed naming and formats
> ---
>  libavutil/Makefile                 |   4 +
>  libavutil/hwcontext.c              |   4 +
>  libavutil/hwcontext.h              |   1 +
>  libavutil/hwcontext_amf.c          | 588 +++++++++++++++++++++++++++++
>  libavutil/hwcontext_amf.h          |  41 ++
>  libavutil/hwcontext_amf_internal.h |  77 ++++
>  libavutil/hwcontext_internal.h     |   1 +
>  libavutil/pixdesc.c                |   4 +
>  libavutil/pixfmt.h                 |   5 +
>  9 files changed, 725 insertions(+)
>  create mode 100644 libavutil/hwcontext_amf.c
>  create mode 100644 libavutil/hwcontext_amf.h
>  create mode 100644 libavutil/hwcontext_amf_internal.h
> 
> ... 
> diff --git a/libavutil/hwcontext_amf.c b/libavutil/hwcontext_amf.c
> new file mode 100644
> index 0000000000..8edfb20fbb
> --- /dev/null
> +++ b/libavutil/hwcontext_amf.c
> ...
> +
> +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;
> +}

This seems to have forgotten to actually allocate anything?  It will always assign NULL to frame->data[0] below.

> +
> +static int amf_frames_init(AVHWFramesContext *ctx)
> +{
> +    int i;
> +
> +    for (i = 0; i < FF_ARRAY_ELEMS(supported_formats); i++) {
> +        if (ctx->sw_format == supported_formats[i])
> +            break;
> +    }
> +    if (i == FF_ARRAY_ELEMS(supported_formats)) {
> +        av_log(ctx, AV_LOG_ERROR, "Pixel format '%s' is not supported\n",
> +               av_get_pix_fmt_name(ctx->sw_format));
> +        return AVERROR(ENOSYS);
> +    }
> +
> +    ffhwframesctx(ctx)->pool_internal =
> +        av_buffer_pool_init2(sizeof(AMFSurface), ctx,
> +                             amf_pool_alloc, NULL);
> +
> +
> +    return 0;
> +}
> +
> +static int amf_get_buffer(AVHWFramesContext *ctx, AVFrame *frame)
> +{
> +    frame->buf[0] = av_buffer_pool_get(ctx->pool);
> +    if (!frame->buf[0])
> +        return AVERROR(ENOMEM);
> +
> +    frame->data[0] = frame->buf[0]->data;
> +    frame->format  = AV_PIX_FMT_AMF_SURFACE;
> +    frame->width   = ctx->width;
> +    frame->height  = ctx->height;
> +    return 0;
> +}
> +
> ...
> +
> +static int amf_transfer_data_to(AVHWFramesContext *ctx, AVFrame *dst,
> +                                 const AVFrame *src)
> +{
> +    AMFSurface* surface = (AMFSurface*)dst->data[0];
> +    AMFPlane *plane;
> +    uint8_t  *dst_data[4];
> +    int       dst_linesize[4];
> +    int       planes;
> +    int       i;
> +    int w = FFMIN(dst->width,  src->width);
> +    int h = FFMIN(dst->height, src->height);
> +
> +    planes = (int)surface->pVtbl->GetPlanesCount(surface);
> +    av_assert0(planes < FF_ARRAY_ELEMS(dst_data));
> +
> +    for (i = 0; i < planes; i++) {
> +        plane = surface->pVtbl->GetPlaneAt(surface, i);
> +        dst_data[i] = plane->pVtbl->GetNative(plane);
> +        dst_linesize[i] = plane->pVtbl->GetHPitch(plane);
> +    }
> +    av_image_copy(dst_data, dst_linesize,
> +        (const uint8_t**)src->data, src->linesize, src->format,
> +        w, h);
> +
> +    return 0;
> +}
> +static int amf_transfer_data_from(AVHWFramesContext *ctx, AVFrame *dst,
> +                                    const AVFrame *src)
> +{
> +    AMFSurface* surface = (AMFSurface*)src->data[0];
> +    AMFPlane *plane;
> +    uint8_t  *src_data[4];
> +    int       src_linesize[4];
> +    int       planes;
> +    int       i;
> +    int w = FFMIN(dst->width,  src->width);
> +    int h = FFMIN(dst->height, src->height);
> +    int ret;
> +
> +    ret = surface->pVtbl->Convert(surface, AMF_MEMORY_HOST);
> +    AMF_RETURN_IF_FALSE(ctx, ret == AMF_OK, AVERROR_UNKNOWN, "Convert(amf::AMF_MEMORY_HOST) failed with error %d\n", AVERROR_UNKNOWN);
> +
> +    planes = (int)surface->pVtbl->GetPlanesCount(surface);
> +    av_assert0(planes < FF_ARRAY_ELEMS(src_data));
> +
> +    for (i = 0; i < planes; i++) {
> +        plane = surface->pVtbl->GetPlaneAt(surface, i);
> +        src_data[i] = plane->pVtbl->GetNative(plane);
> +        src_linesize[i] = plane->pVtbl->GetHPitch(plane);
> +    }
> +    av_image_copy(dst->data, dst->linesize,
> +                  (const uint8_t **)src_data, src_linesize, dst->format,
> +                  w, h);
> +    surface->pVtbl->Release(surface);
> +    return 0;
> +}

This makes it look like you really wanted to implement map_from, not transfer_data_from.

> ...> diff --git a/libavutil/hwcontext_amf.h b/libavutil/hwcontext_amf.h
> new file mode 100644
> index 0000000000..36f13890a0
> --- /dev/null
> +++ b/libavutil/hwcontext_amf.h
> @@ -0,0 +1,41 @@
> +/*
> + * 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 "libavformat/avformat.h"

libavutil cannot depend on libavformat, that would be circular.

> +#include "libavutil/hwcontext.h"
> +
> +typedef struct AVAMFDeviceContextInternal AVAMFDeviceContextInternal;
> +
> +/**
> + * This struct is allocated as AVHWDeviceContext.hwctx
> + */
> +
> +typedef struct AVAMFDeviceContext {
> +    AVBufferRef        *internal;

So what does a user of this hwcontext set this pointer to?

> +} AVAMFDeviceContext;

And what should the user be returning for a user-created pool of frames?

> +
> +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);
> +
> +#endif /* AVUTIL_HWCONTEXT_AMF_H */
> diff --git a/libavutil/hwcontext_amf_internal.h b/libavutil/hwcontext_amf_internal.h
> new file mode 100644
> index 0000000000..023dc8b4bb
> --- /dev/null
> +++ b/libavutil/hwcontext_amf_internal.h
> @@ -0,0 +1,77 @@
> +/*
> + * 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_INTERNAL_H
> +#define AVUTIL_HWCONTEXT_AMF_INTERNAL_H
> +#include <AMF/core/Factory.h>
> +#include <AMF/core/Context.h>
> +#include <AMF/core/Trace.h>
> +
> +typedef struct AVAMFDeviceContextInternal {
> +    amf_handle          library; ///< handle to DLL library
> +    AMFFactory         *factory; ///< pointer to AMF factory
> +    AMFDebug           *debug;   ///< pointer to AMF debug interface
> +    AMFTrace           *trace;   ///< pointer to AMF trace interface
> +
> +    amf_uint64          version; ///< version of AMF runtime
> +    AMFContext         *context; ///< AMF context
> +    AMF_MEMORY_TYPE     mem_type;
> +} AVAMFDeviceContextInternal;

Some of these details look like they should be in the public hwcontext so that a user can create one.  Maybe just the AMFContext?  I'm not sure of the details.

> +
> +typedef struct AmfTraceWriter {
> +    AMFTraceWriterVtbl  *vtbl;
> +    void                *avctx;
> +    void                *avcl;
> +} AmfTraceWriter;
> +
> +extern AmfTraceWriter av_amf_trace_writer;

Mutable variables are not allowed in the libraries, external mutable variables doubly so.

> +
> +int av_amf_context_init(AVAMFDeviceContextInternal* internal, void* avcl);
> +int av_amf_create_context(  AVAMFDeviceContextInternal * internal,
> +                                void* avcl,
> +                                const char *device,
> +                                AVDictionary *opts, int flags);
> +int av_amf_context_internal_create(AVAMFDeviceContextInternal * internal,
> +                                void* avcl,
> +                                const char *device,
> +                                AVDictionary *opts, int flags);
> +void av_amf_context_internal_free(void *opaque, uint8_t *data);
> +int av_amf_context_derive(AVAMFDeviceContextInternal * internal,
> +                              AVHWDeviceContext *child_device_ctx, AVDictionary *opts,
> +                              int flags);

No.  Moving your public functions to an "internal" header is not helpful.

All of these appear to be exposing internals of the hwcontext functions, so should be implemented through them.

> +
> +
> +/**
> +* Error handling helper
> +*/
> +#define AMF_RETURN_IF_FALSE(avctx, exp, ret_value, /*message,*/ ...) \
> +    if (!(exp)) { \
> +        av_log(avctx, AV_LOG_ERROR, __VA_ARGS__); \
> +        return ret_value; \
> +    }
> +
> +#define AMF_GOTO_FAIL_IF_FALSE(avctx, exp, ret_value, /*message,*/ ...) \
> +    if (!(exp)) { \
> +        av_log(avctx, AV_LOG_ERROR, __VA_ARGS__); \
> +        ret = ret_value; \
> +        goto fail; \
> +    }
> +
> +#define AMF_TIME_BASE_Q          (AVRational){1, AMF_SECOND}
> +#endif /* AVUTIL_HWCONTEXT_AMF_INTERNAL_H */
> \ No newline at end of file

Free review comment from your git client.

> ...

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".