encode: fixes related to pNext-chaining#194
Open
srinathkr-nv wants to merge 5 commits intoKhronosGroup:mainfrom
Open
encode: fixes related to pNext-chaining#194srinathkr-nv wants to merge 5 commits intoKhronosGroup:mainfrom
srinathkr-nv wants to merge 5 commits intoKhronosGroup:mainfrom
Conversation
|
|
Contributor
Author
|
@dabrain34 @lolzballs please review these changes. |
srinathkr-nv
changed the title
dabrain34
requested changes
Apr 21, 2026
lolzballs
reviewed
Apr 21, 2026
srinathkr-nv
force-pushed
the
pnext-chaining-fixes
branch
from
44a1cf5 to
7ad6cb2
Compare
Contributor
|
can you add a guard as mention in the thread, rebase, fix the buildbot with reset ? |
lolzballs
approved these changes
Apr 22, 2026
srinathkr-nv
force-pushed
the
pnext-chaining-fixes
branch
from
7ad6cb2 to
3f71be2
Compare
Contributor
Author
I don't know why the build bot decided to complain about the |
Commit 62c184c changed line 1807 in VkVideoEncoder.cpp from ((VkBaseInStructure*)(m_beginRateControlInfo.pNext))->pNext = NULL; to: m_beginRateControlInfo.pNext = (VkBaseInStructure*)(&encodeFrameInfo->pControlCmdChain); , introducing undefined behaviour because &encodeFrameInfo->pControlCmdChain is the address of the pointer member itself —- a VkBaseInStructure** and not a Vulkan structure. Commit 1c603c1 fixed the undefined behaviour by setting the RHS to NULL but went one step too far, because that also ended up removing any chained rate control structures. This change fully reverts 1c603c1 and restores the contents on line 1807 to its pre-62c184cc form, albeit written a bit differently to fix the following warning from GCC: cast from type ‘const void*’ to type ‘VkBaseInStructure*’ casts away qualifiers [-Wcast-qual] Signed-off-by: Tymur Boiko <tboiko@nvidia.com>
This method currently assumes that the first element in the pNext chain of VkVideoEncodeInfoKHR will be a codec-specific structure for the picture info (e.g. VkVideoEncodeH264PictureInfoKHR). This assumption is problematic because: - it complicates the pNext chaining logic for other places in the code which need to insert one or more structures into the pNext chain for VkVideoEncodeInfoKHR. - making assumptions within the application about the order of structures in the pNext chain, although not prohibited by the Vulkan specification, may lead to similar assumptions elsewhere and complicate code writing and comprehension. This method aids VkVideoEncoderH264::GetEncodeFrameInfoH264() and similar methods in the other codec-specific classes in checking that the pointer to the base class points in fact to the actual codec-specific data structure needed, instead of relying on runtime type information (RTTI). This purpose is now fulfilled by changing GetType() into a virtual method which returns the codec operation, and which is overridden by derived classes. Additionally, GetType() has been renamed to GetCodecType() for clarity. Signed-off-by: Srinath Kumarapuram <skr@nvidia.com>
Prior to this commit, the code in VkVideoEncodeFrameInfo::Reset() was
written with the assumption that the first element in the pNext chain of
VkVideoEncodeInfoKHR would be a codec-specific structure describing the
picture info (e.g. VkVideoEncodeH264PictureInfoKHR). Correspondingly,
the overridden versions of ::Reset() in the derived classes would simply
do:
pictureInfo.pNext = nullptr;
to clear the pNext chain.
Commit 16607feb9b86984b53e1747332c1ca386d7ecd2d regressed this by
changing the pNext chaining behaviour in the ChainNextVkStruct() helper
to add structures at the head of the pNext chain rather than at the
tail. This meant that the codec-specific picture info structure was not
guaranteed to be the first structure in the pNext chain of
VkVideoEncodeInfoKHR, so the above assignment was not sufficient to
clear the pNext chain.
This change updates VkVideoEncodeFrameInfo::Reset() to clear the pNext
of encodeInfo (i.e. VkVideoEncodeInfoKHR) and then have it point to
codec-specific picture info structure in the overridden ::Reset()
provided by the derived class. This change also restores the
functionality of quantization maps which was broken by commit
16607feb9b86984b53e1747332c1ca386d7ecd2d.
Signed-off-by: Srinath Kumarapuram <skr@nvidia.com>
Previously, any code adding a structure to the pNext chain of VkVideoEncodeInfoKHR had to be careful to preserve a codec-specific picture as the first element in this chain, or restore it to that state as soon as possible, because code elsewhere assumed it or depended on it for correct operation. With the previous two commits, such assumptions have been eliminated, making pNext chaining for VkVideoEncodeInfoKHR much easier. Signed-off-by: Srinath Kumarapuram <skr@nvidia.com>
The VkVideoEncodeFrameInfo struct contains a Reset() virtual method. Codec-specific structs derived from VkVideoEncodeFrameInfo contain a corresponding Reset() method which overrides VkVideoEncodeFrameInfo::Reset(), but the 'override' keyword was missing from the definitions of these overriding methods, which can cause compilers to complain about missing or inconsistent method overrides.
srinathkr-nv
force-pushed
the
pnext-chaining-fixes
branch
from
3f71be2 to
ac910d3
Compare
dabrain34
approved these changes
Apr 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This change fixes a regression arising from 62c184c which prevented the quantization maps functionality from working correctly and also regressed RC struct chaining code.
Type of change
Bug fix (and associated minor cleanup)
Issue (optional)
See #53 (comment)
Tests
NVIDIA L2 / NVIDIA 595.44.05 / Ubuntu 24.04.4 LTS
Total Tests: 73
Passed: 62
Crashed: 0
Failed: 0
Not Supported: 9
Skipped: 2 (in skip list)
Success Rate: 100.0%