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

[James Almer <jamrial@gmail.com>]

On 10/13/2022 12:48 PM, Wu Jianhua wrote:
>> Lynne<mailto:dev@lynne.ee> wrote:
> 
>> Oct 12, 2022, 13:09 by toqsxw@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.

> 
>> On a first look, this is what stands out. Really must be split apart
>> in patches.
> 
> Already claim that I will split it.
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".