Announcement

Collapse
No announcement yet.

Calling 'Stop()' on a looping UAudioComponent doesn't always work.

Collapse
X
  • Filter
  • Time
  • Show
Clear All
new posts

    [PROGRAMMING] Calling 'Stop()' on a looping UAudioComponent doesn't always work.

    This is a really frustrating bug that I'm having major trouble tracking down.

    I have an Audio Component that is supposed to play a Looping SoundWave until I tell it to stop. Now if I don't put a breakpoint in in Visual Studio, it doesn't work - but if I do put the breakpoint in, it does. I can only assume that the issue is caused by some possible multithreading issue and the extra time it takes to trigger the breakpoint makes it work - but to be honest I'm clutching at straws here.

    This seems to occur mostly when playing 2D sounds, I'm re-using a single Audio Component to play different sound files (one is a loop, and the other is the 'End Loop' sound). When a counter in the UI starts of stops counting, I call this:

    Code:
    void UGESGame_MissionComplete::OnCounterStart()
    {
    	ActiveProgressSound->SetSound(ProgressSound);
    	ActiveProgressSound->Play();
    }
    
    void UGESGame_MissionComplete::OnCounterEnd()
    {
    	ActiveProgressSound->Stop();
    	ActiveProgressSound->SetSound(ProgressEndSound);
    	ActiveProgressSound->Play();
    }
    And this is how I create that AudioComponent.

    Code:
    void UGESGame_MissionComplete::NativeConstruct()
    {
    	Super::NativeConstruct();
    
    	/* Create the Sound Effect */
    	ActiveProgressSound = UGameplayStatics::CreateSound2D(GetOwningPlayer(), ProgressSound);
    	ActiveProgressSound->Stop();
    }
    I haven't tried just creating two audio components and stop/starting them - but either way this strikes me as a bug. SetSound() actually calls 'Stop' as well, so it's possible that FActiveSounds are being left in the active sound list and not actually removed, and have no owner. If those sounds are looping they stay there forever which is a pain.

    I brought this issue up a while ago on Answerhub

    Also paging [MENTION=36447]Minus_Kelvin[/MENTION]

    Edit: An update, it appears I can stop the looping sound once - but never again after that. It's stays playing for eternity.

    Edit #2: FINALLY figured out where the problem is caused. It's in SoundConcurrency.cpp

    Code:
    void FSoundConcurrencyManager::RemoveActiveSound(FActiveSound* ActiveSound)
    {
    	if (!ActiveSound->ConcurrencyGroupID)
    	{
    		return;
    	}
    }
    So basically, if you don't give the sound a concurrency class, it won't be removed from the concurrency manager and will stay there - even though it's perfectly valid to create a sound without a concurrency group.

    This is probably a memory leak situation too, I suspect that any non-looping sounds have the same issue - but they'll just be left in the concurrency manager and never cleaned up, despite the fact that they're not playing anything.
    Last edited by TheJamsh; 10-10-2016, 03:35 PM.

    #2
    I'm not sure where the bug is, but it's probably not in sound concurrency manager. The rest of the function in the code you quoted is:

    Code:
    void FSoundConcurrencyManager::RemoveActiveSound(FActiveSound* ActiveSound)
    {
    	if (!ActiveSound->ConcurrencyGroupID)
    	{
    		return;
    	}
    
    	// Remove this sound from it's concurrency list
    	FConcurrencyGroup* ConcurrencyGroup = ConcurrencyGroups.Find(ActiveSound->ConcurrencyGroupID);
    	check(ConcurrencyGroup);
    
    	TArray<FActiveSound*>& ActiveSounds = ConcurrencyGroup->GetActiveSounds();
    	check(ActiveSounds.Num() > 0);
    	ActiveSounds.Remove(ActiveSound);
    
    }
    In other words the active sound that's getting removed is the active sound in the concurrency group. The corresponding add function is:

    Code:
    void FConcurrencyGroup::AddActiveSound(FActiveSound* ActiveSound)
    {
    	check(ConcurrencyGroupID != 0);
    	ActiveSounds.Add(ActiveSound);
    	ActiveSound->ConcurrencyGroupID = ConcurrencyGroupID;
    	ActiveSound->ConcurrencyGeneration = Generation++;
    }
    Playing a sound without a concurrency group will simply bypass the check in the beginning of a playsound call's life in:

    Code:
    void FAudioDevice::AddNewActiveSound(const FActiveSound& NewActiveSound)
    There, before a sound is allowed to create an FActiveSound, it checks the concurrency manager in FSoundConcurrencyManager::CreateNewActiveSound:

    Code:
    FActiveSound* FSoundConcurrencyManager::CreateNewActiveSound(const FActiveSound& NewActiveSound)
    {
    	check(NewActiveSound.GetSound());
    	
    	// If there are no concurrency settings associated then there is no limit on this sound
    	const FSoundConcurrencySettings* Concurrency = NewActiveSound.GetSoundConcurrencySettingsToApply();
    
    	// If there was no concurrency or the setting was zero, then we will always play this sound.
    	if (!Concurrency)
    	{
    		FActiveSound* ActiveSound = new FActiveSound(NewActiveSound);
    		ActiveSound->SetAudioDevice(AudioDevice);
    		return ActiveSound;
    	}
    
    	check(Concurrency->MaxCount > 0);
    
    	uint32 ConcurrencyObjectID = NewActiveSound.GetSoundConcurrencyObjectID();
    	if (ConcurrencyObjectID == 0)
    	{
    		return HandleConcurrencyEvaluationOverride(NewActiveSound);
    	}
    	else
    	{
    		return HandleConcurrencyEvaluation(NewActiveSound);
    	}
    }
    Note that if there is no concurrency settings on the sound, it simply creates a new active sound and returns that. No concurrency evaluation, no adding to a concurrency group, etc.

    When an active sound finishes, FAudioDevice::RemoveActiveSound is called:

    Code:
    void FAudioDevice::RemoveActiveSound(FActiveSound* ActiveSound)
    {
    	check(IsInAudioThread());
    
    	ConcurrencyManager.RemoveActiveSound(ActiveSound);
    
    	// Perform the notification
    	if (ActiveSound->GetAudioComponentID() > 0)
    	{
    		UAudioComponent::PlaybackCompleted(ActiveSound->GetAudioComponentID(), false);
    	}
    
    	const int32 NumRemoved = ActiveSounds.Remove(ActiveSound);
    	check(NumRemoved == 1);
    }
    Notice that the concurrency manager is called (the function that you suspect is a leak/bug) here. But we've seen that if an active sound doesn't have a concurrency group, that function does nothing. Then it performs a notification on the audio component that it's done (if it has an audio component). Then it actually removes the active sound from the real list in FAudioDevice.
    The "ActiveSounds" member of FAudioDevice is the one that prevents the leak and is the list that is evaluated every frame to determine WaveInstances (and does the evaluation of sound cue graphs, etc) in FAudioDevice::GetSortedActiveWaveInstances.

    Edit:

    For completeness, since the above implies that RemoveActiveSound is intended to be the final clean up and clearly there hasn't been a delete ActiveSound call. The active sound lifetime is finished in:

    Code:
    void FAudioDevice::ProcessingPendingActiveSoundStops(bool bForceDelete)
    {
    	// Process the PendingSoundsToDelete. These may have 
    	// had their deletion deferred due to an async operation
    	for (int32 i = PendingSoundsToDelete.Num() - 1; i >= 0; --i)
    	{
    		FActiveSound* ActiveSound = PendingSoundsToDelete[i];
    		if (bForceDelete || ActiveSound->CanDelete())
    		{
    			ActiveSound->bAsyncOcclusionPending = false;
    			PendingSoundsToDelete.RemoveAtSwap(i, 1, false);
    			delete ActiveSound;
    		}
    	}
    
    	// Stop any pending active sounds that need to be stopped
    	for (FActiveSound* ActiveSound : PendingSoundsToStop)
    	{
    		check(ActiveSound);
    		ActiveSound->Stop();
    
    		// If we can delete the active sound now, then delete it
    		if (bForceDelete || ActiveSound->CanDelete())
    		{
    			ActiveSound->bAsyncOcclusionPending = false;
    			delete ActiveSound;
    		}
    		else
    		{
    			// There was an async operation pending. We need to defer deleting this sound
    			PendingSoundsToDelete.Add(ActiveSound);
    		}
    	}
    	PendingSoundsToStop.Reset();
    }
    Basically, what happens is that when an active sound is called to stop, it doesn't immediately stop it due to a number of reasons. But instead appends the ActiveSounds to a Pending list:

    Code:
    void FAudioDevice::AddSoundToStop(FActiveSound* SoundToStop)
    {
    	check(IsInAudioThread());
    
    	const uint64 AudioComponentID = SoundToStop->GetAudioComponentID();
    	if (AudioComponentID > 0)
    	{
    		AudioComponentIDToActiveSoundMap.Remove(AudioComponentID);
    	}
    
    	check(SoundToStop);
    	bool bIsAlreadyInSet = false;
    	PendingSoundsToStop.Add(SoundToStop, &bIsAlreadyInSet);
    	if (bIsAlreadyInSet)
    	{
    		UE_LOG(LogAudio, Verbose, TEXT("Stopping sound which was already in the process of stopping"));
    	}
    }

    Which is then processed in the above function. ActiveSound->Stop() is the point at which it's removed from the internal list of ActiveSounds. The primary reason why we might not be able to delete the ActiveSound (why we need a PendingSoundsToDelete list) is if there is an async trace call pending for occlusion. The primary reason why we can't immediately stop an active sound when its stopped is to prevent stack overflows with BP delegate functions which attempt to play more sounds in the same concurrency group of limit 1, which immediately trigger a stop, which trigger another play call, which trigger a stop, etc. I discovered that bug when I first implemented concurrency. It was a bug that had actually been possible before the new sound concurrency group feature.
    Last edited by Minus_Kelvin; 10-10-2016, 05:59 PM.

    Comment


      #3
      Your post doesn't mention which version of UE4 you're using.

      There was a few bugs fixed in 4.13 which deal with playing audio components where the reference count wasn't correct and sounds were getting orphaned. Your use-case should be ok, though, as you said, you don't need to call Stop in any of your cases. Using CreateSound2D specifically doesn't call Play on the audio component, so no need to call Stop. And SetSound does stop it.

      If you're using an older version, I think using different audio components should fix your issue.

      The code to check is void UAudioComponent::PlaybackCompleted(bool bFailedToStart). Make sure that it's behaving as you expect. Also make sure that the Warning isn't getting triggered in UAudioComponent::BeginDestroy():

      Code:
      void UAudioComponent::BeginDestroy()
      {
      	Super::BeginDestroy();
      
      	if (bIsActive && Sound && Sound->IsLooping())
      	{
      		UE_LOG(LogAudio, Warning, TEXT("Audio Component is being destroyed without stopping looping sound '%s'"), *Sound->GetName());
      		Stop();
      	}
      
      	AudioIDToComponentMap.Remove(AudioComponentID);
      }
      EDIT:

      That said, there is a bit of weirdness with SetSound:

      Code:
      void UAudioComponent::SetSound( USoundBase* NewSound )
      {
      	const bool bPlay = IsPlaying();
      
      	// If this is an auto destroy component we need to prevent it from being auto-destroyed since we're really just restarting it
      	const bool bWasAutoDestroy = bAutoDestroy;
      	bAutoDestroy = false;
      	Stop();
      	bAutoDestroy = bWasAutoDestroy;
      
      	Sound = NewSound;
      
      	if (bPlay)
      	{
      		Play();
      	}
      }
      So, SetSound will re-trigger a play if the audio component is already playing. If it's not already playing, it won't call Play(). The idea is you can swap out a sound quickly without needing to deal with state. However, in your case, if you don't know if an audio component is going be playing at all when you call SetSound, and you want to prevent calling Play 2x in quick succession, which is probably not a good idea, you'll have to query IsPlaying when you call SetSound.

      Code:
      void UGESGame_MissionComplete::OnCounterStart()
      {
      	ActiveProgressSound->SetSound(ProgressSound);
      	if (!ActiveProgressSound->IsPlaying())
      	{
      		ActiveProgressSound->Play();
      	}
      }
      
      void UGESGame_MissionComplete::OnCounterEnd()
      {
      	ActiveProgressSound->SetSound(ProgressEndSound);
      	if (!ActiveProgressSound->IsPlaying())
      	{
      		ActiveProgressSound->Play();
      	}
      }
      Last edited by Minus_Kelvin; 10-10-2016, 06:11 PM.

      Comment


        #4
        I just tried to repro your use-case in BP (you just used BP functions, so theoretically the behavior should be the same) and wasn't able to get any orphaned looping sounds.

        This is my BP:



        If both sounds (the counter progress sound and the finish sound) are both looping, then the above results in a constantly iterating progress sound and progress finished sound. If the finish sound is a one-shot (which I'm guessing is a reasonable thing since you probably don't know when the thing you're counting down is going to start again), then this fails. The reason it fails is the final one-shot ends up cleaning up the audio component (by GC). In that case you'll need to create a NEW audio component for the next time around.

        The problem, in general, with re-using audio components is that it's confusing their intended usage. Audio components are best thought of as handles to a playing sound. Once a sound finishes, it triggers the garbage collector. Its best to re-create audio components for each instance of a sound. In your case, I'd create an audio component on counter start, set the sound on counter end to the one-shot and let the audio component get GC'd. Then next time you go through the process, make a new audio component.

        Edit:

        And finally, this should be the BP (or C++ code) you use for your use case that allows you to have a one-shot OnCounterEnd and properly creates a new audio component if it's invalid (i.e. was GC'd).

        Click image for larger version

Name:	bp_loop2.PNG
Views:	1
Size:	275.3 KB
ID:	1116477

        Note that I also removed unnecessary audio component API calls.

        Edit 2:

        Sorry, to be super-safe, you probably should also check if the audio component is still valid on counter end. Depending on the use-case of the sound in question, there are a large number of ways a sound can get stopped... your counter progress loop sound could possibly be stopped mid-progress. If you want to be sure to always have a good chance to play the counter-end sound, you should probably check if the audio component is valid before calling SetSound on it. If you're fairly confident the looping sound will still be valid by the time OnCounterEnd is called (you've got the sound set at high priority and it's a 2D UI sound, etc), then by all means, just use SetSound.
        Last edited by Minus_Kelvin; 10-10-2016, 07:25 PM.

        Comment


          #5
          [MENTION=36447]Minus_Kelvin[/MENTION] thanks for digging in, I only went as far as I did and experimented by giving the component a concurrency group - and that fixed the issue for me so I assumed that was the issue.

          In terms of reusing audio components etc, I do it in quite a lot of my projects to prevent creating lots of scene components in short succession or suchlike and just leaving things around to be GC'd, but I'm wondering if there's anything to be said for just working directly with FActiveSound's instead?

          Comment


            #6
            Apologies for the length of the response! Probably got carried away.

            So, the only reason you really need an audio component is if you need a handle to a sound. That means if you need to stop it prematurely, change its volume, pitch, it's position, etc. Unfortunately, also there isn't a way to play a non-audio component sound that is attached to another actor (I'd like to add that as a feature).

            But in other cases you don't need an audio component, use the PlaySound* flavor of BP static functions. PlaySound2D and PlaySoundAtLocation (should probably be called PlaySound3D). Or you can just directly create an ActiveSound and add it to the audio device like other places in the code base.

            Comment


              #7
              That's interesting... for some reason I always assumed that the audio component was neccesary for spatialization etc. Might be room for further optimization on some of my project... hmm


              EDIT: [MENTION=36447]Minus_Kelvin[/MENTION] - any idea why giving it a concurrency class fixed the looping thing? I'm glad it fixed the issue ofc but my mind is wondering now...
              Last edited by TheJamsh; 10-13-2016, 12:34 PM.

              Comment


                #8
                Not sure. Depends on what the concurrency settings were. I actually wasn't able to reproduce your problem with my local code. I never found out what version of the engine you are using, so there may be a fixed bug. I recall that there were some ref-counting issues fixed with audio components relatively recently. I'd have to dig into my P4 CL history.

                For spatialization, all you need is for the sound to play with an "attenuation settings" that has 3d spatialization enabled. Also, keep in mind you can 3d-attenuate without panning (angular spatialization) and you can spatialize (pan) without attenuation when using attenuation structs. The only thing you can't do (right now) without an audio component is automatically have the sound track alongside another object (attached). Once you PlaySoundAtLocation, it stays at that location for the duration of the sound.

                Comment

                Working...
                X