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

[Wu Jianhua <toqsxw@outlook.com>]

________________________________
From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> on behalf of Lynne <dev@lynne.ee>
Sent: Saturday, October 15, 2022 1:16:18 PM
To: FFmpeg development discussions and patches <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.


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

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