Page MenuHomeWildfire Games
Differential D3445

Probably fix OpenAL error on MacOS after a while
ClosedPublic
Actions

Authored by wraitii on Jan 21 2021, 3:16 PM.

Details

Reviewers
None
Commits
rP24762: Fix audio leak that resulted in openAL errors after a while.
Trac Tickets
#5265
Summary

MacOS sometimes report OpenAL errors, and sound stops working, after a while.
There might be other issues, but one consistently reproducible one was reported by @Langbart, and the underlying cause appears to be that:

  • we don't properly clean up some sounds
  • MacOS has a max 'in-flight' buffer limit of 1000, and streaming data use 50 buffers at a time (e.g. music/ambient).

The problem is that sounds can end up paused / in initial state while also being in 'last play', but the deletion code only checks for "STOPPED". This fixes that.

Seems to work for the reproduction by langbart, can't confirm that it removes all cases of OpenAL errors.

Reported by: Eszett

Thanks langbart for the consistent repro'.

Test Plan

Start Combat Demo as many times as necessary to have errors. With the patch, it ought never error.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

wraitii created this revision.Jan 21 2021, 3:16 PM
Stan added a subscriber: Stan.Jan 21 2021, 3:20 PM
Stan added inline comments.
source/soundmanager/items/CBufferItem.cpp
82–83 ↗(On Diff #15603)

Maybe?

source/soundmanager/items/CSoundItem.cpp
60–61 ↗(On Diff #15603)
source/soundmanager/items/CStreamItem.cpp
76–79 ↗(On Diff #15603)

Can you explain this change?

Vulcan added a comment.Jan 21 2021, 3:21 PM

Build has FAILED

builderr-release-gcc7.txt
In file included from ../../../source/pch/engine/precompiled.h:18,
                 from ../../../source/soundmanager/items/CBufferItem.cpp:18:
../../../source/lib/precompiled.h:101: error: unterminated #if
 #if CONFIG_ENABLE_PCH
 
../../../source/lib/precompiled.h:78: error: unterminated #if
 #if CONFIG_ENABLE_BOOST
 
../../../source/lib/precompiled.h:57: error: unterminated #if
 #if ICC_VERSION
 
../../../source/lib/precompiled.h:47: error: unterminated #ifdef
 # ifdef NDEBUG // release: disable all checks
 
../../../source/lib/precompiled.h:44: error: unterminated #if
 # if MSC_VERSION < 1910
 
../../../source/lib/precompiled.h:43: error: unterminated #if
 #if MSC_VERSION
 
../../../source/lib/precompiled.h:35: error: unterminated #ifndef
 #ifndef MINIMAL_PCH
 
In file included from ../../../source/pch/engine/precompiled.h:23,
                 from ../../../source/soundmanager/items/CBufferItem.cpp:18:
../../../source/ps/CStr.h:94: error: unterminated #else

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/4664/display/redirect
See console output for more information: https://jenkins.wildfiregames.com/job/docker-differential/4664/display/redirectconsole

Harbormaster completed remote builds in B15325: Diff 15603.Jan 21 2021, 3:22 PM
Vulcan added a comment.Jan 21 2021, 3:22 PM

Build is green

builderr-debug-macos.txt
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libengine_dbg.a(precompiled.o) has no symbols
ld: warning: text-based stub file /System/Library/Frameworks//CoreAudio.framework/CoreAudio.tbd and library file /System/Library/Frameworks//CoreAudio.framework/CoreAudio are out of sync. Falling back to library file for linking.
ld: warning: text-based stub file /System/Library/Frameworks//AudioToolbox.framework/AudioToolbox.tbd and library file /System/Library/Frameworks//AudioToolbox.framework/AudioToolbox are out of sync. Falling back to library file for linking.
ld: warning: text-based stub file /System/Library/Frameworks//ForceFeedback.framework/ForceFeedback.tbd and library file /System/Library/Frameworks//ForceFeedback.framework/ForceFeedback are out of sync. Falling back to library file for linking.
ld: warning: text-based stub file /System/Library/Frameworks//CoreVideo.framework/Cor

See https://jenkins.wildfiregames.com/job/macos-differential/3007/display/redirect for more details.

wraitii requested review of this revision.Jan 21 2021, 3:22 PM
wraitii added inline comments.Jan 21 2021, 4:40 PM
source/soundmanager/items/CStreamItem.cpp
76–79 ↗(On Diff #15603)

well it was always false given the if above.

Stan added inline comments.Jan 21 2021, 4:44 PM
source/soundmanager/items/CStreamItem.cpp
76–79 ↗(On Diff #15603)

This was changed in rP12989 any information there? Also now with the condition change it's no longer always false.

wraitii added inline comments.Jan 21 2021, 4:53 PM
source/soundmanager/items/CStreamItem.cpp
76–79 ↗(On Diff #15603)

Nah it wasn't, that was merely indentation. It is still ally false because I would have changed the condition so it is :p

Langbart added a comment.Jan 21 2021, 5:24 PM

I tried this patch on rP24750 and was unable to generate OpenAl error messages as I described in the associated ticket. After 90mins of testing I gave up.

Stan requested changes to this revision.Jan 21 2021, 6:25 PM

On windows a lot of sounds are not played.

source/soundmanager/items/CBufferItem.cpp
1 ↗(On Diff #15603)
source/soundmanager/items/CSoundItem.cpp
1 ↗(On Diff #15603)
source/soundmanager/items/CStreamItem.cpp
1 ↗(On Diff #15603)
This revision now requires changes to proceed.Jan 21 2021, 6:25 PM
Stan removed a reviewer: Stan.Jan 21 2021, 10:06 PM

Okay, false alert.

This revision now requires review to proceed.Jan 21 2021, 10:06 PM
wraitii updated this revision to Diff 15613.Jan 21 2021, 10:36 PM

Inlines

Stan added inline comments.Jan 21 2021, 11:11 PM
source/soundmanager/items/CStreamItem.cpp
78–79 ↗(On Diff #15613)

Could be this maybe, judging from the code below. Not sure we need the AL_CHECK.

TBH we could probably reduce the complexity by refactoring a bit, but that's kinda out of scope.

Vulcan added a comment.Jan 21 2021, 11:39 PM

Build is green

builderr-debug-macos.txt
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libsimulation2_dbg.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libengine_dbg.a(precompiled.o) has no symbols
ld: warning: text-based stub file /System/Library/Frameworks//CoreAudio.framework/CoreAudio.tbd and library file /System/Library/Frameworks//CoreAudio.framework/CoreAudio are out of sync. Falling back to library file for linking.
ld: warning: text-based stub file /System/Library/Frameworks//AudioToolbox.framework/AudioToolbox.tbd and library file /System/Library/Frameworks//AudioToolbox.framework/AudioToolbox are out of sync. Falling back to library file for linking.
ld: warning: text-based stub file /System/Library/Frameworks//ForceFeedback.framework/ForceFeedback.tbd and library file /System/Library/Frameworks//Force

See https://jenkins.wildfiregames.com/job/macos-differential/3012/display/redirect for more details.

Harbormaster completed remote builds in B15335: Diff 15613.Jan 21 2021, 11:39 PM
wraitii updated this revision to Diff 15619.Jan 22 2021, 10:00 AM
wraitii marked an inline comment as done.

Cleanup StreamItem control flow per Stan

Vulcan added a comment.Jan 22 2021, 10:06 AM

Build is green

builderr-debug-macos.txt
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libengine_dbg.a(precompiled.o) has no symbols
ld: warning: text-based stub file /System/Library/Frameworks//CoreAudio.framework/CoreAudio.tbd and library file /System/Library/Frameworks//CoreAudio.framework/CoreAudio are out of sync. Falling back to library file for linking.
ld: warning: text-based stub file /System/Library/Frameworks//AudioToolbox.framework/AudioToolbox.tbd and library file /System/Library/Frameworks//AudioToolbox.framework/AudioToolbox are out of sync. Falling back to library file for linking.
ld: warning: text-based stub file /System/Library/Frameworks//ForceFeedback.framework/ForceFeedback.tbd and library file /System/Library/Frameworks//ForceFeedback.framework/ForceFeedback are out of sync. Falling back to library file for linking.
ld: warning: text-based stub file /System/Library/Frameworks//CoreVideo.framework/Cor

See https://jenkins.wildfiregames.com/job/macos-differential/3016/display/redirect for more details.

Harbormaster completed remote builds in B15342: Diff 15619.Jan 22 2021, 10:10 AM
Stan added a comment.Jan 22 2021, 12:48 PM

LGTM. Works too.

This revision was not accepted when it landed; it landed in state Needs Review.Jan 22 2021, 1:50 PM
Closed by commit rP24762: Fix audio leak that resulted in openAL errors after a while. (authored by wraitii). · Explain Why
This revision was automatically updated to reflect the committed changes.
wraitii added a commit: rP24762: Fix audio leak that resulted in openAL errors after a while..

Revision Contents
Changeset List

PathSize
ps/
trunk/
source/
soundmanager/
items/
6 lines
6 lines
76 lines

Diff 15621

View Options

ps/trunk/source/soundmanager/items/CBufferItem.cpp

Loading...
View Options

ps/trunk/source/soundmanager/items/CSoundItem.cpp

Loading...
View Options

ps/trunk/source/soundmanager/items/CStreamItem.cpp

Loading...
Wildfire Games · Phabricator