[PATCH] avcodec: add D3D12VA hardware accelerated H264, HEVC, VP9, and AV1 decoding
[PATCH] avcodec: add D3D12VA hardware accelerated H264, HEVC, VP9, and AV1 decoding Patch attached.
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
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
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.
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.
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.
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".
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.
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.
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.
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?
________________________________ 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".
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.
participants (4)
-
James Almer -
Jean-Baptiste Kempf -
Lynne -
Wu Jianhua