Skip to content

Add -Wconversion and fix the related conversion issues #7

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Add -Wconversion and fix the related conversion issues #7

wants to merge 7 commits into from

Conversation

@dabrain34
Copy link
Contributor

No description provided.

@dabrain34 dabrain34 requested a review from zlatinski December 17, 2024 15:37
@dabrain34
Copy link
Contributor Author

dabrain34 commented Dec 17, 2024 •
edited
Loading

@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
zlatinski requested changes Dec 18, 2024
View reviewed changes
Copy link
Contributor

@zlatinski zlatinski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@dabrain34 dabrain34 force-pushed the dab_fix_conversion branch 2 times, most recently from b184ffc to c18a749 Compare December 19, 2024 10:21
zlatinski
zlatinski requested changes Dec 19, 2024
View reviewed changes
Copy link
Contributor

@zlatinski zlatinski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 dabrain34 force-pushed the dab_fix_conversion branch 2 times, most recently from ba1547b to 2cd1ba3 Compare December 19, 2024 16:37
@dabrain34 dabrain34 changed the title Add -Wconversion and fix the releated conversion issues Add -Wconversion and fix the related conversion issues Dec 19, 2024
@dabrain34 dabrain34 force-pushed the dab_fix_conversion branch from 2cd1ba3 to dbec2b1 Compare March 17, 2025 09:34
@dabrain34
Copy link
Contributor Author

@zlatinski are you okay to merge this change ?

@dabrain34
Copy link
Contributor Author

ping @zlatinski

zlatinski
zlatinski approved these changes May 14, 2025
View reviewed changes
Copy link
Contributor

@zlatinski zlatinski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 dabrain34 force-pushed the dab_fix_conversion branch 4 times, most recently from e7e05b7 to d415221 Compare May 14, 2025 14:36
@dabrain34
Copy link
Contributor Author

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 dabrain34 mentioned this pull request May 19, 2025
Closed
@dabrain34 dabrain34 force-pushed the dab_fix_conversion branch 2 times, most recently from edacae7 to 8b38420 Compare August 20, 2025 12:06
@dabrain34
vkVideoEncoder: Fix data conversion issue
bf1a4cc
@dabrain34
ShaderCompiler: Use shaderc_glsl_infer_from_source for default
a131d28 
As -1 is not shaderc_shader_kind, use the default one as
shaderc_glsl_infer_from_source.

Conversion issue.
@dabrain34
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zlatinski zlatinski zlatinski approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dabrain34 @zlatinski