[FFmpeg-devel,1/2] lavc/amfenc: moving amf common code (library and context) to lavu/hwcontext_amf from amfenc to be reused in other amf components

---
 libavcodec/amfenc.c            | 247 +++++--------------------------------
 libavcodec/amfenc.h            |  27 +---
 libavutil/Makefile             |   2 +
 libavutil/hwcontext.c          |   4 +
 libavutil/hwcontext.h          |   1 +
 libavutil/hwcontext_amf.c      | 271 +++++++++++++++++++++++++++++++++++++++++
 libavutil/hwcontext_amf.h      |  54 ++++++++
 libavutil/hwcontext_internal.h |   1 +
 8 files changed, 368 insertions(+), 239 deletions(-)
 create mode 100644 libavutil/hwcontext_amf.c
 create mode 100644 libavutil/hwcontext_amf.h

Hello.
It is reminder.
Could you please review the patch? if it is ok, could you apply it?
It was published 2 weeks ago and it is required for further updates
    
Thanks,
Alexander



On 10/07/18 15:28, Alexander Kravchenko wrote:
> ---
>  libavcodec/amfenc.c            | 247 +++++--------------------------------
>  libavcodec/amfenc.h            |  27 +---
>  libavutil/Makefile             |   2 +
>  libavutil/hwcontext.c          |   4 +
>  libavutil/hwcontext.h          |   1 +
>  libavutil/hwcontext_amf.c      | 271 +++++++++++++++++++++++++++++++++++++++++
>  libavutil/hwcontext_amf.h      |  54 ++++++++
>  libavutil/hwcontext_internal.h |   1 +
>  8 files changed, 368 insertions(+), 239 deletions(-)
>  create mode 100644 libavutil/hwcontext_amf.c
>  create mode 100644 libavutil/hwcontext_amf.h

I think this would make more sense as two patches - one to add the new code in libavutil, another to change libavcodec to use it.

(Below reordered to reflect that.)

> diff --git a/libavutil/Makefile b/libavutil/Makefile
> index 9ed24cfc82..c7d7f8b2d6 100644
> --- a/libavutil/Makefile
> +++ b/libavutil/Makefile
> @@ -168,6 +168,7 @@ OBJS-$(CONFIG_QSV)                      += hwcontext_qsv.o
>  OBJS-$(CONFIG_VAAPI)                    += hwcontext_vaapi.o
>  OBJS-$(CONFIG_VIDEOTOOLBOX)             += hwcontext_videotoolbox.o
>  OBJS-$(CONFIG_VDPAU)                    += hwcontext_vdpau.o
> +OBJS-$(CONFIG_AMF)                      += hwcontext_amf.o

Ordering.

>  
>  OBJS += $(COMPAT_OBJS:%=../compat/%)
>  
> @@ -183,6 +184,7 @@ SKIPHEADERS-$(CONFIG_OPENCL)           += hwcontext_opencl.h
>  SKIPHEADERS-$(CONFIG_VAAPI)            += hwcontext_vaapi.h
>  SKIPHEADERS-$(CONFIG_VIDEOTOOLBOX)     += hwcontext_videotoolbox.h
>  SKIPHEADERS-$(CONFIG_VDPAU)            += hwcontext_vdpau.h
> +SKIPHEADERS-$(CONFIG_AMF)              += hwcontext_amf.h
>  
>  TESTPROGS = adler32                                                     \
>              aes                                                         \
> diff --git a/libavutil/hwcontext.c b/libavutil/hwcontext.c
> index f1e404ab20..1a11c64764 100644
> --- a/libavutil/hwcontext.c
> +++ b/libavutil/hwcontext.c
> @@ -58,6 +58,9 @@ static const HWContextType * const hw_table[] = {
>  #endif
>  #if CONFIG_MEDIACODEC
>      &ff_hwcontext_type_mediacodec,
> +#endif
> +#if CONFIG_AMF
> +    &ff_hwcontext_type_amf,
>  #endif
>      NULL,
>  };
> @@ -73,6 +76,7 @@ static const char *const hw_type_names[] = {
>      [AV_HWDEVICE_TYPE_VDPAU]  = "vdpau",
>      [AV_HWDEVICE_TYPE_VIDEOTOOLBOX] = "videotoolbox",
>      [AV_HWDEVICE_TYPE_MEDIACODEC] = "mediacodec",
> +    [AV_HWDEVICE_TYPE_AMF] = "amf",

Ignoring the mediacodec entry, these look ordered.  I think put amf at the start in each case.

>  };
>  
>  enum AVHWDeviceType av_hwdevice_find_type_by_name(const char *name)
> diff --git a/libavutil/hwcontext.h b/libavutil/hwcontext.h
> index f5a4b62387..b18591205a 100644
> --- a/libavutil/hwcontext.h
> +++ b/libavutil/hwcontext.h
> @@ -36,6 +36,7 @@ enum AVHWDeviceType {
>      AV_HWDEVICE_TYPE_DRM,
>      AV_HWDEVICE_TYPE_OPENCL,
>      AV_HWDEVICE_TYPE_MEDIACODEC,
> +    AV_HWDEVICE_TYPE_AMF,
>  };
>  
>  typedef struct AVHWDeviceInternal AVHWDeviceInternal;
> diff --git a/libavutil/hwcontext_amf.c b/libavutil/hwcontext_amf.c
> new file mode 100644
> index 0000000000..a4c3c4f4c1
> --- /dev/null
> +++ b/libavutil/hwcontext_amf.c
> @@ -0,0 +1,271 @@
> +/*
> + * 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
> + */
> +
> +#include <string.h>
> +
> +#include "config.h"
> +
> +#include "avassert.h"
> +#include "avstring.h"
> +#include "common.h"

These three headers are all unused?

> +#include "hwcontext.h"
> +#include "hwcontext_internal.h"
> +#include "hwcontext_amf.h"
> +
> +#if CONFIG_D3D11VA
> +#include "libavutil/hwcontext_d3d11va.h"

No need for the path, this is libavutil.

> +#endif
> +
> +#if CONFIG_DXVA2
> +#define COBJMACROS
> +#include "libavutil/hwcontext_dxva2.h"

And here.

> +#endif
> +
> +#ifdef _WIN32
> +#include "compat/w32dlfcn.h"
> +#else
> +#include <dlfcn.h>
> +#endif
> +
> +/**
> +* Error handling helper
> +*/
> +#define AMFAV_RETURN_IF_FALSE(avctx, exp, ret_value, /*message,*/ ...) \
> +    if (!(exp)) { \
> +        av_log(avctx, AV_LOG_ERROR, __VA_ARGS__); \
> +        return ret_value; \
> +    }
> +
> +static const AVClass amflib_class = {
> +    .class_name = "amf",
> +    .item_name = av_default_item_name,
> +    .version = LIBAVUTIL_VERSION_INT,
> +};

This class shouldn't be needed - the right class to use is the one in the AVHWDeviceContext, you should be able to pass it to the right place via your AMFDeviceContextPrivate structure.

> +
> +typedef struct AMFLibraryContext {
> +    const AVClass      *avclass;
> +} AMFLibraryContext;
> +
> +static AMFLibraryContext amflib_context = 
> +{
> +    .avclass = &amflib_class,
> +};

