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

[Wu Jianhua <toqsxw@outlook.com>]

> From: Lynne<mailto:dev@lynne.ee>
> Sent: 2022年10月15日 13:16
> To: FFmpeg development discussions and patches<mailto:ffmpeg-devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH] avcodec: add D3D12VA hardware accelerated H264, HEVC, VP9, and AV1 decoding
>
>Oct 14, 2022, 14:32 by toqsxw@outlook.com:
>
>> Lynne<mailto:dev@lynne.ee> wrote:
>>
>>> Sent: 2022年10月14日 6:28
>>> To: FFmpeg development discussions and patches<mailto:ffmpeg-devel@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@outlook.com:
>>>
>>>>> 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.
>>>>
>>>
>>> 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?
>>
>
> What about the frames context? Frame contexts are also user-settable.

Yeah. The comment is missing. I will add them. Thanks.

>>>>> 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.
>>
>
> It'll limit the hwcontext to just decoding, but whatever.
>
> Are there any devices that don't support Vulkan but support d3d12?

Maybe those device only care about one thing, like a games console or something. Just a guess.

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