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

[Wu Jianhua]

Lynne<mailto:dev at lynne.ee> wrote:
> Sent: 2022年10月14日 6:28
> To: FFmpeg development discussions and patches<mailto:ffmpeg-devel at ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH] avcodec: add D3D12VA hardware accelerated H264, HEVC, VP9, and AV1 decoding
>
> Oct 13, 2022, 17:48 by toqsxw at outlook.com:
>
>>> Lynne<mailto:dev at lynne.ee> wrote:
>>>
>>> Oct 12, 2022, 13:09 by toqsxw at outlook.com:
>>>
>>>> [PATCH] avcodec: add D3D12VA hardware accelerated H264, HEVC, VP9, and AV1 decoding
>>>>
>>>> Patch attached.
>>>>
>>> The Sync locking functions and the queue locking functions should
>>> be a function pointer in the device/frame context. Vulkan has
>>> the same issue, and that's how I did it there. This allows for
>>> API users to plug their own locking primitives in, which they need
>>> to in case they initialize their own contexts.
>>>
>>
>> I don’t need to follow your design.
>>
>
> Yes, you do, because it's not my design, it's the design of the entire
> hwcontext system. If API users cannot initialize a hwcontext with
> their own, it's breakage. It's not optional.
> Locking primitives are a part of this.
> Either fix this or this is simply not getting merged.
>

As I learn from the docs of Direct3D12, it is a multithreading-friendly API,
which means that the only mandatory field, device, set by the user can be
accessed and used without any lock. Why do the API users need locking
primitives to init the hwcontext_d3d12va? Have you got the initialization
failed? If so, could you share the runtime error details here?

And I checked the other hwcontexts, they didn't have a locking primitive also.
The hwcontext_d3d11va has a locking used to protect accesses to device_context
and video_context calls, which d3d12 doesn't need. So why the API users cannot
initialize the hwcontext with their own? Is there any real failure case? Please pardon
I don't quite understand what situation you are talking about, could you elaborate
further if I get it wrong?

>
>>> You should also document which fields API users have to set
>>> themselves if they plan to use their own context.
>>>
>>
>> Where should I document them? Doesn’t the comments enough?
>>

> In the comments. Look at how it's done elsewhere.
There is already a comment like what d3d11va wrote:
    /**
     * Device used for objects creation and access. This can also be
     * used to set the libavcodec decoding device.
     *
     * Can be set by the user. This is the only mandatory field - the other
     * device context fields are set from this and are available for convenience.
     *
     * Deallocating the AVHWDeviceContext will always release this interface,
     * and it does not matter whether it was user-allocated.
     */
    ID3D12Device *device;

Is there anything else missing?

>>> Also, struct names in the public context lack an AV prefix.
>>>
>> Will fix. And which struct? Could you add the reference?
>>
>
> In the main public context.
>
I check the codes and found that AVD3D12VASyncContext, AVD3D12VADeviceContext,
AVD3D12FrameDescriptor and AVD3D12VAFramesContext are the structs in
the hwcontext_d3d12va. Is there any other struct without AV prefix? Could you
paste the name here?

>>> D3D12VA_MAX_SURFACES is a terrible hack. Vendors should
>>> fix their own drivers rather than users running out of memory.
>>>
>>
>> Not my responsibility as a personal developer. I know nothing
>> about the drivers. You can ask those vendors to fix them. I don’t
>> think it’s a `terrible hack`. On my test, The MAX_SURFACES is
>> enough for the decoder. If there are any docs or the drivers fixed
>> it, just simply remove it. Why user will run out of memory?
>>
>
> The whole way the hwcontext is designed is sketchy. Why are
> you keeping all texture information in an array (texture_infos)?Frames are already pooled (it's called AVFramePool), so you're
> not doing anything by adding a second layer on top of this other
> than complexity.
> The initial_pool_size parameter was a hack added to support
> hwcontexts which cannot allocate surfaces dynamically, you don't
> need to support this at all, you can just let users allocate
> frames as they need to rather than preinitializing.
>

It’s the same implementation as d3d11va. The static initial_pool_size is
needed by the decoder heap to initialize and the input stream needs it
as well The feature that allows the user to allocate frames is not the basic
functionalities of decoding. I recommend the user who needs the feature
implement it and contribute the codes.

>>> Also, you have code style issues, don't wrap one-line if statements
>>> or loops in brackets.
>>>
>> Will fix. And which loop? Could you add the reference?
>>
>>> ff_d3d12dec_get_suitable_max_bitstream_size is an awful function.
>>> It does float math for sizes and has a magic mult factor of 1.5.
>>> You have to calculate this properly.
>>>
>> It simply calculate the size of NV12 and P010. Will add comment.
>>
>
>Do it _properly_. We have utilities for it. If not, multiplication by 1.5
>is val + (val >> 1).
>
>>> On a first look, this is what stands out. Really must be split apart
>>> in patches.
>>>
>>
>> Already claim that I will split it.
>>