This structure is just a dummy for the class?  Use the AVHWDeviceContext.

> +
> +typedef struct AmfTraceWriter {
> +    const AMFTraceWriterVtbl    *vtbl;
> +    void                        *avcl;
> +} AmfTraceWriter;
> +
> +static void AMF_CDECL_CALL AMFTraceWriter_Write(AMFTraceWriter *pThis,

It would be sensible to take the opportunity to fix the function name to conform to ffmpeg style.

> +    const wchar_t *scope, const wchar_t *message)
> +{
> +    AmfTraceWriter *tracer = (AmfTraceWriter*)pThis;
> +    av_log(tracer->avcl, AV_LOG_DEBUG, "%ls: %ls", scope, message);
> +}
> +
> +static void AMF_CDECL_CALL AMFTraceWriter_Flush(AMFTraceWriter *pThis)
> +{
> +}
> +
> +static const AMFTraceWriterVtbl tracer_vtbl =
> +{
> +    .Write = AMFTraceWriter_Write,
> +    .Flush = AMFTraceWriter_Flush,

Is this function really required to exist, given that it doesn't do anything?

> +};
> +
> +static const AmfTraceWriter amf_trace_writer = 
> +{
> +    .vtbl = &tracer_vtbl,
> +    .avcl = &amflib_context,
> +};

This should probably be inside the AMFDeviceContextPrivate, so that it can point to the right context structure.

> +#define AMFAV_WRITER_ID L"avlog"
> +
> +typedef struct AMFDeviceContextPrivate {
> +    amf_handle          library;
> +    AMFDebug           *debug;
> +    AMFTrace           *trace;
> +    AmfTraceWriter      tracer;
> +} AMFDeviceContextPrivate;
> +
> +static void amf_device_free(AVHWDeviceContext *ctx)
> +{
> +    AVAMFDeviceContext      *amf_ctx = ctx->hwctx;
> +    AMFDeviceContextPrivate *priv = ctx->internal->priv;
> +    if (amf_ctx->context) {
> +        amf_ctx->context->pVtbl->Terminate(amf_ctx->context);
> +        amf_ctx->context->pVtbl->Release(amf_ctx->context);
> +        amf_ctx->context = NULL;
> +    }
> +    if(priv->library) {
> +        dlclose(priv->library);
> +    }
> +}
> +
> +static int amf_init_device_ctx_object(AVHWDeviceContext *ctx)
> +{
> +    AVAMFDeviceContext         *hwctx = ctx->hwctx;
> +    AMFDeviceContextPrivate    *priv = ctx->internal->priv;
> +    AMF_RESULT                  res;
> +    AMFInit_Fn                  init_fun;
> +
> +    ctx->free = amf_device_free;
> +
> +    priv->library = dlopen(AMF_DLL_NAMEA, RTLD_NOW | RTLD_LOCAL);
> +    AMFAV_RETURN_IF_FALSE(ctx, priv->library != NULL, AVERROR_UNKNOWN, "DLL %s failed to open\n", AMF_DLL_NAMEA);
> +
> +    init_fun = (AMFInit_Fn)dlsym(priv->library, AMF_INIT_FUNCTION_NAME);
> +    AMFAV_RETURN_IF_FALSE(ctx, init_fun != NULL, AVERROR_UNKNOWN, "DLL %s failed to find function %s\n", AMF_DLL_NAMEA, AMF_INIT_FUNCTION_NAME);
> +
> +    res = init_fun(AMF_FULL_VERSION, &hwctx->factory);
> +    AMFAV_RETURN_IF_FALSE(ctx, res == AMF_OK, AVERROR_UNKNOWN, "%s failed with error %d\n", AMF_INIT_FUNCTION_NAME, res);
> +
> +    res = hwctx->factory->pVtbl->GetTrace(hwctx->factory, &priv->trace);
> +    AMFAV_RETURN_IF_FALSE(ctx, res == AMF_OK, AVERROR_UNKNOWN, "GetTrace() failed with error %d\n", res);
> +    res = hwctx->factory->pVtbl->GetDebug(hwctx->factory, &priv->debug);
> +    AMFAV_RETURN_IF_FALSE(ctx, res == AMF_OK, AVERROR_UNKNOWN, "GetDebug() failed with error %d\n", res);
> +
> +    priv->trace->pVtbl->EnableWriter(priv->trace, AMF_TRACE_WRITER_CONSOLE, 0);
> +    priv->trace->pVtbl->SetGlobalLevel(priv->trace, AMF_TRACE_TRACE);
> +
> +    // connect AMF logger to av_log
> +    priv->trace->pVtbl->RegisterWriter(priv->trace, AMFAV_WRITER_ID, (AMFTraceWriter*)&amf_trace_writer, 1);
> +    priv->trace->pVtbl->SetWriterLevel(priv->trace, AMFAV_WRITER_ID, AMF_TRACE_TRACE);
> +
> +    res = hwctx->factory->pVtbl->CreateContext(hwctx->factory, &hwctx->context);
> +    AMFAV_RETURN_IF_FALSE(ctx, res == AMF_OK, AVERROR_UNKNOWN, "CreateContext() failed with error %d\n", res);
> +    return 0;
> +}
> +
> +static int amf_device_create(AVHWDeviceContext *ctx, const char *device,
> +                                AVDictionary *opts, int flags)
> +{
> +    AVAMFDeviceContext *amf_ctx = ctx->hwctx;
> +    AMF_RESULT res;
> +    int err;
> +
> +    err = amf_init_device_ctx_object(ctx);
> +    if(err < 0)
> +        return err;
> +
> +    res = amf_ctx->context->pVtbl->InitDX11(amf_ctx->context, NULL, AMF_DX11_1);
> +    if (res == AMF_OK) {
> +        av_log(ctx, AV_LOG_VERBOSE, "AMF initialisation succeeded via D3D11.\n");
> +    } else {
> +        res = amf_ctx->context->pVtbl->InitDX9(amf_ctx->context, NULL);
> +        if (res == AMF_OK) {
> +            av_log(ctx, AV_LOG_VERBOSE, "AMF initialisation succeeded via D3D9.\n");
> +        } else {
> +            av_log(ctx, AV_LOG_ERROR, "AMF initialisation failed via D3D9: error %d.\n", res);
> +            return AVERROR(ENOSYS);
> +        }
> +    }
> +    return 0;
> +}
> +
> +static int amf_device_derive(AVHWDeviceContext *dst_ctx,
> +                                AVHWDeviceContext *src_ctx,
> +                                int flags)
> +{
> +    AVAMFDeviceContext *amf_ctx = dst_ctx->hwctx;
> +    AMF_RESULT res;

These two variables can give "unused" warnings, depending on configure options.  Move them into the #if branches, maybe?

IMO the separate functions from the code in amfenc was slightly clearer than this, too.

> +    int err;
> +
> +    err = amf_init_device_ctx_object(dst_ctx);
> +    if(err < 0)
> +        return err;
> +
> +    switch (src_ctx->type) {
> +
> +#if CONFIG_D3D11VA

DXVA2, not D3D11VA

> +    case AV_HWDEVICE_TYPE_DXVA2:
> +        {
> +            AVDXVA2DeviceContext *dxva2_ctx = src_ctx->hwctx;
> +            HANDLE device_handle;
> +            IDirect3DDevice9 *device;
> +            HRESULT hr;
> +            AMF_RESULT res;
> +            int ret;
> +
> +            hr = IDirect3DDeviceManager9_OpenDeviceHandle(dxva2_ctx->devmgr, &device_handle);
> +            if (FAILED(hr)) {
> +                av_log(dst_ctx, AV_LOG_ERROR, "Failed to open device handle for Direct3D9 device: %lx.\n", (unsigned long)hr);
> +                return AVERROR_EXTERNAL;
> +            }
> +
> +            hr = IDirect3DDeviceManager9_LockDevice(dxva2_ctx->devmgr, device_handle, &device, FALSE);
> +            if (SUCCEEDED(hr)) {
> +                IDirect3DDeviceManager9_UnlockDevice(dxva2_ctx->devmgr, device_handle, FALSE);
> +                ret = 0;
> +            } else {
> +                av_log(dst_ctx, AV_LOG_ERROR, "Failed to lock device handle for Direct3D9 device: %lx.\n", (unsigned long)hr);
> +                ret = AVERROR_EXTERNAL;
> +            }
> +
> +            IDirect3DDeviceManager9_CloseDeviceHandle(dxva2_ctx->devmgr, device_handle);
> +
> +            if (ret < 0)
> +                return ret;
> +
> +            res = amf_ctx->context->pVtbl->InitDX9(amf_ctx->context, device);
> +
> +            IDirect3DDevice9_Release(device);
> +
> +            if (res != AMF_OK) {
> +                if (res == AMF_NOT_SUPPORTED)
> +                    av_log(dst_ctx, AV_LOG_ERROR, "AMF via D3D9 is not supported on the given device.\n");
> +                else
> +                    av_log(dst_ctx, AV_LOG_ERROR, "AMF failed to initialise on given D3D9 device: %d.\n", res);
> +                return AVERROR(ENODEV);
> +            }

I suggest including a "success" message (maybe at VERBOSE?) in each branch of this function so that you know it successfully initialised and which sort of device it was made from.

(That was less useful when it was inside the encoder, but once it's standalone it's helpful to know it worked - e.g. "ffmpeg -v 55 -init_hw_device d3d11va=d11:0 -init_hw_device amf@d11" tells you about the d3d11 device only.)

> +        }
> +        break;
> +#endif
> +
> +#if CONFIG_D3D11VA
> +    case AV_HWDEVICE_TYPE_D3D11VA:
> +        {
> +            AVD3D11VADeviceContext *d3d11_ctx = src_ctx->hwctx;
> +            res = amf_ctx->context->pVtbl->InitDX11(amf_ctx->context, d3d11_ctx->device, AMF_DX11_1);
> +            if (res != AMF_OK) {
> +                if (res == AMF_NOT_SUPPORTED)
> +                    av_log(dst_ctx, AV_LOG_ERROR, "AMF via D3D11 is not supported on the given device.\n");
> +                else
> +                    av_log(dst_ctx, AV_LOG_ERROR, "AMF failed to initialise on the given D3D11 device: %d.\n", res);
> +                return AVERROR(ENODEV);
> +            }
> +        }
> +        break;
> +#endif
> +    default:
> +        av_log(dst_ctx, AV_LOG_ERROR, "AMF initialisation from a %s device is not supported.\n",
> +                av_hwdevice_get_type_name(src_ctx->type));
> +        return AVERROR(ENOSYS);
> +    }
> +    return 0;
> +}
> +
> +const HWContextType ff_hwcontext_type_amf = {
> +    .type                   = AV_HWDEVICE_TYPE_AMF,
> +    .name                   = "AMF",
> +
> +    .device_hwctx_size      = sizeof(AVAMFDeviceContext),
> +    .device_priv_size       = sizeof(AMFDeviceContextPrivate),
> +
> +    .device_create          = &amf_device_create,
> +    .device_derive          = &amf_device_derive,
> +};
> diff --git a/libavutil/hwcontext_amf.h b/libavutil/hwcontext_amf.h
> new file mode 100644
> index 0000000000..58e1c8a4bf
> --- /dev/null
> +++ b/libavutil/hwcontext_amf.h
> @@ -0,0 +1,54 @@
> +/*
> + * 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
> +
> +/**
> + * @file
> + * API-specific header for AV_HWDEVICE_TYPE_AMF.
> + *
> + */
> +
> +#include "frame.h"

