Vassili's compilation fixes by zlatinski · Pull Request #145 · nvpro-samples/vk_video_samples

zlatinski · 2025-06-09T14:27:28Z

common: Code compilation fixes
common: SAFE_EXIT macro to print and exit
common: Use the CRC generator instead of embedding it
encoder: Deal with the Vulkan chained stuctures
Fix frame to file from adding .yuv to filenames with .y4m already

@vasn1k @srinathkr-nv please review.

vasn1k · 2025-06-09T19:24:50Z

+
+inline void safe_exit(int status, const char* function_name, int line_number, const char* description) {
+    printf("Exiting from %s:%d - %s (status: %d)\n", function_name, line_number, description, status);
+    exit(status);


Using an additional file was done so implementations of this code could leave it out and easily define their own safe_exit function. Defining safe_exit in the header, requires additional MACRO's to go around the exit call, preventing it from being compiled in. So there we would need the BUILD_DECODER_APP_AS_LIB check which will switch between exit and printf.

I kind of agree with @vasn1k here regarding the separate .cpp file, although one other header-only approach is for other uses of this code to provide their own ExitWrapper.h with a different definition of safe_exit().

Per our discussion, this morning. Lets get rid of ExitWrapper and convert all exit() calls into returns.
I'll look into compiler settings to disallow exit() entirely later.

vasn1k

There are a few minor things:

Exit wrapper - needs the cpp file or an abstract overloadable interface. The cpp file is a lot simpler.

Another thing that seems to have been missed is: VulkanVideoProcessor::GetVideoExtent()
Where the extent attempts to call a constructor with an object present. But extent is a structure without a constructor. This causes a compilation error: /VulkanVideoProcessor.cpp(221): error C2100: illegal indirection
Same for: VulkanVideoProcessor::GetVkProfile

And the opposite for: VkVideoCoreProfile videoProfile(m_videoStreamDemuxer->GetVideoCodec()

safe_exits in: vk_video_encoder/libs/VkVideoEncoder/VkEncoderDpbH264.cpp
are missing.

So we don't forget, below changes have been dropped but are necessary:
cpudetect changes are missing.
m_crcOutput is missing, important.
REDUCE_VK_VIDEO_STDOUT_SPEW is missing.

zlatinski · 2025-06-10T14:15:17Z

Another thing that seems to have been missed is: VulkanVideoProcessor::GetVideoExtent() Where the extent attempts to call a constructor with an object present. But extent is a structure without a constructor. This causes a compilation error: /VulkanVideoProcessor.cpp(221): error C2100: illegal indirection Same for: VulkanVideoProcessor::GetVkProfile

And the opposite for: VkVideoCoreProfile videoProfile(m_videoStreamDemuxer->GetVideoCodec()

Sorry, fixed with the latest change.

safe_exits in: vk_video_encoder/libs/VkVideoEncoder/VkEncoderDpbH264.cpp are missing.

I see all the exit functions in place, including the ones in VkEncoderDpbH264.cpp

The rest of the changes should go with a different change.

zlatinski · 2025-06-10T14:20:06Z

cpudetect changes are missing.

Yes, I've included that one two.

zlatinski · 2025-06-10T14:23:24Z

So we don't forget, below changes have been dropped but are necessary:

m_crcOutput is missing, important.

This one needs a more robust change using a local vector that the client would request to copy from.

REDUCE_VK_VIDEO_STDOUT_SPEW is missing.

Can we rename this one to VK_VIDEO_NO_STDOUT_INFO?

vasn1k · 2025-06-10T15:38:16Z

(m_crcOutput) This one needs a more robust change using a local vector that the client would request to copy from.

I think this boils down to preference, my preference was caller allocation and management. Your voiced preference is lib ownership, allocation and management, with a potential needed copy. It's really just a vector<uint32_t>. I'm not extremely picky on how it happens, it will just be more changes. On both sides.

Can we rename this one to VK_VIDEO_NO_STDOUT_INFO?

Sounds good to me.

What about: #if !defined(DISABLE_VK_VIDEO_PARSER_SIMD_OPTIMIZATIONS)?

It would be good to get ExitWrapper.cpp back. That way we get all flexibility. And you want the implementation of the function in cpp, otherwise the pre-processor will copy it at every include.

srinathkr-nv · 2025-06-11T12:47:05Z

+// Returns true if the caller would need to handle the exit
+// Otherwise, the function will call exit() that would exit the process.
+inline bool safe_exit(int status, const char* function_name, int line_number, const char* description) {
+    printf("Exiting with a critical error from %s:%d - %s (status: %d)\n", function_name, line_number, description, status);


I find the "with a critical error" part problematic -- we have cases where the call site is safe_exit(EXIT_SUCCESS, ...).

srinathkr-nv · 2025-06-11T12:49:00Z

+
+inline void safe_exit(int status, const char* function_name, int line_number, const char* description) {
+    printf("Exiting from %s:%d - %s (status: %d)\n", function_name, line_number, description, status);
+    exit(status);


I kind of agree with @vasn1k here regarding the separate .cpp file, although one other header-only approach is for other uses of this code to provide their own ExitWrapper.h with a different definition of safe_exit().

srinathkr-nv · 2025-06-11T12:50:02Z

+
+// Returns true if the caller would need to handle the exit
+// Otherwise, the function will call exit() that would exit the process.
+inline bool safe_exit(int status, const char* function_name, int line_number, const char* description) {


Why does this return bool ? void is perfectly good because no caller checks for the return value from SAFE_EXIT().

srinathkr-nv · 2025-06-12T05:07:20Z

            if (m_currDpbIdx >= MAX_DPB_SLOTS) {
                VK_DPB_DBG_PRINT(("could not allocate a frame buffer\n"));
-                exit(1);
+                SAFE_EXIT(1, "Failed to allocate frame buffer - DPB slots exhausted");


For the elimination of exit() from the code, this entire block (lines 280 - 283) can be deleted. We will not have a case where m_currDpbIdx >= MAX_DPB_SLOTS because if it were true, we would have not come out of the while (IsDpbFull()) loop above. It's probably sufficient to add a comment here stating this while deleting this code.

srinathkr-nv · 2025-06-12T05:11:28Z

                    if (m_currDpbIdx >= MAX_DPB_SLOTS) {
                        VK_DPB_DBG_PRINT(("could not allocate a frame buffer\n"));
-                        exit(1);
+                        SAFE_EXIT(1, "Failed to allocate frame buffer - DPB slots exhausted");


This block of code in lines 357 - 360 can be deleted because this is the else branch of an if-else, where the DBP is not full, so the above for loop (lines 354 - 356) will find an empty entry. Just like I wrote in the previous comment, it's probably sufficient to leave a comment here stating this while deleting this code (which will eliminate the exit()).

Signed-off-by: Vassili Nikolaev (NVIDIA) <vnikolaev@nvidia.com>

zlatinski · 2025-06-12T10:02:55Z

Updated the change as agreed, removing the exit code.

srinathkr-nv · 2025-06-12T10:36:26Z

The latest changes (as of commit e514b85) LGTM.

vasn1k · 2025-06-12T15:20:32Z

I thought you were going to replace the exits with returns, not just revert?

Please also add VkVideoFrameOutput::GetCrcValues() and remove the dependency on supplying a crc output file. Facilitating initCRCValue should force the CRC to be calculated and output into the crcAllocation, it should avoid printing.

I still don't see an update for: #if !defined(DISABLE_VK_VIDEO_PARSER_SIMD_OPTIMIZATIONS)

Where is VK_VIDEO_NO_STDOUT_INFO?

I've created an MR into this branch to add the missing pieces:
#146

zlatinski · 2025-06-16T13:21:34Z

I'm merging these basic changes and compiler fixes so far. There are going to be follow-up changes from #140

vasn1k reviewed Jun 9, 2025

View reviewed changes

Comment thread common/libs/VkCodecUtils/VkVideoFrameToFile.cpp Outdated

vasn1k suggested changes Jun 9, 2025

View reviewed changes

zlatinski force-pushed the vassili-compilation-fixes branch from da3950d to 2baf0d3 Compare June 10, 2025 14:01

common: Code compilation fixes

d80ce11

zlatinski force-pushed the vassili-compilation-fixes branch from 2baf0d3 to e4f3145 Compare June 10, 2025 14:19

zlatinski force-pushed the vassili-compilation-fixes branch from e4f3145 to 7a4db15 Compare June 11, 2025 08:48

srinathkr-nv reviewed Jun 11, 2025

View reviewed changes

srinathkr-nv reviewed Jun 12, 2025

View reviewed changes

vasn1k added 3 commits June 12, 2025 05:01

common: Use the CRC generator instead of embedding it

1b798d5

encoder: Deal with the Vulkan chained stuctures

3dfa56d

Fix frame to file from adding .yuv to filenames with .y4m already

e514b85

Signed-off-by: Vassili Nikolaev (NVIDIA) <vnikolaev@nvidia.com>

zlatinski force-pushed the vassili-compilation-fixes branch from 7a4db15 to e514b85 Compare June 12, 2025 10:01

zlatinski merged commit 001c2ff into main Jun 16, 2025
4 of 5 checks passed

dabrain34 mentioned this pull request Jun 25, 2025

Backport: integrate vk video samples from NVPro workspace, part 2 KhronosGroup/Vulkan-Video-Samples#53

Merged

zlatinski deleted the vassili-compilation-fixes branch October 10, 2025 19:37

Conversation

zlatinski commented Jun 9, 2025

Uh oh!

vasn1k Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srinathkr-nv Jun 11, 2025

Choose a reason for hiding this comment

Uh oh!

vasn1k Jun 11, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vasn1k left a comment

Choose a reason for hiding this comment

Uh oh!

zlatinski commented Jun 10, 2025

Uh oh!

zlatinski commented Jun 10, 2025

Uh oh!

zlatinski commented Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vasn1k commented Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

srinathkr-nv Jun 11, 2025

Choose a reason for hiding this comment

Uh oh!

srinathkr-nv Jun 11, 2025

Choose a reason for hiding this comment

Uh oh!

srinathkr-nv Jun 11, 2025

Choose a reason for hiding this comment

Uh oh!

srinathkr-nv Jun 12, 2025

Choose a reason for hiding this comment

Uh oh!

srinathkr-nv Jun 12, 2025

Choose a reason for hiding this comment

Uh oh!

zlatinski commented Jun 12, 2025

Uh oh!

srinathkr-nv commented Jun 12, 2025

Uh oh!

vasn1k commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zlatinski commented Jun 16, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vasn1k Jun 9, 2025 •

edited

Loading

zlatinski commented Jun 10, 2025 •

edited

Loading

vasn1k commented Jun 10, 2025 •

edited

Loading

vasn1k commented Jun 12, 2025 •

edited

Loading