bugfix(milesaudiomanager): Prevent dangling pointer to AudioEventRTS in PlayingAudio when handing it over to a new AudioRequest after a call to MilesAudioManager::startNextLoop() - #2774
Conversation
xezon
added
Audio
|
| Filename | Overview |
|---|---|
| Core/GameEngine/Include/Common/AudioEventRTS.h | Makes dynamic audio events reference-counted while preserving the reference count during event-data assignment. |
| Core/GameEngine/Include/Common/AudioRequest.h | Replaces conditional raw event ownership with an independently stored reference-counted pending event. |
| Core/GameEngineDevice/Include/MilesAudioDevice/MilesAudioManager.h | Gives playing audio shared event ownership and adds a main-update loop-rerequest signal. |
| Core/GameEngineDevice/Source/MilesAudioDevice/MilesAudioManager.cpp | Transfers delayed-loop request creation to the main update, retains events through handoff, and correctly matches pending requests during immediate cancellation. |
| GeneralsMD/Code/GameEngine/Include/Common/ThingTemplate.h | Converts template audio storage to reference-counted pointers without copying live reference counts. |
| Core/GameEngine/Source/Common/Audio/GameAudio.cpp | Allocates dynamic audio events under reference-counted ownership and updates request lifecycle handling. |
Sequence Diagram
sequenceDiagram
participant Timer as Miles timer thread
participant Playing as PlayingAudio
participant Main as Main audio update
participant Queue as AudioRequest queue
Timer->>Playing: mark rerequestOnNextUpdate
Timer->>Playing: transition Playing to Stopping
Main->>Playing: observe Stopping
Main->>Queue: enqueue shared DynamicAudioEventRTS
Main->>Playing: transition Stopping to Stopped
Queue->>Queue: process delayed loop request
Reviews (15): Last reviewed commit: "bugfix(milesaudiomanager): Prevent dangl..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
c9bfbb0 to
049a95b
Compare
|
I revisited the implementation and added new fixup commits to simplify it, because I noticed that we can avoid adding the deferred audio requests container by using a new flag in The RefCountMTClass is now no longer used, but we can keep it anyway for future use cases. |
730b739 to
cb3b9b4
Compare
|
Polished. Ready for review. |
|
I've been debugging this PR, and it sometimes softlocks when I exit near the factory and town in Alpine Assault. I didn't see anything strange in the debugger, but having a heightened camera increases the chances that it hangs. I think I found a race condition that explains it. |
|
|
||
| if (playing->m_rerequestOnNextUpdate) { | ||
| rerequestPlayingAudio(playing); | ||
| playing->m_rerequestOnNextUpdate = false; |
There was a problem hiding this comment.
Since processPlayingList() iterates without a lock, could a Miles background callback be modifying this object's state at the exact same time we read/overwrite it?
There was a problem hiding this comment.
I don't think so, because this bool is set true only in that miles callback and the miles callback will not be called again.
Break the debugger to see the callstack and where it hangs. |
|
I don't see where Regardless, there are some thread safety issues with that class so I'd like to see that extracted to a separate PR. |
|
I did use RefCountMTClass before. Then simplified code and it was no longer needed. I left it because it might be needed in future. I can remove it again. |
Fair enough.
Please remove it from this PR at least. A reference counting class is quite tricky stuff for multi-threaded purposes. It should be added in a separate PR if we ever need it. |
cb3b9b4 to
ad3b52e
Compare
First commit removed. |
ad3b52e to
9c94e13
Compare
9c94e13 to
6fd2dbb
Compare
xezon
changed the title
0ed037e to
7b96d91
Compare
|
The bot had a lot of complaints here about my code. Pffft. |
7b96d91 to
e4ab4ea
Compare
|
Needs review |
I'll do my best to review it some time this week. It's not an easy one to review. |
e4ab4ea to
efc2946
Compare
|
Rebased and merge conflicts resolved. |
Mauller
left a comment
Mauller
left a comment
There was a problem hiding this comment.
Looks okay overall apart from what Bobtista brought up, just some small things.
I had to stop reviewing for now since Github started breaking in weird ways.
070d109 to
d254f25
Compare
Mauller
left a comment
Mauller
left a comment
There was a problem hiding this comment.
I don't see any glaring problems so i believe this is okay.
|
There are a lot of merge conflicts in drawable around the sound. Looks like that needs merging a bit first. |
…in PlayingAudio when handing it over to a new AudioRequest after a call to MilesAudioManager::startNextLoop() (#2774)
d254f25 to
d395db7
Compare
|
Replicated to Generals with 1 minor conflict |
5 participants
Merge with Rebase
This change has 2 commits to work towards fixing race conditions in
MilesAudioManagerconcerning a sharedAudioEventRTSinstance in classesAudioRequestandPlayingAudio.The first commit implements a newRefCountMTClasswhich is fundamentally identical toRefCountClass, except it has a thread safe counter and all the debug functionality is omitted.The first commit adds the
RefCountMTClassRefCountClasstoDynamicAudioEventRTSto allow for shared ownership. All existing users ofDynamicAudioEventRTSaccomodate it and will now useRefCountPtrfor automatic reference counting.The second commit replaces
AudioEventRTS*withRefCountPtr<DynamicAudioEventRTS>inAudioRequestandPlayingAudioto allow sharing the audio event data between them. This is needed, because ownership will be shared in functionMilesAudioManager::startNextLoop(orMilesAudioManager::stopPlayingAudio), where previouslyAudioRequestwas given the sole authority to delete theAudioEventRTSwhilePlayingAudiostill kept a pointer to it. Now both classes need to release their reference count before the audio event data is deleted.This likely was also a problem in retail, because
AudioEventRTSis heap allocated, not pool allocated.TODO