Not used?

> +#include "AMF/core/Context.h"
> +#include "AMF/core/Factory.h"
> +
> +
> +/**
> + * This struct is allocated as AVHWDeviceContext.hwctx
> + */
> +typedef struct AVAMFDeviceContext {
> +    /**
> +     * Context used for:
> +     * texture and buffers allocation.
> +     * Access to device objects (DX9, DX11, OpenCL, OpenGL) which are being used in the context
> +     */
> +    AMFContext *context;
> +
> +    /**
> +     * Factory used for:
> +     * AMF component creation such as encoder, decoder, converter...
> +     * Access AMF Library settings such as trace/debug/cache
> +     */
> +    AMFFactory *factory;
> +} AVAMFDeviceContext;
> +
> +
> +#endif /* AVUTIL_HWCONTEXT_AMF_H */
> diff --git a/libavutil/hwcontext_internal.h b/libavutil/hwcontext_internal.h
> index 77dc47ddd6..b9680e4963 100644
> --- a/libavutil/hwcontext_internal.h
> +++ b/libavutil/hwcontext_internal.h
> @@ -172,5 +172,6 @@ extern const HWContextType ff_hwcontext_type_vaapi;
>  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_amf;
>  
>  #endif /* AVUTIL_HWCONTEXT_INTERNAL_H */
> 

--- separate patch ---

> diff --git a/libavcodec/amfenc.c b/libavcodec/amfenc.c
> index 384d8efc92..5a0fb90433 100644
> --- a/libavcodec/amfenc.c
> +++ b/libavcodec/amfenc.c
> @@ -21,13 +21,7 @@
>  #include "libavutil/avassert.h"
>  #include "libavutil/imgutils.h"
>  #include "libavutil/hwcontext.h"
> -#if CONFIG_D3D11VA
> -#include "libavutil/hwcontext_d3d11va.h"
> -#endif
> -#if CONFIG_DXVA2
> -#define COBJMACROS
> -#include "libavutil/hwcontext_dxva2.h"
> -#endif
> +

Extraneous newline.

>  #include "libavutil/mem.h"
>  #include "libavutil/pixdesc.h"
>  #include "libavutil/time.h"
> @@ -35,14 +29,12 @@
>  #include "amfenc.h"
>  #include "internal.h"
>  
> -#if CONFIG_D3D11VA
> -#include <d3d11.h>
> +#if CONFIG_DXVA2
> +#include <d3d9.h>
>  #endif
>  
> -#ifdef _WIN32
> -#include "compat/w32dlfcn.h"
> -#else
> -#include <dlfcn.h>
> +#if CONFIG_D3D11VA
> +#include <d3d11.h>
>  #endif

d3d9.h and d3d11.h are already included, with suitable conditionality, via hwcontext_amf.h, so just drop these.

>  
>  #define FFMPEG_AMF_WRITER_ID L"ffmpeg_amf"

