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

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

[Wu Jianhua]

[-- Attachment #1: Type: text/plain, Size: 108 bytes --]

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

Patch attached.



[-- Attachment #2: 0001-avcodec-add-D3D12VA-hardware-accelerated-H264-HEVC-V.patch --]
[-- Type: application/octet-stream, Size: 145028 bytes --]

[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

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

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

[Jean-Baptiste Kempf]

Hello,

You really need to split this patch into logical patches.

jb

On Wed, 12 Oct 2022, at 13:09, Wu Jianhua wrote:
> [PATCH] avcodec: add D3D12VA hardware accelerated H264, HEVC, VP9, and 
> AV1 decoding
>
> Patch attached.
>
>
>
> _______________________________________________
> 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".
>
> Attachments:
> * 0001-avcodec-add-D3D12VA-hardware-accelerated-H264-HEVC-V.patch

-- 
Jean-Baptiste Kempf -  President
+33 672 704 734
_______________________________________________
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".

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

[James Almer]

On 10/12/2022 12:12 PM, Jean-Baptiste Kempf wrote:
> Hello,
> 
> You really need to split this patch into logical patches.
> 
> jb

To expand on this, you should split into the following: One patch adding 
the hwcontext and pixel format to libavutil plus an entry in the 
APIChanges file mentioning the new public symbols and defines, then one 
patch per new hwaccel in libavcodec, the first of which should contain 
the shared code, and the last one the Changelog entry and the version 
bump (The changes in configure should of course be split across said 
patches).

Compilation must succeed for every patch, hence the above suggested order.

> 
> On Wed, 12 Oct 2022, at 13:09, Wu Jianhua wrote:
>> [PATCH] avcodec: add D3D12VA hardware accelerated H264, HEVC, VP9, and
>> AV1 decoding
>>
>> Patch attached.
>>
>>
>>
>> _______________________________________________
>> 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".
>>
>> Attachments:
>> * 0001-avcodec-add-D3D12VA-hardware-accelerated-H264-HEVC-V.patch
> 
_______________________________________________
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".

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

[Wu Jianhua]

James Almer<mailto:jamrial@gmail.com> wrote:
> On 10/12/2022 12:12 PM, Jean-Baptiste Kempf wrote:
>> Hello,
>>
>> You really need to split this patch into logical patches.
>>
>> jb
>
> To expand on this, you should split into the following: One patch adding
> the hwcontext and pixel format to libavutil plus an entry in the
> APIChanges file mentioning the new public symbols and defines, then one
> patch per new hwaccel in libavcodec, the first of which should contain
> the shared code, and the last one the Changelog entry and the version
> bump (The changes in configure should of course be split across said
> patches).
>
> Compilation must succeed for every patch, hence the above suggested order.
>

I’ll try to do that. Thanks for the advice.

>>
>> On Wed, 12 Oct 2022, at 13:09, Wu Jianhua wrote:
>>> [PATCH] avcodec: add D3D12VA hardware accelerated H264, HEVC, VP9, and
>>> AV1 decoding
>>>
>>> Patch attached.
>>>
>>>


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

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

[Lynne]

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.

You should also document which fields API users have to set
themselves if they plan to use their own context.

Also, struct names in the public context lack an AV prefix.

D3D12VA_MAX_SURFACES is a terrible hack. Vendors should
fix their own drivers rather than users running out of memory.

Also, you have code style issues, don't wrap one-line if statements
or loops in brackets.

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.

On a first look, this is what stands out. Really must be split apart
in patches.
_______________________________________________
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".

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

[Wu Jianhua]

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

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

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

[James Almer]

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

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

[Wu Jianhua]

James Almer<mailto:jamrial@gmail.com> wrote:
> 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.

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


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

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

[Lynne]

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.


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

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

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

[Wu Jianhua]

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?

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

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


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

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

[Lynne]

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

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

[Wu Jianhua]

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

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

[Wu Jianhua]

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