build: add -wconversion and fix the related conversion issues by dabrain34 · Pull Request #7 · KhronosGroup/Vulkan-Video-Samples

dabrain34 · 2024-12-17T15:37:23Z

Description

Fix the conversion issues using -Wconversion during the build. Usefull for CTS.

Type of change

bug fix

Issue (optional)

Tests

Intel(R) Iris(R) Xe Graphics (ADL GT2) / Intel open-source Mesa driver Mesa 26.2.0-devel (git-e6064cf077) / Ubuntu 24.04.4 LTS

Total Tests: 77
Passed: 52
Crashed: 0
Failed: 0
Not Supported: 16
Skipped: 9 (in skip list)
Success Rate: 100.0%

Additional Details (optional)

dabrain34 · 2024-12-17T17:54:56Z

@zlatinski Can you give a glance to this PR. It allows to avoid non regression when we change something in vulkan-video-samples. I had the issue today again with CTS bots failing because of Vulkan-Video-Samples.

This is quite a big change but necessary.

zlatinski

Thank you, Stephane, for these changes. These look good in general, but I would like to request a bit more improvements. If you want, you can change the VP9 types to concrete, but you don't have to at this point because we are updating the parser based on the latest Vulkan VP9 spec, anyway.

zlatinski

Thank you, Stephane. Your change has exposed some issues, which are indeed good to address. Thank you for doing that! Unfortunately, there are more fixes needed :( sorry!

dabrain34 · 2025-03-18T11:06:30Z

@zlatinski are you okay to merge this change ?

dabrain34 · 2025-03-28T17:35:50Z

ping @zlatinski

zlatinski

Thank you for the fixes, Stephane! I'm fine with the changes, as long as after applying the changes, the Fluster's results don't get regressed.

dabrain34 · 2025-05-14T15:17:08Z

Found some regression with Fluster AV1 tests that have been addressed.

Having a bot to check that would be great but thanks for asking to check that again.

Unfortunately the bot is unhappy now because of spirv. Need to investigate whats happening ...

dabrain34 · 2025-08-20T16:03:00Z

Just pushed a new serie to fix the latest conversion issues.

I gave a try to fluster AV1, H264, H265, VP9 and so far no regressions.

Would request anyway another review to be sure about it.

Due to -Wconversion, a lot of conversion have been discovered.

As -1 is not shaderc_shader_kind, use the default one as shaderc_glsl_infer_from_source. Conversion issue.

clang does not support string to boolean conversation in the assert.

crmurillo · 2026-05-22T13:46:01Z

    size_t i = 0;
    {
-        static const int lanes = (int)svcntb();
+        static const uint8_t lanes = (uint8_t)svcntb();


SVE vector length is hardware-dependent, using uint8_t for svcntb() is unsafe because implementations could expose vector lengths larger than 255 bytes.

crmurillo · 2026-05-26T14:51:18Z

 inline int VulkanAV1Decoder::ReadDeltaQ(uint32_t bits)
 {
-    return u(1) ? ReadSignedBits(bits) : 0;
+    return u(1) ? (uint8_t)ReadSignedBits(bits) : 0;


I would drop the (uint8_t). ReadDeltaQ return type is int.
In line 1314, where ReadDeltaQ is called, the return value is converted to int8_t.
The -Wconversion warning should already be silenced by the explicit (int8_t) in line 1314 and other places where the function is called.

crmurillo · 2026-05-26T15:06:24Z

+#define VKVS_DEBUG_TRAP() abort()
+#endif
+
+#define VKVS_ASSERT(cond, fmt, ...) \


This changes the behavior of release builds. Now if the assertion is triggered, it always writes to stderr, only the abort() is conditional.

crmurillo · 2026-05-26T17:21:00Z

    if (scaleInput) {
        if (displayWidth && (displayWidth != inputImageToDrawFrom->imageWidth)) {
-            constants.texMatrix[0] = vk::Vec2((float)displayWidth / inputImageToDrawFrom->imageWidth, 0.0f);
+            constants.texMatrix[0] = vk::Vec2((float)(double)displayWidth / (float)(double)inputImageToDrawFrom->imageWidth, 0.0f);


See previous review comment:#7 (comment).
The code was changed to:
constants.texMatrix[0] = vk::Vec2((float)(double)displayWidth / (float)(double)inputImageToDrawFrom->imageWidth, 0.0f);
It keeps the division precision in float instead of double.
Same for line 800.

crmurillo · 2026-05-27T08:54:27Z

-        h264->svcext.f.ref_layer_chroma_phase_x_plus1_flag = slh->ref_layer_chroma_phase_x_plus1_flag;
-        h264->svcext.f.store_ref_base_pic_flag = slh->store_ref_base_pic_flag;
+        h264->svcext.f.inter_layer_deblocking_filter_control_present_flag = sps->svc.inter_layer_deblocking_filter_control_present_flag & 0x1;
+        h264->svcext.f.extended_spatial_scalability_idc = sps->svc.extended_spatial_scalability_idc & 0x3;


seq_parameter_set_svc_extension_s has not been modified yet, therefore these changes are not reverted. Is there a plan for this?

crmurillo · 2026-05-27T10:18:41Z

-        assert(!"Invalid level idc");
-        return -1;
+        VKVS_FAIL("Invalid level idc");
+        return 0;


Is it correct to return here 0 instead of -1?
Are the callers also treating 0 as an error?

crmurillo · 2026-05-27T12:31:51Z

@@ -2442,13 +2442,13 @@ bool VulkanH265Decoder::dpb_sequence_start(VkSharedBaseObj<hevc_seq_param_s>& sp
        nvsi.frameRate = PackFrameRate(sps->stdVui.vui_time_scale, sps->stdVui.vui_num_units_in_tick);
    }
    nvsi.bProgSeq = 1; //!sps->vui.field_seq_flag;


bProgSeq type changed to bool. To be consistent, assign here to true

crmurillo · 2026-05-27T12:45:37Z

-                    m_loopFilterRefDeltas[i] = u(6);
+                    m_loopFilterRefDeltas[i] = (int8_t)u(6);
                    if (u(1)) { // sign
                        m_loopFilterRefDeltas[i] = -m_loopFilterRefDeltas[i];


This line now triggers a warning with Clang 21.1.8:
error: implicit conversion loses integer precision: 'int' to 'int8_t' (aka 'signed char') on negation [-Werror,-Wimplicit-int-conversion-on-negation].
Same for lines 667 and 757.

crmurillo · 2026-05-27T12:49:01Z

-                    m_loopFilterModeDeltas[i] = u(6);
+                    m_loopFilterModeDeltas[i] = (int8_t)u(6);
                    if(u(1)) { // sign
                        m_loopFilterModeDeltas[i] = -m_loopFilterRefDeltas[i];


Apart from the warning with newer clang versions mentioned for line 656, this line seems line a copy paste error (already existing bug before this PR).
m_loopFilterModeDeltas[i] should be assigned to -m_loopFilterModeDeltas[i], not -m_loopFilterRefDeltas[i].

dabrain34 requested a review from zlatinski December 17, 2024 15:37

dabrain34 force-pushed the dab_fix_conversion branch from 5cfbe9c to 64769ce Compare December 17, 2024 17:32

zlatinski requested changes Dec 18, 2024

View reviewed changes

dabrain34 force-pushed the dab_fix_conversion branch 2 times, most recently from b184ffc to c18a749 Compare December 19, 2024 10:21

zlatinski requested changes Dec 19, 2024

View reviewed changes

dabrain34 force-pushed the dab_fix_conversion branch 2 times, most recently from ba1547b to 2cd1ba3 Compare December 19, 2024 16:37

dabrain34 changed the title ~~Add -Wconversion and fix the releated conversion issues~~ Add -Wconversion and fix the related conversion issues Dec 19, 2024

dabrain34 force-pushed the dab_fix_conversion branch from 2cd1ba3 to dbec2b1 Compare March 17, 2025 09:34

zlatinski approved these changes May 14, 2025

View reviewed changes

dabrain34 force-pushed the dab_fix_conversion branch 4 times, most recently from e7e05b7 to d415221 Compare May 14, 2025 14:36

dabrain34 mentioned this pull request May 19, 2025

Make the code more generic for different compiler settings nvpro-samples/vk_video_samples#140

Closed

dabrain34 force-pushed the main branch from fdd3cfc to c29c26e Compare June 11, 2025 10:28

dabrain34 force-pushed the main branch from 3c8497b to c5155c0 Compare June 20, 2025 09:21

dabrain34 force-pushed the main branch from 82ffea8 to 9de2b32 Compare August 19, 2025 09:45

dabrain34 force-pushed the dab_fix_conversion branch 3 times, most recently from 6909596 to a131d28 Compare August 20, 2025 15:15

dabrain34 force-pushed the dab_fix_conversion branch from a131d28 to 3d4758c Compare May 7, 2026 08:50

dabrain34 changed the title ~~Add -Wconversion and fix the related conversion issues~~ build: add -wconversion and fix the related conversion issues May 7, 2026

CMake: add -Wconversion

73d5a54

dabrain34 added 2 commits May 21, 2026 12:35

common: fix conversion data warnings

2e3e63c

Due to -Wconversion, a lot of conversion have been discovered.

FFMPEGDemuxer: Fix data conversion warnings

6ac2d9f

dabrain34 force-pushed the dab_fix_conversion branch from 3d4758c to b13efdc Compare May 21, 2026 10:56

dabrain34 added 4 commits May 21, 2026 13:50

VkVideoDecoder: Fix data conversion issue

848ccde

NvVideoParser: fix data conversion warnings

ba776ff

VkVideoEncoder: Fix data conversion issue

ac34e43

ShaderCompiler: Use shaderc_glsl_infer_from_source for default

24a25de

As -1 is not shaderc_shader_kind, use the default one as shaderc_glsl_infer_from_source. Conversion issue.

dabrain34 force-pushed the dab_fix_conversion branch from b13efdc to 3c57c30 Compare May 21, 2026 11:50

common: rename assert(string) to VKVS_FAIL

41629a0

clang does not support string to boolean conversation in the assert.

dabrain34 force-pushed the dab_fix_conversion branch from 3c57c30 to 41629a0 Compare May 22, 2026 08:56

dabrain34 added the cts-build label May 22, 2026

crmurillo reviewed May 27, 2026

View reviewed changes

Conversation

dabrain34 commented Dec 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

Issue (optional)

Tests

Intel(R) Iris(R) Xe Graphics (ADL GT2) / Intel open-source Mesa driver Mesa 26.2.0-devel (git-e6064cf077) / Ubuntu 24.04.4 LTS

Additional Details (optional)

Uh oh!

dabrain34 commented Dec 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zlatinski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zlatinski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dabrain34 commented Mar 18, 2025

Uh oh!

dabrain34 commented Mar 28, 2025

Uh oh!

zlatinski left a comment

Choose a reason for hiding this comment

Uh oh!

dabrain34 commented May 14, 2025

Uh oh!

dabrain34 commented Aug 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dabrain34 commented Dec 17, 2024 •

edited

Loading

dabrain34 commented Dec 17, 2024 •

edited

Loading