Skip to content

Consistently cast and wrap the mixer's position #2745

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
Merged
merged 3 commits into from
Aug 10, 2023
Merged

Consistently cast and wrap the mixer's position #2745

merged 3 commits into from
Aug 10, 2023

Conversation

kcgen
Copy link
Member

@kcgen kcgen commented Aug 9, 2023

Applies the same ring-buffer wrapping in all places in the mixer, consistently. Possibly fixes #2704.

@kcgen kcgen added bug Something isn't working audio Audio related issues or enhancements labels Aug 9, 2023
@kcgen kcgen requested review from johnnovak and weirddan455 August 9, 2023 22:29
@kcgen kcgen self-assigned this Aug 9, 2023
@weirddan455
Copy link
Collaborator

I still get a crash in a debug build of this branch, though the stack trace looks a bit different now:

dosbox: ../../include/support.h:150: constexpr cast_t check_cast(check_t) [with cast_t = short unsigned int; check_t = long int]: Assertion `sizeof(check_t) < sizeof(cast_t) || static_cast<next_int_t<check_t>>(in) <= static_cast<next_int_t<cast_t>>( std::numeric_limits<cast_t>::max())' failed.

Loguru caught a signal: SIGABRT
Stack trace:
34      0x5606ac0ddbe5 _start + 37
33      0x7f3389427d8a __libc_start_main + 138
32      0x7f3389427cd0 /usr/lib/libc.so.6(+0x27cd0) [0x7f3389427cd0]
31      0x5606ac0ddce6 main + 32
30      0x5606ac44a8e4 sdl_main(int, char**) + 4220
29      0x5606ac106b79 Config::StartUp() + 25
28      0x5606ac6771a8 SHELL_Init() + 5120
27      0x5606ac674286 DOS_Shell::Run() + 1484
26      0x5606ac6737dc DOS_Shell::ParseLine(char*) + 2158
25      0x5606ac67bbe8 DOS_Shell::DoCommand(char*) + 504
24      0x5606ac69f54f DOS_Shell::Execute(std::basic_string_view<char, std::char_traits<char> >, std::basic_string_view<char, std::char_traits<char> >) + 1135
23      0x5606ac6a047d build/debug/dosbox(+0xbe547d) [0x5606ac6a047d]
22      0x5606ac2092b4 CALLBACK_RunRealInt(unsigned char) + 94
21      0x5606ac0de5c3 DOSBOX_RunMachine() + 14
20      0x5606ac0dde89 build/debug/dosbox(+0x622e89) [0x5606ac0dde89]
19      0x5606ac258e7b CPU_Core_Normal_Run() + 71510
18      0x5606ac4ec6e7 IO_WriteB(unsigned short, unsigned char) + 599
17      0x5606ac4ef576 write_byte_to_port(unsigned short, unsigned char) + 214
16      0x5606ac4f0ccc std::function<void (unsigned short, unsigned int, io_width_t)>::operator()(unsigned short, unsigned int, io_width_t) const + 120
15      0x5606ac47f8ae std::_Function_handler<void (unsigned short, unsigned int, io_width_t), void (*)(unsigned short, unsigned int, io_width_t)>::_M_invoke(std::_Any_data const&, unsigned short&&, unsigned int&&, io_width_t&&) + 103
14      0x5606ac47fbf4 std::enable_if<is_invocable_r_v<void, void (*&)(unsigned short, unsigned int, io_width_t), unsigned short, unsigned int, io_width_t>, void>::type std::__invoke_r<void, void (*&)(unsigned short, unsigned int, io_width_t), unsigned short, unsigned int, io_width_t>(void (*&)(unsigned short, unsigned int, io_width_t), unsigned short&&, unsigned int&&, io_width_t&&) + 103
13      0x5606ac480190 void std::__invoke_impl<void, void (*&)(unsigned short, unsigned int, io_width_t), unsigned short, unsigned int, io_width_t>(std::__invoke_other, void (*&)(unsigned short, unsigned int, io_width_t), unsigned short&&, unsigned int&&, io_width_t&&) + 102
12      0x5606ac576fa4 build/debug/dosbox(+0xabbfa4) [0x5606ac576fa4]
11      0x5606ac5759a3 build/debug/dosbox(+0xaba9a3) [0x5606ac5759a3]
10      0x5606ac574ad6 build/debug/dosbox(+0xab9ad6) [0x5606ac574ad6]
9       0x5606ac573f78 build/debug/dosbox(+0xab8f78) [0x5606ac573f78]
8       0x5606ac573ca1 build/debug/dosbox(+0xab8ca1) [0x5606ac573ca1]
7       0x5606ac50e237 MixerChannel::FillUp() + 137
6       0x5606ac1ce66b unsigned short check_cast<unsigned short, long>(long) + 120
5       0x7f3389436d26 /usr/lib/libc.so.6(+0x36d26) [0x7f3389436d26]
4       0x7f33894263dc /usr/lib/libc.so.6(+0x263dc) [0x7f33894263dc]
3       0x7f33894264b8 abort + 215
2       0x7f338943e668 raise + 24
1       0x7f338948e83c /usr/lib/libc.so.6(+0x8e83c) [0x7f338948e83c]
0       0x7f338943e710 /usr/lib/libc.so.6(+0x3e710) [0x7f338943e710]
2023-08-09 17:34:04.959 | Signal: SIGABRT
Aborted (core dumped)

@kcgen
Copy link
Member Author

kcgen commented Aug 9, 2023

Hmm.. yes; it's still overflowing on the quantity itself now.

MIXER_LockAudioDevice();
Mix(check_cast<uint16_t>(static_cast<int64_t>(index * mixer.frames_needed)));
MIXER_UnlockAudioDevice();

That's a quantity and not a ring-buffer index, so I'll digest the quantity down using a loop.

@johnnovak
Copy link
Member

This fixes the macOS debug build assertion failure when plugging in headphones. Tried it with a few programs, works 100% reliably. Nice one @kcgen 🎉

johnnovak
johnnovak approved these changes Aug 9, 2023
View reviewed changes
Copy link
Member

@johnnovak johnnovak left a comment

Choose a reason for hiding this comment

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

Looks good @kcgen 🎉

@kcgen
Copy link
Member Author

kcgen commented Aug 10, 2023

@weirddan455 ,

I still get a crash in a debug build of this branch, though the stack trace looks a bit different now:

If you can try the update, curious if the chunked mixing avoids the crash on your side.

kcgen added 3 commits August 9, 2023 17:19
@kcgen
Consistently cast and wrap the mixer's position
748e5a2
@kcgen
Standardize the mixer's buffer length constants
35075bd 
Rename to CamelCase and use length instead of size because
we're dealing with sample counts and not raw bytes.
@kcgen
Fillup the audio mixer in chunks
1c0d796
@kcgen kcgen force-pushed the kc/queue-audio-frames-1 branch from 73f202d to 1c0d796 Compare August 10, 2023 00:20
@weirddan455
Copy link
Collaborator

@kcgen Yep, that fixes the crash. Great job. 👍

Sidenote, I did notice an additional bug. Dosbox hangs on quit when I have aplay running. I'm pretty confident it's an SDL bug though. I've been debugging it and reproduced it on a simple test program using only SDL. It hangs on SDL_Quit waiting for the pulseaudio thread that never terminates.

But, like @johnnovak said in the bug thread, this is a very corner case and I think it's not our bug anyway so I think we can close this one up.

@weirddan455
Copy link
Collaborator

Also, if you're curious what was happening, I found that SDL opened the audio device without reporting any errors, makes a single call to the callback function, and then stops making callbacks. So definitely an edge case but good job getting it fixed 😄

@kcgen
Copy link
Member Author

kcgen commented Aug 10, 2023 •
edited
Loading

I found that SDL opened the audio device without reporting any errors, makes a single call to the callback function, and then stops making callbacks.

Very interesting!

Sounds like little SDL went trick-or-treating to the haunted audio mansion at the end of the block, and never returned. "Your callbacks will never be heard again!" 🦇 🧛‍♂️

@kcgen kcgen merged commit 2541315 into main Aug 10, 2023
@kcgen kcgen deleted the kc/queue-audio-frames-1 branch August 10, 2023 16:30
@Grounded0 Grounded0 mentioned this pull request Nov 17, 2023
Closed
4 tasks
@johnnovak johnnovak added audio Audio related issues or enhancements and removed audio Audio related issues or enhancements labels Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johnnovak johnnovak johnnovak approved these changes

@weirddan455 weirddan455 Awaiting requested review from weirddan455

Assignees

@kcgen kcgen

Labels
audio Audio related issues or enhancements bug Something isn't working
Projects
No open projects
0.81 Project
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Debug build crashes when plugging in headphones
3 participants
@kcgen @weirddan455 @johnnovak