Video Encode/Decode#226
Conversation
|
Wow! Magnificent! Thanks a ton for bringing this work to life! I will try to review later today. |
Should
Should we add more formats? We should invent better names :) DXGI has short names, but they are unclear (seems to be a good start):
No D3D11 support? Probably not needed, but, IM, nice to have :) (...to be continued) |
|
Q1 Q2: Q3 |
+1, clarifying comments may be added if needed. They are pretty standard.
No objections here (assuming the interface is implementable in D3D11) |
|
Do we need other formats, like |
|
Please, add this link somewhere in the beginning of |
|
I'm ready to merge it after you are done with polishing. Just let me know. Further improvements can be made in I haven't deeply analyzed the implementation, but the API additions are nice and clear. Also I think that these formats: can be removed and |
Definitely. I'll look into it as soon as I can.
I like that. |
The current set targets the common 4:2:0 format family needed by the decode/encode path: NV12, P010, P016. Y410 is packed 4:4:4 and would add support for it in a follow-up if the need arises. |
|
I don't like that you're using native API structures in NRI public API. Wouldn't be logical to implemented it as a translation layer, not in the NRIWrapper* way? |
- "TextureViewDesc::readonlyPlanes" renamed to "planes". Logic switched to "direct" from "inverse"
- removed special depth-stencil format for shader resource views ("R24_UNORM_X8", "X24_G8_UINT", "R32_SFLOAT_X8_X24" and "X32_G8_UINT_X24"). Use "planes" instead
- explained valid usage
OTHER:
- VK: untangled "planes" (aspect mask) logic to be friendly for subpass inputs
- prerequisite for video support (PR #226)
@Daedie-git I have implemented it. Please, update your code to the latest since it's a prerequisite for the video support. Please, note that I have removed PR #222 since the code is completely different for these lines. Feel free to adjust, re-add or discuss. Thanks in advance! We are moving forward!
Just echoing already said, I absolutely agree with this. GAPI specific structs must be moved to corresponding |
Indeed. I have addressed this locally already. I'll rebase it on your change and push it soon. |
…ueues # Conflicts: # Source/VK/DescriptorVK.hpp
437decc to
4a2960d
Compare
|
I wrapped up my changes. |
|
I found some issues, so I will follow up with another commit. |
|
Additional question: For the sake of testing this, I've been building up quite the test suite in my own repo. Do you want to have it in some form? |
|
Just compared prev with the latest - it's getting better! FEEDBACK: One more thing (just spotted): if (isVideoDecode || isVideoEncode) {
if (m_Desc.videoCodec == VideoCodec::NONE)
return Result::INVALID_ARGUMENT;
...
} This check (and all similar) needs to be moved to |
|
I think I have addressed all your feedback so far |
|
Thanks. Apologies, I was busy with other stuff. I will review tomorrow. |
|
Quick stuff: FEEDBACK:
|
|
Hi, I committed some changes to bring the supported feature set of both backends in line. There were some things that were accepted by one back-end that weren't by the other. Next I will address your latest feedback. |
|
Thanks for the changes. FEEDBACK: I asked to change year only in the newly added header. Not everywhere :) The year of publication must be there. I personally prefer to not use "range" to avoid the noise in diffs. Please, revert all related changes in the already existing files. |
|
FEEDBACK:
const void* vkDecodeInfo; // VkVideoDecodeInfoKHR*
NriOptional const NriPtr(VideoH265VideoParameterSetDesc) videoParameterSets; // if provided, must include "videoParameterSetNum" entries
uint32_t videoParameterSetNum;to: const NriPtr(VideoH265VideoParameterSetDesc) videoParameterSets;
uint32_t videoParameterSetNum;
NriOptional const uint16_t* miColumnStarts; // if provided, must include "columnNum + 1" entries
NriOptional const uint16_t* miRowStarts; // if provided, must include "rowNum + 1" entries
NriOptional const uint16_t* widthInSuperblocksMinus1; // if provided, must include "columnNum" entries
NriOptional const uint16_t* heightInSuperblocksMinus1; // if provided, must include "rowNum" entries
(OPTIONAL) Gemini suggests this: (OPTIONAL) Gemini suggests this: |
|
AI review (AI vs AI? ^_^) I think it makes sense to post, since it looks legit. I have inlined my comments: Functional Improvements
Technical Refinements
Synchronization Logic[ME: just to keep in mind, all good here]
|
|
Thank you. I will review the latest today. |
|
Much better! But a bit more iterations are needed: FEEDBACK:
NriForwardStruct(VideoSession);
NriForwardStruct(VideoSessionParameters);
NriForwardStruct(VideoPicture);into corresponding
if (SUCCEEDED(hr)) {
m_Desc.shaderStage.compute.dispatchMaxDim[0] = options22.Max1DDispatchSize;
m_Desc.shaderStage.task.dispatchMaxDim[0] = options22.Max1DDispatchMeshSize;
}original (I like it better, it matches 21 other of such blocks): if (FAILED(hr))
NRI_REPORT_WARNING(this, "ID3D12Device::CheckFeatureSupport(options22) failed, result = 0x%08X!", hr);
m_Desc.shaderStage.compute.dispatchMaxDim[0] = options22.Max1DDispatchSize;
m_Desc.shaderStage.task.dispatchMaxDim[0] = options22.Max1DDispatchMeshSize;I suspect there maybe more such blocks, I think AI is capable to compare with the original and fix all the cases. I see some video-unrelated changes, which probably makes sense. Ask LLM to log such cases and explain the reasoning. It's an optional request |
|
I'm really happy that you have been working on this. And I will accept the code. Please, don't give up! FEEDBACK: This I found myself, but asked AI to type for me to save a bit of my time ;) This is messy (with all dependencies): using CommandList = std::variant<ComPtr<ID3D12GraphicsCommandListBest>, ComPtr<ID3D12VideoDecodeCommandList>, ComPtr<ID3D12VideoEncodeCommandList>>;
ID3D12CommandList* GetCommandList() const;SUGGESTION: Rewrite the code with all dependencies as: // Change member from std::variant to base ComPtr
private:
ComPtr<ID3D12CommandList> m_CommandList;
// Replace existing getters with type-specific static_casts
private:
inline ID3D12GraphicsCommandListBest* GetGraphicsCommandList() const {
NRI_CHECK(
m_CommandListType == D3D12_COMMAND_LIST_TYPE_DIRECT ||
m_CommandListType == D3D12_COMMAND_LIST_TYPE_COMPUTE ||
m_CommandListType == D3D12_COMMAND_LIST_TYPE_COPY
);
return static_cast<ID3D12GraphicsCommandListBest*>(m_CommandList.Get());
}
inline ID3D12VideoDecodeCommandList* GetVideoDecodeCommandList() const {
NRI_CHECK(m_CommandListType == D3D12_COMMAND_LIST_TYPE_VIDEO_DECODE);
return static_cast<ID3D12VideoDecodeCommandList*>(m_CommandList.Get());
}
inline ID3D12VideoEncodeCommandList* GetVideoEncodeCommandList() const {
NRI_CHECK(m_CommandListType == D3D12_COMMAND_LIST_TYPE_VIDEO_ENCODE);
return static_cast<ID3D12VideoEncodeCommandList*>(m_CommandList.Get());
}and fix usages like: void CommandBufferD3D12::ResourceBarrier(uint32_t barrierNum, const D3D12_RESOURCE_BARRIER* barriers) const {
if (m_CommandListType == D3D12_COMMAND_LIST_TYPE_VIDEO_DECODE)
GetVideoDecodeCommandList()->ResourceBarrier(barrierNum, barriers);
else if (m_CommandListType == D3D12_COMMAND_LIST_TYPE_VIDEO_ENCODE)
GetVideoEncodeCommandList()->ResourceBarrier(barrierNum, barriers);
else
GetGraphicsCommandList()->ResourceBarrier(barrierNum, barriers);
}and remove |
|
FEEDBACK:
NriBits(VideoEncodeBits, uint8_t,
NONE = 0,
// Forces the current frame to be an IDR (H.26x) or Key Frame (AV1)
// Useful for scene cuts or starting a new stream segment
FORCE_KEY_FRAME = NriBit(0),
// Signals that this is the last frame of the sequence
// The hardware will flush its internal pipeline and write any pending data
END_OF_STREAM = NriBit(1)
);add |
|
I will follow up with more changes soon. |
The open encode/decode enticed me to try and get VK hardware decoding working in my software (until now, I only had it on the D3D12 side). I expect this will need further changes, just let me know.
I wasn't sure what the right way to handle the D3D12 CommandBuffer was, I settled on the variant for now.
Summary
Adds hardware video queue support and a minimal native-backed video encode/decode extension API.
This branch makes video-capable queues visible through NRI, allows D3D12/Vulkan video command buffers to be created and submitted, adds
basic multi-planar video format support, and exposes
NRIVideoentry points for issuing native encode/decode commands.Changes
Public API
QueueType::VIDEO_DECODEQueueType::VIDEO_ENCODEG8_B8R8_2PLANE_420_UNORMG10X6_B10X6R10X6_2PLANE_420_UNORM_3PACK16G16_B16R16_2PLANE_420_UNORMPLANE_0PLANE_1PLANE_2Include/Extensions/NRIVideo.hVideoInterfaceCmdDecodeVideoCmdEncodeVideoAdapter and Queue Discovery
ID3D12VideoDeviceis availableVkQueueFamilyVideoPropertiesKHRto ensure Vulkan video queues also expose compatible codec operationsD3D12 Backend
D3D12_COMMAND_LIST_TYPE_VIDEO_DECODED3D12_COMMAND_LIST_TYPE_VIDEO_ENCODECommandBufferD3D12to hold graphics, video decode, or video encode command listsCmdDecodeVideoviaID3D12VideoDecodeCommandList::DecodeFrameCmdEncodeVideoviaID3D12VideoEncodeCommandList2::EncodeFrameID3D12CommandList*, since video command lists are not graphics command listsVulkan Backend
VK_KHR_video_queueVK_KHR_video_decode_queueVK_KHR_video_encode_queuevkCmdDecodeVideoKHRandvkCmdEncodeVideoKHRCmdDecodeVideo/CmdEncodeVideoforwarding to Vulkan command buffersValidation / Interface Wiring
VideoInterfacethroughnriGetInterfaceDeviceBase::FillFunctionTable(VideoInterface&)FillFunctionTable(VideoInterface&)implementationsAPI Shape
The new video API is intentionally small and native-backed.
NRI handles queue discovery, command buffer ownership, function table exposure, and command dispatch. Codec/session setup remains
backend-native (initially):
VkVideoDecodeInfoKHR/VkVideoEncodeInfoKHRpayloadsValidation
Built successfully with: