Skip to content
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.

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

Fix regression in VGA draw's wrapped line case #2441

Merged
merged 1 commit into from Apr 30, 2023
Merged

Conversation

kcgen
Copy link
Member

@kcgen kcgen commented Apr 29, 2023

After incorporating vgaonly's features as well as using a 32-bit DAC palette, the draw_linear_line_from_dac_palette's wrapped portion of the function was not updated in the same way as the unwrapped (or "whole-line") approach.

This commit moves the line-writing chunk into a lambda (to avoid code-repetition), which also ensures they all use the same writing logic.

Fixes #2439.

@kcgen kcgen self-assigned this Apr 29, 2023
@kcgen kcgen added bug Something isn't working video Graphics and video related issues labels Apr 29, 2023
@kcgen kcgen changed the title Use a lamda for VGA draw's wrapped and unwrapped portions Fix regression in VGA draw's wrapped line case Apr 30, 2023
@kcgen kcgen force-pushed the kc/vga-wrapped-portion-1 branch 3 times, most recently from 76d8da1 to e0b170a Compare April 30, 2023 03:37
@dreamer
Copy link
Member

dreamer commented Apr 30, 2023

Tested our QPEG testcase in debug build - works fine. Thanks @kcgen!

@dreamer
Copy link
Member

dreamer commented Apr 30, 2023

Oh, compilers complain about unused-lambda-capture? Hmm, maybe constexpr variable does not need to be captured in this case?

@kcgen
Copy link
Member Author

kcgen commented Apr 30, 2023

Oh, compilers complain about unused-lambda-capture? Hmm, maybe constexpr variable does not need to be captured in this case?

Interesting; yes - looks like it's using the palette_map by value without needing to be explicitly captured. Oh, this is also true for the bytes_per_palette_pixel, which is also a constexpr value.

(updated)

@kcgen
Copy link
Member Author

kcgen commented Apr 30, 2023

Oh, compilers complain about unused-lambda-capture? Hmm, maybe constexpr variable does not need to be captured in this case?

Tug-o'-war between compilers :-) clang is happy now, but MSVC is not:

D:\a\dosbox-staging\dosbox-staging\src\hardware\vga_draw.cpp(377,1): error C3493: 'palette_map' cannot be implicitly captured because no default capture mode has been specified [D:\a\dosbox-staging\dosbox-staging\vs\dosbox.vcxproj]

D:\a\dosbox-staging\dosbox-staging\src\hardware\vga_draw.cpp(378,1): error C3493: 'bytes_per_palette_pixel' cannot be implicitly captured because no default capture mode has been specified [D:\a\dosbox-staging\dosbox-staging

@kcgen
Copy link
Member Author

kcgen commented Apr 30, 2023

Tried w/ reference and two values:

... = [&target_addr, palette_map, bytes_per_palette_pixel](...)

But now clang is warning again:

../../src/hardware/vga_draw.cpp:372:42: warning: lambda capture 'palette_map' is not required to be captured for this use [-Wunused-lambda-capture]
        auto write_pixels_from = [&target_addr, palette_map, bytes_per_palette_pixel](const uint8_t* vga_line_start,
                                              ~~^~~~~~~~~~~
../../src/hardware/vga_draw.cpp:372:55: warning: lambda capture 'bytes_per_palette_pixel' is not required to be captured for this use [-Wunused-lambda-capture]
        auto write_pixels_from = [&target_addr, palette_map, bytes_per_palette_pixel](const uint8_t* vga_line_start,

Going back to the single reference.

@kcgen
Copy link
Member Author

kcgen commented Apr 30, 2023

Investigated the additional clang static analysis issue:

2023-04-30_13-16

These turned out to be all voodoo-related:

2023-04-30_13-12

I've added a commit to accommodate them so we can carry on.

Will merge once this is passing CI.

@dreamer
Copy link
Member

dreamer commented Apr 30, 2023

Investigated the additional clang static analysis issue:

These turned out to be all voodoo-related:

Yeah, this is weird... we already had 12 warnings with voodoo enabled and now it jumped back up…

Good call, let's keep the higher limit for now and limit it by fixing issues in voodoo code.

After incorperating "vgaonly"'s features and using a 32-bit
DAC palette, the draw_linear_line_from_dac_palette's wrapped
portion of the function was not updated in the same way
as the unwrapped (or "whole-line") approach.

This commit moves the line-writing chunk into a lambda
(to avoid code-reptition), which also ensures that both the
wrapped and unwrapped code chunks use the same writing
logic.
@kcgen
Copy link
Member Author

kcgen commented Apr 30, 2023

the warning bump was my fault in this PR.

I was testing some SIMD acceleration (both SSE and NEON) in the VGA draw function - and forgot to remove that GCC flag from Meson.

This exposed 4 new warnings in libFLAC that I incorrectly believed were there all along.. I've snipped out that Meson SSE flag and we're back to 12 warnings.

@kcgen kcgen merged commit 9f2f833 into main Apr 30, 2023
58 of 59 checks passed
@GranMinigun
Copy link
Contributor

GranMinigun commented May 1, 2023

MSVC cannot into standards-conforming lambdas by default, I had it spew out a ton of errors on my branch because of that. You can just cherry-pick 1453a5b (or bump to C++20, MSVC will turn on updated parser automatically then) to make everyone happy.

@johnnovak
Copy link
Member

@kcgen Just tested this on my end and now QPEG works fine without any crashes! Great quick fix! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working video Graphics and video related issues
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Hard crash on Windows when using QPEG with the latest dev build
4 participants