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

[Wu Jianhua]

James Almer<mailto:jamrial at gmail.com> wrote:
> On 10/13/2022 12:48 PM, Wu Jianhua wrote:
>>> 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.
>>
>>> 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?
>>
>>> Also, struct names in the public context lack an AV prefix.
>> Will fix. And which struct? Could you add the reference?
>>
>>> 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?
>>
>>> 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.
>
> Then you should probably use imgutils.h functions for that, and/or
> AVPixFmtDescriptor from pixdesc.h.

Great! Really thanks for the details. I will try to take a look at how to do that better.