Re: [FFmpeg-devel] [PATCH 1/9] libavutil: add hwcontext_amf.

[James Almer <jamrial@gmail.com>]

On 2/13/2024 10:55 PM, Dmitrii Ovchinnikov wrote:
> +const FormatMap format_map[] =
> +{
> +    { AV_PIX_FMT_NONE,          AMF_SURFACE_UNKNOWN },
> +    { AV_PIX_FMT_NV12,          AMF_SURFACE_NV12 },
> +    { AV_PIX_FMT_BGR0,          AMF_SURFACE_BGRA },
> +    { AV_PIX_FMT_RGB0,          AMF_SURFACE_RGBA },
> +    { AV_PIX_FMT_BGRA,          AMF_SURFACE_BGRA },
> +    { AV_PIX_FMT_ARGB,          AMF_SURFACE_ARGB },
> +    { AV_PIX_FMT_RGBA,          AMF_SURFACE_RGBA },
> +    { AV_PIX_FMT_GRAY8,         AMF_SURFACE_GRAY8 },
> +    { AV_PIX_FMT_YUV420P,       AMF_SURFACE_YUV420P },
> +    { AV_PIX_FMT_YUYV422,       AMF_SURFACE_YUY2 },
> +    { AV_PIX_FMT_P010,          AMF_SURFACE_P010 },
> +    { AV_PIX_FMT_YUV420P10,     AMF_SURFACE_P010 },

yuv420p10 is not equal to p010.

> +    { AV_PIX_FMT_YUV420P12,     AMF_SURFACE_P012 },
> +    { AV_PIX_FMT_YUV420P12,     AMF_SURFACE_P012 },

Why the duplication? And there's AV_PIX_FMT_P012.

> +    { AV_PIX_FMT_YUV420P16,     AMF_SURFACE_P016 },

AV_PIX_FMT_P016?

> +    { AV_PIX_FMT_YUV422P10LE,   AMF_SURFACE_Y210 },

AV_PIX_FMT_Y210?

> +    { AV_PIX_FMT_YUV444P10LE,   AMF_SURFACE_Y416 },
> +};

[...]

> diff --git a/libavutil/hwcontext_amf.h b/libavutil/hwcontext_amf.h
> new file mode 100644
> index 0000000000..0161b9a29c
> --- /dev/null
> +++ b/libavutil/hwcontext_amf.h
> @@ -0,0 +1,105 @@
> +/*
> + * 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 <AMF/core/Factory.h>
> +#include <AMF/core/Context.h>
> +#include <AMF/core/Trace.h>
> +#include "pixfmt.h"
> +
> +#include "libavformat/avformat.h"

lavu can't depend on lavf.

> +#include "libavutil/hwcontext.h"
> +
> +#define FFMPEG_AMF_WRITER_ID L"ffmpeg_amf"

Bad namespace. And this doesn't need to be public.

> +
> +typedef struct AmfTraceWriter {

Ditto namespace. Does this need to be public?

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

If this is internal, then define it in an internal header and put only 
the typedef here.

> +    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;
> +
> +/**
> + * This struct is allocated as AVHWDeviceContext.hwctx
> + */
> +
> +typedef struct AVAMFDeviceContext {
> +    AVBufferRef        *internal;

AVAMFDeviceContextInternal *internal. Can't say if this is needed here 
at all or not.

> +} AVAMFDeviceContext;
> +
> +typedef struct AMFFramesContext {

Again namespace.

> +    AMFSurface * surfaces;
> +    int            nb_surfaces;
> +} AMFFramesContext;
> +
> +/**
> +* 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}

These should probably be internal.

> +
> +typedef struct FormatMap {
> +    enum AVPixelFormat       av_format;
> +    enum AMF_SURFACE_FORMAT  amf_format;
> +} FormatMap;
> +
> +extern const FormatMap format_map[];

This doesn't need to be in a public header.

> +enum AMF_SURFACE_FORMAT av_amf_av_to_amf_format(enum AVPixelFormat fmt);

Namespace.

> +enum AVPixelFormat av_amf_to_av_format(enum AMF_SURFACE_FORMAT fmt);
> +extern AmfTraceWriter av_amf_trace_writer;

Same as AmfTraceWriter, this probably doesn't need to be public.

> +
> +int av_amf_context_init(AVAMFDeviceContextInternal* internal, void* avcl);
> +int av_amf_load_library(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);

These should probably take a AVAMFDeviceContext pointer.

> +
> +#endif /* AVUTIL_HWCONTEXT_AMF_H */
> diff --git a/libavutil/hwcontext_internal.h b/libavutil/hwcontext_internal.h
> index 4df516ee6a..48d2dc012c 100644
> --- a/libavutil/hwcontext_internal.h
> +++ b/libavutil/hwcontext_internal.h
> @@ -175,5 +175,6 @@ extern const HWContextType ff_hwcontext_type_vdpau;
>   extern const HWContextType ff_hwcontext_type_videotoolbox;
>   extern const HWContextType ff_hwcontext_type_mediacodec;
>   extern const HWContextType ff_hwcontext_type_vulkan;
> +extern const HWContextType ff_hwcontext_type_amf;
>   
>   #endif /* AVUTIL_HWCONTEXT_INTERNAL_H */
> diff --git a/libavutil/pixdesc.c b/libavutil/pixdesc.c
> index f6d4d01460..ebc79b0c74 100644
> --- a/libavutil/pixdesc.c
> +++ b/libavutil/pixdesc.c
> @@ -2125,6 +2125,10 @@ static const AVPixFmtDescriptor av_pix_fmt_descriptors[AV_PIX_FMT_NB] = {
>           .name = "cuda",
>           .flags = AV_PIX_FMT_FLAG_HWACCEL,
>       },
> +    [AV_PIX_FMT_AMF] = {
> +        .name = "amf",
> +        .flags = AV_PIX_FMT_FLAG_HWACCEL,
> +    },
>       [AV_PIX_FMT_AYUV64LE] = {
>           .name = "ayuv64le",
>           .nb_components = 4,
> diff --git a/libavutil/pixfmt.h b/libavutil/pixfmt.h
> index 9c87571f49..06459be62c 100644
> --- a/libavutil/pixfmt.h
> +++ b/libavutil/pixfmt.h
> @@ -251,6 +251,10 @@ enum AVPixelFormat {
>        * exactly as for system memory frames.
>        */
>       AV_PIX_FMT_CUDA,
> +    /**
> +     * HW acceleration through AMF. data[3] contain AMFSurface pointer
> +     */
> +    AV_PIX_FMT_AMF,

This is an ABI break, it needs to be the last entry before AV_PIX_FMT_NB.

>   
>       AV_PIX_FMT_0RGB,        ///< packed RGB 8:8:8, 32bpp, XRGBXRGB...   X=unused/undefined
>       AV_PIX_FMT_RGB0,        ///< packed RGB 8:8:8, 32bpp, RGBXRGBX...   X=unused/undefined

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