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

[NOSQUASH] Move soundmanager into its own thread #13633

Merged
merged 2 commits into from
Sep 26, 2023

Conversation

Desour
Copy link
Member

@Desour Desour commented Jun 27, 2023

  • What: Moves the soundmgr into a thread. It's communicating with a proxy soundmgr via mutexed queues.
  • Why: If some client step takes too long, the sound queues run empty. (There's also a warning when this happens.) The separate thread is not affected by these hiccups.
    It's also nice to leverage more cpu cores, and not take time from the main client thread's step time for loading sounds.

This PR also bumps the minimum clang version to 7, because older clang has issues with std::variant.

To do

This PR is a Ready for Review.

How to test

  • Keep playing a long sound.
  • Cause client lag.
    • With CPCSM: .lua local gut=core.get_us_time local t0 = gut() while gut()-t0 < 2e6 do end
    • Or e.g. open a formspec that takes long to load, like the first time you open a unifined inv inv on some servers.

@Desour
Copy link
Member Author

Desour commented Jun 27, 2023

Uh, looks like std::variant is broken in old clang with gcc's libstdc++. https://stackoverflow.com/questions/46859053/stdvisit-for-variant-fails-to-compile-under-clang-5

@Desour Desour force-pushed the sound_thread branch 4 times, most recently from 1a3984a to 2f321b8 Compare June 29, 2023 09:43
@Desour Desour changed the title Move soundmanager into its own thread [NOSQUASH] Move soundmanager into its own thread Jun 29, 2023
@Desour
Copy link
Member Author

Desour commented Jun 29, 2023

I've decided to just bump clang. clang 7 is available in ubuntu 20.04 (focal) and debian 10.0 (buster).

@SmallJoker
Copy link
Member

If some client step takes too long, the sound queues run empty.

  1. How does it run dry? OpenAL already receives entire playback instructions when the packets are received from the server.
  2. How long is "too long"? I would consider anything above 0.1 s (10 Hz) to be acceptable to drop sounds

This PR does make proper use of a whole new action queue but I think this is way too overengineered for the edge-case it tries to solve.

@Desour
Copy link
Member Author

Desour commented Jul 13, 2023

If some client step takes too long, the sound queues run empty.

  1. How does it run dry? OpenAL already receives entire playback instructions when the packets are received from the server.

Too long server steps are irrelevant here, the server does not send stream instructions or anything like that (yet).
But the client has to enqueue openal buffers to keep the sound playing. If it does this too slowly (because the sound manager didn't get cpu time for too long), the buffer runs out.

  1. How long is "too long"? I would consider anything above 0.1 s (10 Hz) to be acceptable to drop sounds

It's configurable via compile constants (see sound_openal_internal.h). Right now I think in the worst case 0.4 s is too long (1.0 - 0.3*2).

The thing is tough that in practice we reach higher client dtimes.
And also, it's a good thing to take pressure from the main client thread (as said in top comment).

This PR does make proper use of a whole new action queue but I think this is way too overengineered for the edge-case it tries to solve.

It's not the first use of MutexedQueue for inter-thread communication. See for example Connection::m_command_queue. There's also Client::m_client_event_queue which uses a plain std::queue, and tagged unions.
The only new thing is the use of C++17 std::variant, which is basically a tagged union but with destructors, std::visit (as opposed to using a function-ptr array, like Game::clientEventHandler) and other convenience stuff.

I thought about passing arbitrary std::functions instead of the variant, or tuples of member function ptrs plus arguments, but I preferred simplicity over brevity.

@SmallJoker
Copy link
Member

SmallJoker commented Jul 15, 2023

The thing is tough that in practice we reach higher client dtimes.

That's true but I don't think it's a major concern to pause the audio while something intense is being loaded.


What about std::bind? That way you might not need to create std::variant and custom structs. Example:

	std::vector<std::function<void()>> queue;
	auto f1 = std::bind(&MyClassName::my_function, class_ptr, ARG1, ARG2, ....);
	queue.push_back(f1);
	queue[0]();

This is less code and at least to me this looks simpler. What do you think?

@Desour
Copy link
Member Author

Desour commented Jul 16, 2023

The bind approach is what I meant with the std::function thing.
Here's an example how this can look like.
It looks a bit shorter. But there are things I don't like about this approach, e.g.:

  • Idk how I can properly move the oggvorbis filedata string. I've used a const_cast, but I'm not sure if it's correct, or if the string in the std::function is still copied somewhere. C++23 std::move_only_function looks appropriate.
  • The details of how the soundmgr thread handles the messages is moved out of the thread, to the sender (the proxy class). This feels a bit ugly to me.

@Desour Desour changed the title [NOSQUASH] Move soundmanager into its own thread [MANUAL SQUASH] Move soundmanager into its own thread Aug 23, 2023
@Desour
Copy link
Member Author

Desour commented Aug 23, 2023

Reverted the last commit, because it was not good, as said above.

@Sokomine
Copy link
Contributor

The warning messages for "sound queue ran empty" do get a bit much. Searching through my debug.txt yielded over 420000 such warning lines withhin less than 14 days. Help!
Example message:
WARNING[Main]: PlayingSound::stepStream: Sound queue ran empty for "env_sounds_water.4.ogg
If this PR can help, then please get it on its way.

@Desour Desour added this to the 5.8.0 milestone Sep 26, 2023
@Desour Desour added the Blocker The issue needs to be addressed before the next release. label Sep 26, 2023
@Desour
Copy link
Member Author

Desour commented Sep 26, 2023

Added to milestone, because I consider the warnings a regression.

If the PR won't be considered mergable in reasonable time, an alternative temporary fix could be to put the warnings to inforstream.

@SmallJoker SmallJoker self-requested a review September 26, 2023 11:36
std::variant is broken in clang < 7.0.1 with libstdc++
see: llvm/llvm-project#32569
Fixes sound queues running empty on client step hiccups.
@Desour
Copy link
Member Author

Desour commented Sep 26, 2023

(Trivially rebased.)

Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

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

Issue reproduced. The sound now stops properly after a while when the lag is too high.

@Desour
Copy link
Member Author

Desour commented Sep 26, 2023

Thanks!

@Desour Desour changed the title [MANUAL SQUASH] Move soundmanager into its own thread [NoSQUASH] Move soundmanager into its own thread Sep 26, 2023
@Desour Desour changed the title [NoSQUASH] Move soundmanager into its own thread [NOSQUASH] Move soundmanager into its own thread Sep 26, 2023
@Desour
Copy link
Member Author

Desour commented Sep 26, 2023

Did the manual squash, to remove the std::function commit and its revert. Click on the compare button to see that it changed nothing.

@Desour Desour merged commit 8fa2ea7 into minetest:master Sep 26, 2023
15 checks passed
@Desour Desour deleted the sound_thread branch September 26, 2023 20:11
@ancientmarinerdev
Copy link

Sweet. Good work, Desour. Sound has definitely needed attention, and it's always great to see progress in this area.

@sfan5
Copy link
Member

sfan5 commented Oct 12, 2023

You forgot to change this:

set(CLANG_MINIMUM_VERSION "6.0")

@Desour
Copy link
Member Author

Desour commented Oct 12, 2023

Oops, thx for noticing. #13889

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocker The issue needs to be addressed before the next release. Bugfix 🐛 PRs that fix a bug One approval ✅ ◻️ Sounds
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants