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

[Lynne]

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.


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


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


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


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