[FFmpeg-devel] [PATCH v2] avcodec: add D3D12VA hardware accelerated H264, HEVC, VP9, and AV1 decoding

[Wu Jianhua]

From: Anton Khirnov<mailto:anton at khirnov.net>
Sent: 2023年3月1日 23:52
To: FFmpeg development discussions and patches<mailto:ffmpeg-devel at ffmpeg.org>
Subject: Re: [FFmpeg-devel] [PATCH v2] avcodec: add D3D12VA hardware accelerated H264, HEVC, VP9, and AV1 decoding

Hi Anton,

>> +#ifndef AVUTIL_HWCONTEXT_D3D12VA_H
>> +#define AVUTIL_HWCONTEXT_D3D12VA_H
>> +
>> +/**
>> + * @def COBJMACROS
>> + *
>> + * @brief Enable C style interface for D3D12
>> + */
>> +#ifndef COBJMACROS
>> +#define COBJMACROS
>> +#endif
>
>Does this need to be in the public header?
>
>> +
>> +/**
>>+ * @file
>> + * An API-specific header for AV_HWDEVICE_TYPE_D3D12VA.
>> + *
>> + */
>> +#include <stdint.h>
>> +#include <initguid.h>
>> +#include <d3d12.h>
>> +#include <d3d12video.h>
>> +
>> +/**
>> + * @def DX_CHECK
>> + *
>> + * @brief A check macro used by D3D12 functions highly frequently
>> + */
>
> All identifiers declared in public headers should be AV-prefixed. Also,
> these seem like they should be in a private header instead (i.e. one
> which is not installed and not usable by our API callers).
>

Can I put COBJMACROS, and the DX_CHECK without AV-prefixed in hwcontext_d3d12va_internal.h
and used them in d3d12dec_h264/HEVC/VP9/AV1.cpp and hwcontext_d3d12va.cpp?

>> +
>> +/**
>> + * @brief Wait for a specified fence value
>> + *
>> + * The execution fence values timeline may be like:
>> + * #0 -> #1 -> #2 -> #3 ----------------> #n
>> + *             ^
>> + *             |
>> + * Call av_d3d12va_wait_for_fence_value(2) will wait #2 execution to be completed.
>> + *
>> + * @param sync_ctx
>> + * @param fence_value
>> + * @return Error code (ret < 0 if failed)
>> + */
>> +int av_d3d12va_wait_for_fence_value(AVD3D12VASyncContext *sync_ctx, uint64_t fence_value);
>
> Does this need to be public? I don't see it being used in any other
> patches. Then fence_value could also be made private.

It’s provided to the users so they can control the synchronization by themselves.
Both keeping or deleting it is fine because the user can write the codes to do the
same thing. I use it frequently when rendering video frames with D3D12 so I think
it's helpful to make it a public API. What do you think?

All the other comments are very helpful and I will try to fix them in the next patch.
Thanks for your detailed review.

Thanks,
Jianhua