Sleep the coreaudio driver in-editor after 15s of silence #45948

ellenhp · 2021-02-13T08:49:33Z

Fixes #28039
Should help mitigate #38154 but might not solve the root cause

This PR allows the coreaudio driver to enter a sleep state when the AudioServer requests it to. Mutexes have been added to all audio players and the video player to prevent a play() call acquiring a playback "lock" from racing with a _mix_audio call releasing it. I'm not a C++ expert so maybe there's some fancy RAII way to do this but I figured I'd start simple and avoid overengineering.

How to test:

Build & run the editor with these changes on a mac
Create each kind of AudioStreamPlayer. Play the players one at a time and watch Godot's "Preventing Sleep" status in Activity Monitor flip from "No" to "Yes" within a few seconds of starting the player, then flip back to "No" about 15 seconds after the audio is finished playing.

I have been unable to test video players. They don't seem to work in latest master as far as I can tell.

drivers/coreaudio/audio_driver_coreaudio.cpp

naithar · 2021-02-13T16:52:17Z

servers/audio_server.cpp

@@ -1032,6 +1048,14 @@ void AudioServer::update() {
 	for (Set<CallbackItem>::Element *E = update_callbacks.front(); E; E = E->next()) {
 		E->get().callback(E->get().userdata);
 	}
+
+	if (Engine::get_singleton()->is_editor_hint()) {


If this change should only work for editor, maybe #ifdef TOOLS_ENABLED should also be added, so it's not included in templates builds?

The main reason I see to do that is export binary size and potentially (slightly) reducing lock contention, which makes me wonder if I should do it elsewhere too. What do you think?

I haven't yet checked the whole PR, but I can already say if you want something to happen only in the editor the current check is better, because it is false both on tools builds when running a project (vs. running the editor) and on non-tools builds.
Furthermore, in non-tools builds, given the check is known to fail at compile time, I'm quite certain that compilers can optimize out the body.

Well, some places seem to use only Engine::get_singleton()->is_editor_hint() while others use both Engine::get_singleton()->is_editor_hint() and TOOLS_ENABLED checks. Maybe I'm missing something, but if using only Engine::get_singleton()->is_editor_hint() is preferred it's good to know :)

There's no clear policy, but yes we sometimes put both checks in place to ensure that the editor-specific code (often expensive debug code) is compiled out in the runtime. If @RandomShaper is correct that compiler would do it just fine with is_editor_hint() only, then we shouldn't need to do that indeed.

If we want to have access to this feature enabled for any other engine mode in project settings, then I guess TOOLS_ENABLED isn't needed too.

naithar · 2021-02-13T16:56:23Z

I'll try testing it once I'm free, but so far changes look okay.

RandomShaper · 2021-02-14T03:34:50Z

I'm thinking this could be simpler. Let me explain.

We have the audio/channel_disable_time project setting, that defaults to 2 seconds, but let's assume the editor could override it internally to 15.

Now, AudioServer::_mix_step() goes through all the buses and items in it and is able to flag as inactive those that meet the condition for inactivation (i.e., too much time without playing):

godot/servers/audio_server.cpp

Line 446 in 2b95372

bus->channels.write[k].active = false;

Because of that, and more importantly, it has a notion about if any channel is playing or none at all. This check can be used to contribute to a local boolean that tells that once the loop has ended:

godot/servers/audio_server.cpp

Line 403 in 2b95372

if (!bus->channels[k].active) {

My point is: couldn't that be used to make the driver sleep and wake up in a simpler that than all the added code? (The added API to the drivers would still be needed and leveraged.)

ellenhp · 2021-02-14T03:45:32Z

That's similar to the way I wanted to do this fix originally, but that code to check if a bus is active in _mix_step won't be called if the audio driver is sleeping.

The CoreAudio driver sets up its own output_callback function to be called by CoreAudio, I believe. That callback calls audio_server_process which calls _driver_process which calls _mix_step. Without that callback actively being called by CoreAudio, the only way to restart the driver would be from outside the audio thread.

RandomShaper · 2021-02-14T05:16:35Z

Oh, I wasn't aware of that. Makes sense.

scene/2d/audio_stream_player_2d.h

servers/audio_server.cpp

fab918 · 2021-02-14T08:02:25Z

Is this improvement can be available for release build? (useful if you make a software)

ellenhp · 2021-02-14T08:33:17Z

@fab918 I'm hoping this fix will be cherry-picked for 3.2 so those of us who prefer stable builds (like me 😄) can benefit. I don't know if it will make it into 3.2.4 though. It may have to wait until the release after that. It really depends on how many more release candidates there are and what the project maintainers are comfortable cherry picking between release candidates. It would be nice though to get into 3.2.4 but I need to remind myself to be patient sometimes 👍

I'll respond to the most recent round of review comments tomorrow morning UTC-8.

naithar · 2021-02-14T08:53:20Z

Tested on M1 MacBook, but I don't think result would be different for Intel based macs.

When running Project Editor everything seems to work exactly as described - after some time coreaudiod returns to 0% CPU and Godot is no longer preventing mac from going to sleep mode.
But Project Manager doesn't seems to be affected, so it's still preventing sleep and taking CPU time through coreaudiod. I'm not sure it's anything serious, but if it's possible to apply this fix to all editor windows I think it should be applied.

Edit:
@ellenhp I've also hasn't been able to cherry-pick this PR without heavily modifying the commit. I guess 4.0 handles mutexes and some other stuff differently, so you might want to create a 3.2 PR after this one is ready for merge.

Calinou · 2021-02-14T13:14:58Z

But Project Manager doesn't seems to be affected, so it's still preventing sleep and taking CPU time through coreaudiod. I'm not sure it's anything serious, but if it's possible to apply this fix to all editor windows I think it should be applied.

#38208 would fix this on all platforms 🙂

ellenhp · 2021-02-14T17:27:54Z

But Project Manager doesn't seems to be affected, so it's still preventing sleep and taking CPU time through coreaudiod. I'm not sure it's anything serious, but if it's possible to apply this fix to all editor windows I think it should be applied.

I haven't confirmed this, but I suspect the issue here is that is_editor_hint() returns false in the project manager.

naithar · 2021-02-14T17:30:33Z

But Project Manager doesn't seems to be affected, so it's still preventing sleep and taking CPU time through coreaudiod. I'm not sure it's anything serious, but if it's possible to apply this fix to all editor windows I think it should be applied.

I haven't confirmed this, but I suspect the issue here is that is_editor_hint() returns false in the project manager.

Yeah, seems like it. But as @Calinou mentioned #38208 should fix it, so that should be okay.
I'll retest again after you make other changes, but everything looks good so far.

RandomShaper

I think others need still to give a final review. For me, as far as I'm concerned, this is good enough the way it is now.

Also, @ellenhp, thanks for your patience and nicely accepting the feedback!

ellenhp · 2021-02-14T22:04:29Z

No problem! Code review is like, really important for the health of the project and as a user of Godot I care a lot about the health of the project! 😄 I'll work on a sister PR for 3.2. There will be some small changes to mutexes and I think some of the files may have been renamed, but it should largely be the same. I'll leave it in a draft state until this one is merged though.

naithar · 2021-02-15T08:23:59Z

Everything seems to work correctly, so LGTM.

akien-mga · 2021-02-15T08:57:14Z

Changes look good to me overall, though it would be good to have a quick review from @reduz as there are changes to AudioServer.

Some comments on the form:

It would be good to squash the three commits into one, as the last two comments amend/refactor what the first commit did. We usually prefer to have the "final" state of a PR reflected in the commits we merge, so that potential bisecting or post-merge code review doesn't need to jump through several iterations of code design. See PR workflow for instructions on how to rebase/squash if needed.
Your commits seem not to be attributed to your GitHub account. Most likely the email used (see header in https://patch-diff.githubusercontent.com/raw/godotengine/godot/pull/45948.patch) is not linked to your @ellenhp account. It's not a problem for Git, but if you want to claim ownership of those commits, you can add this email as secondary email in your GitHub settings.

akien-mga · 2021-02-15T09:00:07Z

Regarding the use of Engine::get_singleton()->is_editor_hint() and it not applying to the project manager, another option would be to use a project setting to control this behavior, and turn it on in main.cpp for both the editor and the project_manager.

This would make it possible to use this in Godot projects too, e.g. when making games without sound, or non-game apps.

The sleep threshold could also be made a project setting I guess.

I'd suggest waiting for additional feedback from @reduz before implementing such settings though, maybe the current approach is good as is.

bruvzg · 2021-02-15T10:41:32Z

I haven't confirmed this, but I suspect the issue here is that is_editor_hint() returns false in the project manager.

It is false in project manager, use Main::is_project_manager() to check for PM (include main\main.h).

reduz · 2021-02-15T13:51:07Z

Some thoughts on this PR..

I don't like the added complexity in AudioStreams
In many situations, you will use streams continually toggled on (like voip, microphone, etc).

If the problem is actually not being able to sleep on macs when the editor is turned on, I would probably prefer to not stop the audio thread but stop Coreaudio after some time with silence, and then keep a thread alive checking if there is audio data in order to re-enable coreaudio. This of course would be used in editor or lowpcu mode only.

reduz · 2021-02-15T16:44:28Z

I also wonder if this could probably be done in AudioServer directly, so drivers also dont really know whats going on either. When low cpu mode is enabled, turn off the audio driver after X time of silence and start a thread that scans for any audio going on, then re-initializes audio.

ellenhp · 2021-02-15T16:57:11Z

Could you expand on what you mean by stopping CoreAudio without stopping the audio thread? If that's possible I think it would be the way to go, but I'm not sure how I'd go about that. It looks to me like stopping CoreAudio stops the audio thread, but I could be wrong.

edit: To my untrained eyes it seems like the entire audio thread is just whatever thread CoreAudio uses in its "give me more frames please" callback. A lot of the complexity of this PR is getting around the fact that the callback isn't being called (therefore audio_server_process) isn't being called, etc. I don't think there's a way around that short of creating an entirely new thread to continue calling audio_server_process but that sounds like even more complexity.

edit again: sorry, I was waking up when I read your comments originally. I think I understand what you're saying. I'll hack away at this a bit more and see if I can come up with something.

ellenhp · 2021-02-22T06:52:04Z

This PR will get way simpler if I manage to implement godotengine/godot-proposals#2299 so I think I'm going to close it. The downside is that the implementation of that proposal will almost certainly not be backported, so fixing this bug will probably be something that only happens in 4.0.

Calinou · 2021-12-04T00:25:31Z

It looks like this approach would be useful to explore again, with #28039 also occurring on Windows.

ellenhp · 2021-12-04T00:48:34Z

Interesting. This is a much easier fix now after the AudioServer rewrite because we don't have to add code in each player node anymore. The audio server has all the information it needs to toggle the driver on and off.

Sleep the coreaudio driver in-editor after 15s of silence

a3187f3

ellenhp marked this pull request as ready for review February 13, 2021 08:51

Calinou added cherrypick:3.x Considered for cherry-picking into a future 3.x release enhancement platform:macos topic:audio topic:porting labels Feb 13, 2021

Calinou added this to the 4.0 milestone Feb 13, 2021

fire requested review from a team February 13, 2021 16:26

naithar reviewed Feb 13, 2021

View reviewed changes

drivers/coreaudio/audio_driver_coreaudio.cpp Outdated Show resolved Hide resolved

naithar reviewed Feb 13, 2021

View reviewed changes

Fix a comment and refactor coreaudio driver start/stop

02b3631

RandomShaper reviewed Feb 14, 2021

View reviewed changes

scene/2d/audio_stream_player_2d.h Show resolved Hide resolved

servers/audio_server.cpp Outdated Show resolved Hide resolved

Manage playing_sources_count with atomics instead of mutexes.

d60e691

RandomShaper approved these changes Feb 14, 2021

View reviewed changes

akien-mga requested a review from a team February 15, 2021 08:53

ellenhp closed this Feb 22, 2021

akien-mga added archived and removed cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Feb 22, 2021

Calinou mentioned this pull request Dec 4, 2021

Editor uses CPU time in the background due to the audio server being kept active #55608

Open

tavurth mentioned this pull request Feb 14, 2022

Confusing docs about AudioServer.set_device #58096

Closed

This was referenced Jul 25, 2022

[3.x] Optimize AudioServer::_driver_process() #63430

Merged

[3.x] Add ability to mute AudioServer. #63458

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sleep the coreaudio driver in-editor after 15s of silence #45948

Sleep the coreaudio driver in-editor after 15s of silence #45948

ellenhp commented Feb 13, 2021

naithar Feb 13, 2021

ellenhp Feb 13, 2021

RandomShaper Feb 13, 2021

naithar Feb 14, 2021

akien-mga Feb 15, 2021

naithar Feb 15, 2021

naithar commented Feb 13, 2021

RandomShaper commented Feb 14, 2021

ellenhp commented Feb 14, 2021

RandomShaper commented Feb 14, 2021

fab918 commented Feb 14, 2021

ellenhp commented Feb 14, 2021

naithar commented Feb 14, 2021 •

edited

Loading

Calinou commented Feb 14, 2021 •

edited

Loading

ellenhp commented Feb 14, 2021

naithar commented Feb 14, 2021

RandomShaper left a comment •

edited

Loading

ellenhp commented Feb 14, 2021

naithar commented Feb 15, 2021

akien-mga commented Feb 15, 2021

akien-mga commented Feb 15, 2021 •

edited

Loading

bruvzg commented Feb 15, 2021

reduz commented Feb 15, 2021

reduz commented Feb 15, 2021

ellenhp commented Feb 15, 2021 •

edited

Loading

ellenhp commented Feb 22, 2021

Calinou commented Dec 4, 2021

ellenhp commented Dec 4, 2021

Sleep the coreaudio driver in-editor after 15s of silence #45948

Sleep the coreaudio driver in-editor after 15s of silence #45948

Conversation

ellenhp commented Feb 13, 2021

naithar Feb 13, 2021

Choose a reason for hiding this comment

ellenhp Feb 13, 2021

Choose a reason for hiding this comment

RandomShaper Feb 13, 2021

Choose a reason for hiding this comment

naithar Feb 14, 2021

Choose a reason for hiding this comment

akien-mga Feb 15, 2021

Choose a reason for hiding this comment

naithar Feb 15, 2021

Choose a reason for hiding this comment

naithar commented Feb 13, 2021

RandomShaper commented Feb 14, 2021

ellenhp commented Feb 14, 2021

RandomShaper commented Feb 14, 2021

fab918 commented Feb 14, 2021

ellenhp commented Feb 14, 2021

naithar commented Feb 14, 2021 • edited Loading

Calinou commented Feb 14, 2021 • edited Loading

ellenhp commented Feb 14, 2021

naithar commented Feb 14, 2021

RandomShaper left a comment • edited Loading

Choose a reason for hiding this comment

ellenhp commented Feb 14, 2021

naithar commented Feb 15, 2021

akien-mga commented Feb 15, 2021

akien-mga commented Feb 15, 2021 • edited Loading

bruvzg commented Feb 15, 2021

reduz commented Feb 15, 2021

reduz commented Feb 15, 2021

ellenhp commented Feb 15, 2021 • edited Loading

ellenhp commented Feb 22, 2021

Calinou commented Dec 4, 2021

ellenhp commented Dec 4, 2021

naithar commented Feb 14, 2021 •

edited

Loading

Calinou commented Feb 14, 2021 •

edited

Loading

RandomShaper left a comment •

edited

Loading

akien-mga commented Feb 15, 2021 •

edited

Loading

ellenhp commented Feb 15, 2021 •

edited

Loading