From 3ae3a9fb722ad9f4754cf339c92aef9b8c915df3 Mon Sep 17 00:00:00 2001
From: Florent Rougon <f.rougon@frougon.net>
Date: Mon, 17 Oct 2022 17:37:49 +0200
Subject: [PATCH] SGSampleGroup::remove(): immediately remove from _samples by
 default

Code in AudioIdent::setIdent() expects that SGSampleGroup::remove() does
remove the corresponding item from _samples, however SG commit 47aae88ab
broke this assumption, which caused the bug reported at [1].

Restore the immediate removal from _samples by default. For the special
case where we do need delayed removal (i.e., while
SGSampleGroup::update() is iterating over _samples), the new optional
parameter is passed the value true. For a longer explanation, see [2].

This fixes a crash when swapping the selected and standby frequencies
for the NAV1 or NAV2 instrument (more precise description at [1]).

[1] https://forum.flightgear.org/viewtopic.php?f=25&t=40890
[2] https://sourceforge.net/p/flightgear/mailman/message/37721699/
---
 simgear/sound/sample_group.cxx | 32 +++++++++++++++++++-------------
 simgear/sound/sample_group.hxx |  5 ++++-
 2 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/simgear/sound/sample_group.cxx b/simgear/sound/sample_group.cxx
index 86843482..cff2408c 100644
--- a/simgear/sound/sample_group.cxx
+++ b/simgear/sound/sample_group.cxx
@@ -94,7 +94,8 @@ void SGSampleGroup::check_playing_sample(SGSoundSample *sample)
         sample->no_valid_source();
         _smgr->release_source( sample->get_source() );
         _smgr->release_buffer( sample );
-        remove( sample->get_sample_name() );
+        // Delayed removal because this is called while iterating over _samples
+        remove( sample->get_sample_name(), true );
     } else if ( sample->has_changed() ) {
         if ( !sample->is_playing() ) {
             // a request to stop playing the sound has been filed.
@@ -133,15 +134,10 @@ void SGSampleGroup::update( double dt ) {
         testForMgrError("update");
     }
 
-    for (auto current : _removed_samples) {
-        for (auto sample = _samples.begin(); sample != _samples.end(); ) {
-            if (sample->second == current) {
-                sample = _samples.erase(sample);
-            } else {
-                ++sample;
-            }
-        }
+    for (const auto& refname : _refsToRemoveFromSamplesMap) {
+        _samples.erase(refname);
     }
+    _refsToRemoveFromSamplesMap.clear();
 }
 
 // add a sound effect, return true if successful
@@ -160,8 +156,14 @@ bool SGSampleGroup::add( SGSharedPtr<SGSoundSample> sound,
 
 
 // remove a sound effect, return true if successful
-bool SGSampleGroup::remove( const std::string &refname ) {
-
+//
+// TODO: the 'delayedRemoval' boolean parameter has been specifically added
+// for check_playing_sample() which is only called while update() is iterating
+// over _samples. It may be better style to remove this parameter and add a
+// separate member function for use in check_playing_sample(), whose behavior
+// would be the same as when passing delayedRemoval=true here.
+bool SGSampleGroup::remove( const std::string &refname, bool delayedRemoval )
+{
     auto sample_it = _samples.find( refname );
     if ( sample_it == _samples.end() ) {
         // sample was not found
@@ -171,8 +173,12 @@ bool SGSampleGroup::remove( const std::string &refname ) {
     if ( sample_it->second->is_valid_buffer() )
         _removed_samples.push_back( sample_it->second );
 
-    // Do not erase within the loop
-//  _samples.erase( sample_it );
+    // Do not erase within the loop in update()
+    if (delayedRemoval) {
+        _refsToRemoveFromSamplesMap.push_back(refname);
+    } else {
+        _samples.erase(refname);
+    }
 
     return true;
 }
diff --git a/simgear/sound/sample_group.hxx b/simgear/sound/sample_group.hxx
index d4cbb30d..b45dbbec 100644
--- a/simgear/sound/sample_group.hxx
+++ b/simgear/sound/sample_group.hxx
@@ -91,9 +91,11 @@ public:
     /**
      * Remove an audio sample from this group.
      * @param refname Reference name of the audio sample to remove
+     * @param delayedRemoval If true, don't modify _samples before the main
+     *        loop in update() is finished
      * @return return true if successful
      */
-    bool remove( const std::string& refname );
+    bool remove( const std::string& refname, bool delayedRemoval = false );
 
     /**
      * Test if a specified audio sample is registered at this sample group
@@ -240,6 +242,7 @@ private:
 
     sample_map _samples;
     std::vector< SGSharedPtr<SGSoundSample> > _removed_samples;
+    std::vector<std::string> _refsToRemoveFromSamplesMap;
 
     bool testForMgrError(std::string s);
     bool testForError(void *p, std::string s);