Not used anywhere any more?

> @@ -88,34 +80,18 @@ static enum AMF_SURFACE_FORMAT amf_av_to_amf_format(enum AVPixelFormat fmt)
>      return AMF_SURFACE_UNKNOWN;
>  }
>  
> -static void AMF_CDECL_CALL AMFTraceWriter_Write(AMFTraceWriter *pThis,
> -    const wchar_t *scope, const wchar_t *message)
> -{
> -    AmfTraceWriter *tracer = (AmfTraceWriter*)pThis;
> -    av_log(tracer->avctx, AV_LOG_DEBUG, "%ls: %ls", scope, message); // \n is provided from AMF
> -}
>  
> -static void AMF_CDECL_CALL AMFTraceWriter_Flush(AMFTraceWriter *pThis)
> -{
> -}
> -
> -static AMFTraceWriterVtbl tracer_vtbl =
> -{
> -    .Write = AMFTraceWriter_Write,
> -    .Flush = AMFTraceWriter_Flush,
> -};
> -
> -static int amf_load_library(AVCodecContext *avctx)
> +static int amf_init_context(AVCodecContext *avctx)
>  {
> -    AmfContext        *ctx = avctx->priv_data;
> -    AMFInit_Fn         init_fun;
> -    AMFQueryVersion_Fn version_fun;
> -    AMF_RESULT         res;
> +    AmfContext *ctx = avctx->priv_data;
> +    AVAMFDeviceContext *amf_ctx;
> +    int ret;
>  
>      ctx->delayed_frame = av_frame_alloc();
>      if (!ctx->delayed_frame) {
>          return AVERROR(ENOMEM);
>      }
> +
>      // hardcoded to current HW queue size - will realloc in timestamp_queue_enqueue() if too small
>      ctx->timestamp_list = av_fifo_alloc((avctx->max_b_frames + 16) * sizeof(int64_t));
>      if (!ctx->timestamp_list) {
> @@ -123,119 +99,9 @@ static int amf_load_library(AVCodecContext *avctx)
>      }
>      ctx->dts_delay = 0;
>  
> -
> -    ctx->library = dlopen(AMF_DLL_NAMEA, RTLD_NOW | RTLD_LOCAL);
> -    AMF_RETURN_IF_FALSE(ctx, ctx->library != NULL,
> -        AVERROR_UNKNOWN, "DLL %s failed to open\n", AMF_DLL_NAMEA);
> -
> -    init_fun = (AMFInit_Fn)dlsym(ctx->library, AMF_INIT_FUNCTION_NAME);
> -    AMF_RETURN_IF_FALSE(ctx, init_fun != NULL, AVERROR_UNKNOWN, "DLL %s failed to find function %s\n", AMF_DLL_NAMEA, AMF_INIT_FUNCTION_NAME);
> -
> -    version_fun = (AMFQueryVersion_Fn)dlsym(ctx->library, AMF_QUERY_VERSION_FUNCTION_NAME);
> -    AMF_RETURN_IF_FALSE(ctx, version_fun != NULL, AVERROR_UNKNOWN, "DLL %s failed to find function %s\n", AMF_DLL_NAMEA, AMF_QUERY_VERSION_FUNCTION_NAME);
> -
> -    res = version_fun(&ctx->version);
> -    AMF_RETURN_IF_FALSE(ctx, res == AMF_OK, AVERROR_UNKNOWN, "%s failed with error %d\n", AMF_QUERY_VERSION_FUNCTION_NAME, res);
> -    res = init_fun(AMF_FULL_VERSION, &ctx->factory);
> -    AMF_RETURN_IF_FALSE(ctx, res == AMF_OK, AVERROR_UNKNOWN, "%s failed with error %d\n", AMF_INIT_FUNCTION_NAME, res);
> -    res = ctx->factory->pVtbl->GetTrace(ctx->factory, &ctx->trace);
> -    AMF_RETURN_IF_FALSE(ctx, res == AMF_OK, AVERROR_UNKNOWN, "GetTrace() failed with error %d\n", res);
> -    res = ctx->factory->pVtbl->GetDebug(ctx->factory, &ctx->debug);
> -    AMF_RETURN_IF_FALSE(ctx, res == AMF_OK, AVERROR_UNKNOWN, "GetDebug() failed with error %d\n", res);
> -    return 0;
> -}
> -
> -#if CONFIG_D3D11VA
> -static int amf_init_from_d3d11_device(AVCodecContext *avctx, AVD3D11VADeviceContext *hwctx)
> -{
> -    AmfContext *ctx = avctx->priv_data;
> -    AMF_RESULT res;
> -
> -    res = ctx->context->pVtbl->InitDX11(ctx->context, hwctx->device, AMF_DX11_1);
> -    if (res != AMF_OK) {
> -        if (res == AMF_NOT_SUPPORTED)
> -            av_log(avctx, AV_LOG_ERROR, "AMF via D3D11 is not supported on the given device.\n");
> -        else
> -            av_log(avctx, AV_LOG_ERROR, "AMF failed to initialise on the given D3D11 device: %d.\n", res);
> -        return AVERROR(ENODEV);
> -    }
> -
> -    return 0;
> -}
> -#endif
> -
> -#if CONFIG_DXVA2
> -static int amf_init_from_dxva2_device(AVCodecContext *avctx, AVDXVA2DeviceContext *hwctx)
> -{
> -    AmfContext *ctx = avctx->priv_data;
> -    HANDLE device_handle;
> -    IDirect3DDevice9 *device;
> -    HRESULT hr;
> -    AMF_RESULT res;
> -    int ret;
> -
> -    hr = IDirect3DDeviceManager9_OpenDeviceHandle(hwctx->devmgr, &device_handle);
> -    if (FAILED(hr)) {
> -        av_log(avctx, AV_LOG_ERROR, "Failed to open device handle for Direct3D9 device: %lx.\n", (unsigned long)hr);
> -        return AVERROR_EXTERNAL;
> -    }
> -
> -    hr = IDirect3DDeviceManager9_LockDevice(hwctx->devmgr, device_handle, &device, FALSE);
> -    if (SUCCEEDED(hr)) {
> -        IDirect3DDeviceManager9_UnlockDevice(hwctx->devmgr, device_handle, FALSE);
> -        ret = 0;
> -    } else {
> -        av_log(avctx, AV_LOG_ERROR, "Failed to lock device handle for Direct3D9 device: %lx.\n", (unsigned long)hr);
> -        ret = AVERROR_EXTERNAL;
> -    }
> -
> -    IDirect3DDeviceManager9_CloseDeviceHandle(hwctx->devmgr, device_handle);
> -
> -    if (ret < 0)
> -        return ret;
> -
> -    res = ctx->context->pVtbl->InitDX9(ctx->context, device);
> -
> -    IDirect3DDevice9_Release(device);
> -
> -    if (res != AMF_OK) {
> -        if (res == AMF_NOT_SUPPORTED)
> -            av_log(avctx, AV_LOG_ERROR, "AMF via D3D9 is not supported on the given device.\n");
> -        else
> -            av_log(avctx, AV_LOG_ERROR, "AMF failed to initialise on given D3D9 device: %d.\n", res);
> -        return AVERROR(ENODEV);
> -    }
> -
> -    return 0;
> -}
> -#endif
> -
> -static int amf_init_context(AVCodecContext *avctx)
> -{
> -    AmfContext *ctx = avctx->priv_data;
> -    AMF_RESULT  res;
> -    av_unused int ret;
> -
>      ctx->hwsurfaces_in_queue = 0;
>      ctx->hwsurfaces_in_queue_max = 16;
>  
> -    // configure AMF logger
> -    // the return of these functions indicates old state and do not affect behaviour
> -    ctx->trace->pVtbl->EnableWriter(ctx->trace, AMF_TRACE_WRITER_DEBUG_OUTPUT, ctx->log_to_dbg != 0 );
> -    if (ctx->log_to_dbg)
> -        ctx->trace->pVtbl->SetWriterLevel(ctx->trace, AMF_TRACE_WRITER_DEBUG_OUTPUT, AMF_TRACE_TRACE);
> -    ctx->trace->pVtbl->EnableWriter(ctx->trace, AMF_TRACE_WRITER_CONSOLE, 0);
> -    ctx->trace->pVtbl->SetGlobalLevel(ctx->trace, AMF_TRACE_TRACE);
> -
> -    // connect AMF logger to av_log
> -    ctx->tracer.vtbl = &tracer_vtbl;
> -    ctx->tracer.avctx = avctx;
> -    ctx->trace->pVtbl->RegisterWriter(ctx->trace, FFMPEG_AMF_WRITER_ID,(AMFTraceWriter*)&ctx->tracer, 1);
> -    ctx->trace->pVtbl->SetWriterLevel(ctx->trace, FFMPEG_AMF_WRITER_ID, AMF_TRACE_TRACE);
> -
> -    res = ctx->factory->pVtbl->CreateContext(ctx->factory, &ctx->context);
> -    AMF_RETURN_IF_FALSE(ctx, res == AMF_OK, AVERROR_UNKNOWN, "CreateContext() failed with error %d\n", res);
> -
>      // If a device was passed to the encoder, try to initialise from that.
>      if (avctx->hw_frames_ctx) {
>          AVHWFramesContext *frames_ctx = (AVHWFramesContext*)avctx->hw_frames_ctx->data;
> @@ -246,26 +112,9 @@ static int amf_init_context(AVCodecContext *avctx)
>              return AVERROR(EINVAL);
>          }
>  
> -        switch (frames_ctx->device_ctx->type) {
> -#if CONFIG_D3D11VA
> -        case AV_HWDEVICE_TYPE_D3D11VA:
> -            ret = amf_init_from_d3d11_device(avctx, frames_ctx->device_ctx->hwctx);
> -            if (ret < 0)
> -                return ret;
> -            break;
> -#endif
> -#if CONFIG_DXVA2
> -        case AV_HWDEVICE_TYPE_DXVA2:
> -            ret = amf_init_from_dxva2_device(avctx, frames_ctx->device_ctx->hwctx);
> -            if (ret < 0)
> -                return ret;
> -            break;
> -#endif
> -        default:
> -            av_log(avctx, AV_LOG_ERROR, "AMF initialisation from a %s frames context is not supported.\n",
> -                   av_hwdevice_get_type_name(frames_ctx->device_ctx->type));
> -            return AVERROR(ENOSYS);
> -        }
> +        ret = av_hwdevice_ctx_create_derived(&ctx->amf_device_ctx, AV_HWDEVICE_TYPE_AMF, frames_ctx->device_ref, 0);
> +        if (ret < 0)
> +            return ret;
>  
>          ctx->hw_frames_ctx = av_buffer_ref(avctx->hw_frames_ctx);
>          if (!ctx->hw_frames_ctx)
> @@ -275,47 +124,23 @@ static int amf_init_context(AVCodecContext *avctx)
>              ctx->hwsurfaces_in_queue_max = frames_ctx->initial_pool_size - 1;
>  
>      } else if (avctx->hw_device_ctx) {
> -        AVHWDeviceContext *device_ctx = (AVHWDeviceContext*)avctx->hw_device_ctx->data;
> -
> -        switch (device_ctx->type) {
> -#if CONFIG_D3D11VA
> -        case AV_HWDEVICE_TYPE_D3D11VA:
> -            ret = amf_init_from_d3d11_device(avctx, device_ctx->hwctx);
> -            if (ret < 0)
> -                return ret;
> -            break;
> -#endif
> -#if CONFIG_DXVA2
> -        case AV_HWDEVICE_TYPE_DXVA2:
> -            ret = amf_init_from_dxva2_device(avctx, device_ctx->hwctx);
> -            if (ret < 0)
> -                return ret;
> -            break;
> -#endif
> -        default:
> -            av_log(avctx, AV_LOG_ERROR, "AMF initialisation from a %s device is not supported.\n",
> -                   av_hwdevice_get_type_name(device_ctx->type));
> -            return AVERROR(ENOSYS);
> -        }
> +        ret = av_hwdevice_ctx_create_derived(&ctx->amf_device_ctx, AV_HWDEVICE_TYPE_AMF, avctx->hw_device_ctx, 0);
> +        if (ret < 0)
> +            return ret;
>  
>          ctx->hw_device_ctx = av_buffer_ref(avctx->hw_device_ctx);
>          if (!ctx->hw_device_ctx)
>              return AVERROR(ENOMEM);

The derivation already holds a ref internally, so this isn't needed any more (you never want to look inside the source device).

>  
>      } else {
> -        res = ctx->context->pVtbl->InitDX11(ctx->context, NULL, AMF_DX11_1);
> -        if (res == AMF_OK) {
> -            av_log(avctx, AV_LOG_VERBOSE, "AMF initialisation succeeded via D3D11.\n");
> -        } else {
> -            res = ctx->context->pVtbl->InitDX9(ctx->context, NULL);
> -            if (res == AMF_OK) {
> -                av_log(avctx, AV_LOG_VERBOSE, "AMF initialisation succeeded via D3D9.\n");
> -            } else {
> -                av_log(avctx, AV_LOG_ERROR, "AMF initialisation failed via D3D9: error %d.\n", res);
> -                return AVERROR(ENOSYS);
> -            }
> -        }
> +        ret = av_hwdevice_ctx_create(&ctx->amf_device_ctx, AV_HWDEVICE_TYPE_AMF, NULL, NULL, 0);
> +        if (ret < 0)
> +            return ret;
>      }
> +
> +    amf_ctx = ((AVHWDeviceContext*)ctx->amf_device_ctx->data)->hwctx;
> +    ctx->context = amf_ctx->context;
> +    ctx->factory = amf_ctx->factory;
>      return 0;
>  }
>  
> @@ -368,29 +193,17 @@ int av_cold ff_amf_encode_close(AVCodecContext *avctx)
>          ctx->encoder = NULL;
>      }
>  
> -    if (ctx->context) {
> -        ctx->context->pVtbl->Terminate(ctx->context);
> -        ctx->context->pVtbl->Release(ctx->context);
> -        ctx->context = NULL;
> -    }
>      av_buffer_unref(&ctx->hw_device_ctx);
>      av_buffer_unref(&ctx->hw_frames_ctx);
>  
> -    if (ctx->trace) {
> -        ctx->trace->pVtbl->UnregisterWriter(ctx->trace, FFMPEG_AMF_WRITER_ID);
> -    }
> -    if (ctx->library) {
> -        dlclose(ctx->library);
> -        ctx->library = NULL;
> -    }
> -    ctx->trace = NULL;
> -    ctx->debug = NULL;
>      ctx->factory = NULL;
> -    ctx->version = 0;
> +    ctx->context = NULL;
>      ctx->delayed_drain = 0;
>      av_frame_free(&ctx->delayed_frame);
>      av_fifo_freep(&ctx->timestamp_list);
>  
> +    av_buffer_unref(&ctx->amf_device_ctx);
> +
>      return 0;
>  }
>  
> @@ -494,11 +307,9 @@ int ff_amf_encode_init(AVCodecContext *avctx)
>  {
>      int ret;
>  
> -    if ((ret = amf_load_library(avctx)) == 0) {
> -        if ((ret = amf_init_context(avctx)) == 0) {
> -            if ((ret = amf_init_encoder(avctx)) == 0) {
> -                return 0;
> -            }
> +    if ((ret = amf_init_context(avctx)) == 0) {
> +        if ((ret = amf_init_encoder(avctx)) == 0) {
> +            return 0;
>          }
>      }
>      ff_amf_encode_close(avctx);
> diff --git a/libavcodec/amfenc.h b/libavcodec/amfenc.h
> index b1361842bd..9ce577fa8f 100644
> --- a/libavcodec/amfenc.h
> +++ b/libavcodec/amfenc.h
> @@ -19,41 +19,26 @@
>  #ifndef AVCODEC_AMFENC_H
>  #define AVCODEC_AMFENC_H
>  
> -#include <AMF/core/Factory.h>
> -
>  #include <AMF/components/VideoEncoderVCE.h>
>  #include <AMF/components/VideoEncoderHEVC.h>

Kindof unrelated, but is there any reason why both of these are in the header rather than in the per-codec files?

>  
>  #include "libavutil/fifo.h"
> +#include "libavutil/hwcontext_amf.h"
>  
>  #include "avcodec.h"
>  
>  
> -/**
> -* AMF trace writer callback class
> -* Used to capture all AMF logging
> -*/
> -
> -typedef struct AmfTraceWriter {
> -    AMFTraceWriterVtbl *vtbl;
> -    AVCodecContext     *avctx;
> -} AmfTraceWriter;
> -
>  /**
>  * AMF encoder context
>  */
>  
>  typedef struct AmfContext {
>      AVClass            *avclass;
> -    // access to AMF runtime
> -    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
> -    AmfTraceWriter      tracer;  ///< AMF writer registered with AMF
> -    AMFContext         *context; ///< AMF context
> +
> +    AMFContext         *context;
> +    AMFFactory         *factory;
> +    AVBufferRef        *amf_device_ctx;
> +
>      //encoder
>      AMFComponent       *encoder; ///< AMF encoder object
>      amf_bool            eof;     ///< flag indicating EOF happened

The log_to_dbg option is orphaned by this change.  Is it worth keeping?  (If you want to keep it then maybe it could be a named option to av_hwdevice_ctx_create() -> amf_device_create().)

- Mark

Hi, Mark.
Thanks for review.
Could you please check the following comments/questions?

> +
> > +static const AVClass amflib_class = {
> > +    .class_name = "amf",
> > +    .item_name = av_default_item_name,
> > +    .version = LIBAVUTIL_VERSION_INT,
> > +};
>
> This class shouldn't be needed - the right class to use is the one in the
> AVHWDeviceContext, you should be able to pass it to the right place via
> your AMFDeviceContextPrivate structure.
>
> > +
> > +typedef struct AMFLibraryContext {
> > +    const AVClass      *avclass;
> > +} AMFLibraryContext;
> > +
> > +static AMFLibraryContext amflib_context =
> > +{
> > +    .avclass = &amflib_class,
> > +};
>
> This structure is just a dummy for the class?  Use the AVHWDeviceContext.
>
> > +
> > +typedef struct AmfTraceWriter {
> > +    const AMFTraceWriterVtbl    *vtbl;
> > +    void                        *avcl;
> > +} AmfTraceWriter;
> > +
> > +static void AMF_CDECL_CALL AMFTraceWriter_Write(AMFTraceWriter *pThis,
>
> It would be sensible to take the opportunity to fix the function name to
> conform to ffmpeg style.
>
> > +    const wchar_t *scope, const wchar_t *message)
> > +{
> > +    AmfTraceWriter *tracer = (AmfTraceWriter*)pThis;
> > +    av_log(tracer->avcl, AV_LOG_DEBUG, "%ls: %ls", scope, message);
> > +}
> > +
> > +static void AMF_CDECL_CALL AMFTraceWriter_Flush(AMFTraceWriter *pThis)
> > +{
> > +}
> > +
> > +static const AMFTraceWriterVtbl tracer_vtbl =
> > +{
> > +    .Write = AMFTraceWriter_Write,
> > +    .Flush = AMFTraceWriter_Flush,
>
> Is this function really required to exist, given that it doesn't do
> anything?
>
> > +};
> > +
> > +static const AmfTraceWriter amf_trace_writer =
> > +{
> > +    .vtbl = &tracer_vtbl,
> > +    .avcl = &amflib_context,
> > +};
>
> This should probably be inside the AMFDeviceContextPrivate, so that it can
> point to the right context structure.
>
> This is the question.
AMF Library has global Trace settings, not per AMFContext object.
My intension was to create global AMF lib class and Tracer object which
refers to it as class parameter in av_log call.
It is required in scenario when multiple hwcontext_amf are created during
application lifecycle.
if this way is ok, should I add comments to code describes this? or is
there another way to have global object to handle this?

AMFTraceWriterVtbl.Flush - I am not sure that it can be set as null
pointer, I could double check with AMD developers.




> >  #include <AMF/components/VideoEncoderVCE.h>
> >  #include <AMF/components/VideoEncoderHEVC.h>
>
> Kindof unrelated, but is there any reason why both of these are in the
> header rather than in the per-codec files?
>
>
Component management code is the same for all encoder components.
The only encoder id defines is used for component creation here.



>
> The log_to_dbg option is orphaned by this change.  Is it worth keeping?
> (If you want to keep it then maybe it could be a named option to
> av_hwdevice_ctx_create() -> amf_device_create().)
>
> log_to_dbg is removed from here, because this setting is global for AMF
library, not per component(encoder) or per AMFContext(hwcontext_amf object)

Probably I could implement some global AMF lib options which configures
tracer more precisely in general

Thanks,
Alexander

On 29/10/18 11:45, Alexander Kravchenko wrote:
> Hi, Mark.
> Thanks for review.
> Could you please check the following comments/questions?
> 
>> +
>>> +static const AVClass amflib_class = {
>>> +    .class_name = "amf",
>>> +    .item_name = av_default_item_name,
>>> +    .version = LIBAVUTIL_VERSION_INT,
>>> +};
>>
>> This class shouldn't be needed - the right class to use is the one in the
>> AVHWDeviceContext, you should be able to pass it to the right place via
>> your AMFDeviceContextPrivate structure.
>>
>>> +
>>> +typedef struct AMFLibraryContext {
>>> +    const AVClass      *avclass;
>>> +} AMFLibraryContext;
>>> +
>>> +static AMFLibraryContext amflib_context =
>>> +{
>>> +    .avclass = &amflib_class,
>>> +};
>>
>> This structure is just a dummy for the class?  Use the AVHWDeviceContext.
>>
>>> +
>>> +typedef struct AmfTraceWriter {
>>> +    const AMFTraceWriterVtbl    *vtbl;
>>> +    void                        *avcl;
>>> +} AmfTraceWriter;
>>> +
>>> +static void AMF_CDECL_CALL AMFTraceWriter_Write(AMFTraceWriter *pThis,
>>
>> It would be sensible to take the opportunity to fix the function name to
>> conform to ffmpeg style.
>>
>>> +    const wchar_t *scope, const wchar_t *message)
>>> +{
>>> +    AmfTraceWriter *tracer = (AmfTraceWriter*)pThis;
>>> +    av_log(tracer->avcl, AV_LOG_DEBUG, "%ls: %ls", scope, message);
>>> +}
>>> +
>>> +static void AMF_CDECL_CALL AMFTraceWriter_Flush(AMFTraceWriter *pThis)
>>> +{
>>> +}
>>> +
>>> +static const AMFTraceWriterVtbl tracer_vtbl =
>>> +{
>>> +    .Write = AMFTraceWriter_Write,
>>> +    .Flush = AMFTraceWriter_Flush,
>>
>> Is this function really required to exist, given that it doesn't do
>> anything?
>>
>>> +};
>>> +
>>> +static const AmfTraceWriter amf_trace_writer =
>>> +{
>>> +    .vtbl = &tracer_vtbl,
>>> +    .avcl = &amflib_context,
>>> +};
>>
>> This should probably be inside the AMFDeviceContextPrivate, so that it can
>> point to the right context structure.
>>
>> This is the question.
> AMF Library has global Trace settings, not per AMFContext object.

So that global state is inside the AMF library?

How does that interact with the fact that you reload it and reconfigure it every time it gets loaded - if one thread calls amf_device_create() while another one is inside the encoder and generating log output (say), what happens?

(I also note that it presumably means the current AMF encoder code will crash if you have two encoders with nested lifetimes - the second encoder will overwrite the global state, and once it finishes and gets freed the global trace output from the first encoder will have an invalid class pointer.)

> My intension was to create global AMF lib class and Tracer object which
> refers to it as class parameter in av_log call.
> It is required in scenario when multiple hwcontext_amf are created during
> application lifecycle.
> if this way is ok, should I add comments to code describes this? or is
> there another way to have global object to handle this?
Global state in libraries really isn't ok.  I think in response to that I would force it to be completely off by default, maybe switch it on if the user passes some special named option to amf_device_create()?

> AMFTraceWriterVtbl.Flush - I am not sure that it can be set as null
> pointer, I could double check with AMD developers.

It doesn't really matter; just the empty function didn't seem useful.

>>>  #include <AMF/components/VideoEncoderVCE.h>
>>>  #include <AMF/components/VideoEncoderHEVC.h>
>>
>> Kindof unrelated, but is there any reason why both of these are in the
>> header rather than in the per-codec files?
>>
>>
> Component management code is the same for all encoder components.
> The only encoder id defines is used for component creation here.

Yep, missed that.  Sure.

>>
>> The log_to_dbg option is orphaned by this change.  Is it worth keeping?
>> (If you want to keep it then maybe it could be a named option to
>> av_hwdevice_ctx_create() -> amf_device_create().)
>>
>> log_to_dbg is removed from here, because this setting is global for AMF
> library, not per component(encoder) or per AMFContext(hwcontext_amf object)
> 
> Probably I could implement some global AMF lib options which configures
> tracer more precisely in general

Comment above applies, I think.

- Mark

Hi Mark,
see my comments bellow.

вт, 30 окт. 2018 г. в 0:23, Mark Thompson <sw@jkqxz.net>:

> On 29/10/18 11:45, Alexander Kravchenko wrote:
> > Hi, Mark.
> > Thanks for review.
> > Could you please check the following comments/questions?
> >
> >> +
> >>> +static const AVClass amflib_class = {
> >>> +    .class_name = "amf",
> >>> +    .item_name = av_default_item_name,
> >>> +    .version = LIBAVUTIL_VERSION_INT,
> >>> +};
> >>
> >> This class shouldn't be needed - the right class to use is the one in
> the
> >> AVHWDeviceContext, you should be able to pass it to the right place via
> >> your AMFDeviceContextPrivate structure.
> >>
> >>> +
> >>> +typedef struct AMFLibraryContext {
> >>> +    const AVClass      *avclass;
> >>> +} AMFLibraryContext;
> >>> +
> >>> +static AMFLibraryContext amflib_context =
> >>> +{
> >>> +    .avclass = &amflib_class,
> >>> +};
> >>
> >> This structure is just a dummy for the class?  Use the
> AVHWDeviceContext.
> >>
> >>> +
> >>> +typedef struct AmfTraceWriter {
> >>> +    const AMFTraceWriterVtbl    *vtbl;
> >>> +    void                        *avcl;
> >>> +} AmfTraceWriter;
> >>> +
> >>> +static void AMF_CDECL_CALL AMFTraceWriter_Write(AMFTraceWriter *pThis,
> >>
> >> It would be sensible to take the opportunity to fix the function name to
> >> conform to ffmpeg style.
> >>
> >>> +    const wchar_t *scope, const wchar_t *message)
> >>> +{
> >>> +    AmfTraceWriter *tracer = (AmfTraceWriter*)pThis;
> >>> +    av_log(tracer->avcl, AV_LOG_DEBUG, "%ls: %ls", scope, message);
> >>> +}
> >>> +
> >>> +static void AMF_CDECL_CALL AMFTraceWriter_Flush(AMFTraceWriter *pThis)
> >>> +{
> >>> +}
> >>> +
> >>> +static const AMFTraceWriterVtbl tracer_vtbl =
> >>> +{
> >>> +    .Write = AMFTraceWriter_Write,
> >>> +    .Flush = AMFTraceWriter_Flush,
> >>
> >> Is this function really required to exist, given that it doesn't do
> >> anything?
> >>
> >>> +};
> >>> +
> >>> +static const AmfTraceWriter amf_trace_writer =
> >>> +{
> >>> +    .vtbl = &tracer_vtbl,
> >>> +    .avcl = &amflib_context,
> >>> +};
> >>
> >> This should probably be inside the AMFDeviceContextPrivate, so that it
> can
> >> point to the right context structure.
> >>
> >> This is the question.
> > AMF Library has global Trace settings, not per AMFContext object.
>
> So that global state is inside the AMF library?
>
> How does that interact with the fact that you reload it and reconfigure it
> every time it gets loaded - if one thread calls amf_device_create() while
> another one is inside the encoder and generating log output (say), what
> happens?
>

One of my first proposed patch had singleton amf_library, which was
refcounted. main goal of it was init library (set Trace options) when the
first reference is requested and clean when it was finally released (last
hwcontext_amf is destroyed).
I was told that global state is not good (global amf_lib object keeper) and
I removed it and now Tracer options are updated each time amf_device_create
is called.

Now there is no global state in avutils/hwcontext_amf : all globals here
are const static objects. The only global state in AMF Library itself
(configuring tracer is thread-safe in AMF).


> (I also note that it presumably means the current AMF encoder code will
> crash if you have two encoders with nested lifetimes - the second encoder
> will overwrite the global state, and once it finishes and gets freed the
> global trace output from the first encoder will have an invalid class
> pointer.)
>

Yes, I aware the issue since I started to support amfenc and this is the
first reason to move amf lib code to hwcontext_amf.



> > My intension was to create global AMF lib class and Tracer object which
> > refers to it as class parameter in av_log call.
> > It is required in scenario when multiple hwcontext_amf are created during
> > application lifecycle.
> > if this way is ok, should I add comments to code describes this? or is
> > there another way to have global object to handle this?
> Global state in libraries really isn't ok.  I think in response to that I
> would force it to be completely off by default, maybe switch it on if the
> user passes some special named option to amf_device_create()?
>

Probably this is the option, but current implementation should not cause
any issues except multiple work of tracer configuration.


> > AMFTraceWriterVtbl.Flush - I am not sure that it can be set as null
> > pointer, I could double check with AMD developers.
>
> It doesn't really matter; just the empty function didn't seem useful.
>
> >>>  #include <AMF/components/VideoEncoderVCE.h>
> >>>  #include <AMF/components/VideoEncoderHEVC.h>
> >>
> >> Kindof unrelated, but is there any reason why both of these are in the
> >> header rather than in the per-codec files?
> >>
> >>
> > Component management code is the same for all encoder components.
> > The only encoder id defines is used for component creation here.
>
> Yep, missed that.  Sure.
>
> >>
> >> The log_to_dbg option is orphaned by this change.  Is it worth keeping?
> >> (If you want to keep it then maybe it could be a named option to
> >> av_hwdevice_ctx_create() -> amf_device_create().)
> >>
> >> log_to_dbg is removed from here, because this setting is global for AMF
> > library, not per component(encoder) or per AMFContext(hwcontext_amf
> object)
> >
> > Probably I could implement some global AMF lib options which configures
> > tracer more precisely in general
>
> Comment above applies, I think.
>
> - Mark
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

On 29/10/18 22:16, Alexander Kravchenko wrote:
> Hi Mark,
> see my comments bellow.
> 
> вт, 30 окт. 2018 г. в 0:23, Mark Thompson <sw@jkqxz.net>:
> 
>> On 29/10/18 11:45, Alexander Kravchenko wrote:
>>> Hi, Mark.
>>> Thanks for review.
>>> Could you please check the following comments/questions?
>>>
>>>> +
>>>>> +static const AVClass amflib_class = {
>>>>> +    .class_name = "amf",
>>>>> +    .item_name = av_default_item_name,
>>>>> +    .version = LIBAVUTIL_VERSION_INT,
>>>>> +};
>>>>
>>>> This class shouldn't be needed - the right class to use is the one in
>> the
>>>> AVHWDeviceContext, you should be able to pass it to the right place via
>>>> your AMFDeviceContextPrivate structure.
>>>>
>>>>> +
>>>>> +typedef struct AMFLibraryContext {
>>>>> +    const AVClass      *avclass;
>>>>> +} AMFLibraryContext;
>>>>> +
>>>>> +static AMFLibraryContext amflib_context =
>>>>> +{
>>>>> +    .avclass = &amflib_class,
>>>>> +};
>>>>
>>>> This structure is just a dummy for the class?  Use the
>> AVHWDeviceContext.
>>>>
>>>>> +
>>>>> +typedef struct AmfTraceWriter {
>>>>> +    const AMFTraceWriterVtbl    *vtbl;
>>>>> +    void                        *avcl;
>>>>> +} AmfTraceWriter;
>>>>> +
>>>>> +static void AMF_CDECL_CALL AMFTraceWriter_Write(AMFTraceWriter *pThis,
>>>>
>>>> It would be sensible to take the opportunity to fix the function name to
>>>> conform to ffmpeg style.
>>>>
>>>>> +    const wchar_t *scope, const wchar_t *message)
>>>>> +{
>>>>> +    AmfTraceWriter *tracer = (AmfTraceWriter*)pThis;
>>>>> +    av_log(tracer->avcl, AV_LOG_DEBUG, "%ls: %ls", scope, message);
>>>>> +}
>>>>> +
>>>>> +static void AMF_CDECL_CALL AMFTraceWriter_Flush(AMFTraceWriter *pThis)
>>>>> +{
>>>>> +}
>>>>> +
>>>>> +static const AMFTraceWriterVtbl tracer_vtbl =
>>>>> +{
>>>>> +    .Write = AMFTraceWriter_Write,
>>>>> +    .Flush = AMFTraceWriter_Flush,
>>>>
>>>> Is this function really required to exist, given that it doesn't do
>>>> anything?
>>>>
>>>>> +};
>>>>> +
>>>>> +static const AmfTraceWriter amf_trace_writer =
>>>>> +{
>>>>> +    .vtbl = &tracer_vtbl,
>>>>> +    .avcl = &amflib_context,
>>>>> +};
>>>>
>>>> This should probably be inside the AMFDeviceContextPrivate, so that it
>> can
>>>> point to the right context structure.
>>>>
>>>> This is the question.
>>> AMF Library has global Trace settings, not per AMFContext object.
>>
>> So that global state is inside the AMF library?
>>
>> How does that interact with the fact that you reload it and reconfigure it
>> every time it gets loaded - if one thread calls amf_device_create() while
>> another one is inside the encoder and generating log output (say), what
>> happens?
>>
> 
> One of my first proposed patch had singleton amf_library, which was
> refcounted. main goal of it was init library (set Trace options) when the
> first reference is requested and clean when it was finally released (last
> hwcontext_amf is destroyed).
> I was told that global state is not good (global amf_lib object keeper) and
> I removed it and now Tracer options are updated each time amf_device_create
> is called.
> 
> Now there is no global state in avutils/hwcontext_amf : all globals here
> are const static objects. The only global state in AMF Library itself
> (configuring tracer is thread-safe in AMF).

Oh well.  I guess this ends up being the least objectionable way of dealing with the API, so ok.

>> (I also note that it presumably means the current AMF encoder code will
>> crash if you have two encoders with nested lifetimes - the second encoder
>> will overwrite the global state, and once it finishes and gets freed the
>> global trace output from the first encoder will have an invalid class
>> pointer.)
>>
> 
> Yes, I aware the issue since I started to support amfenc and this is the
> first reason to move amf lib code to hwcontext_amf.

Right, makes sense.

>>> My intension was to create global AMF lib class and Tracer object which
>>> refers to it as class parameter in av_log call.
>>> It is required in scenario when multiple hwcontext_amf are created during
>>> application lifecycle.
>>> if this way is ok, should I add comments to code describes this? or is
>>> there another way to have global object to handle this?
>> Global state in libraries really isn't ok.  I think in response to that I
>> would force it to be completely off by default, maybe switch it on if the
>> user passes some special named option to amf_device_create()?
>>
> 
> Probably this is the option, but current implementation should not cause
> any issues except multiple work of tracer configuration.

Given the above, fair enough.

Thanks,

- Mark