Merge pull request #8273 from Robyt3/Sound-Various-Fixes

Fix crashes and memory leaks related to editor map sounds and opus file decoding, refactoring

src/engine/client/sound.cpp

@@ -292,14 +292,15 @@ CSample *CSound::AllocSample()
 		return nullptr;
 
 	CSample *pSample = &m_aSamples[m_FirstFreeSampleIndex];
-	m_FirstFreeSampleIndex = pSample->m_NextFreeSampleIndex;
-	pSample->m_NextFreeSampleIndex = SAMPLE_INDEX_USED;
-	if(pSample->m_pData != nullptr)
+	if(pSample->m_pData != nullptr || pSample->m_NextFreeSampleIndex == SAMPLE_INDEX_USED)
 	{
-		char aError[64];
-		str_format(aError, sizeof(aError), "Sample was not unloaded (index=%d, duration=%f)", pSample->m_Index, pSample->TotalTime());
+		char aError[128];
+		str_format(aError, sizeof(aError), "Sample was not unloaded (index=%d, next=%d, duration=%f, data=%p)",
+			pSample->m_Index, pSample->m_NextFreeSampleIndex, pSample->TotalTime(), pSample->m_pData);
 		dbg_assert(false, aError);
 	}
+	m_FirstFreeSampleIndex = pSample->m_NextFreeSampleIndex;
+	pSample->m_NextFreeSampleIndex = SAMPLE_INDEX_USED;
 	return pSample;
 }
 
@@ -341,29 +342,36 @@ void CSound::RateConvert(CSample &Sample) const
 
 bool CSound::DecodeOpus(CSample &Sample, const void *pData, unsigned DataSize) const
 {
-	OggOpusFile *pOpusFile = op_open_memory((const unsigned char *)pData, DataSize, nullptr);
+	int OpusError = 0;
+	OggOpusFile *pOpusFile = op_open_memory((const unsigned char *)pData, DataSize, &OpusError);
 	if(pOpusFile)
 	{
 		const int NumChannels = op_channel_count(pOpusFile, -1);
-		const int NumSamples = op_pcm_total(pOpusFile, -1); // per channel!
-
-		Sample.m_Channels = NumChannels;
-
-		if(Sample.m_Channels > 2)
+		if(NumChannels > 2)
 		{
+			op_free(pOpusFile);
 			dbg_msg("sound/opus", "file is not mono or stereo.");
 			return false;
 		}
 
-		Sample.m_pData = (short *)calloc((size_t)NumSamples * NumChannels, sizeof(short));
+		const int NumSamples = op_pcm_total(pOpusFile, -1); // per channel!
+		if(NumSamples < 0)
+		{
+			op_free(pOpusFile);
+			dbg_msg("sound/opus", "failed to get number of samples, error %d", NumSamples);
+			return false;
+		}
+
+		short *pSampleData = (short *)calloc((size_t)NumSamples * NumChannels, sizeof(short));
 
 		int Pos = 0;
 		while(Pos < NumSamples)
 		{
-			const int Read = op_read(pOpusFile, Sample.m_pData + Pos * NumChannels, NumSamples * NumChannels, nullptr);
+			const int Read = op_read(pOpusFile, pSampleData + Pos * NumChannels, (NumSamples - Pos) * NumChannels, nullptr);
 			if(Read < 0)
 			{
-				free(Sample.m_pData);
+				free(pSampleData);
+				op_free(pOpusFile);
 				dbg_msg("sound/opus", "op_read error %d at %d", Read, Pos);
 				return false;
 			}
@@ -372,15 +380,19 @@ bool CSound::DecodeOpus(CSample &Sample, const void *pData, unsigned DataSize) c
 			Pos += Read;
 		}
 
+		op_free(pOpusFile);
+
+		Sample.m_pData = pSampleData;
 		Sample.m_NumFrames = Pos;
 		Sample.m_Rate = 48000;
+		Sample.m_Channels = NumChannels;
 		Sample.m_LoopStart = -1;
 		Sample.m_LoopEnd = -1;
 		Sample.m_PausedAt = 0;
 	}
 	else
 	{
-		dbg_msg("sound/opus", "failed to decode sample");
+		dbg_msg("sound/opus", "failed to decode sample, error %d", OpusError);
 		return false;
 	}
 
@@ -459,10 +471,7 @@ bool CSound::DecodeWV(CSample &Sample, const void *pData, unsigned DataSize) con
 		const unsigned int SampleRate = WavpackGetSampleRate(pContext);
 		const int NumChannels = WavpackGetNumChannels(pContext);
 
-		Sample.m_Channels = NumChannels;
-		Sample.m_Rate = SampleRate;
-
-		if(Sample.m_Channels > 2)
+		if(NumChannels > 2)
 		{
 			dbg_msg("sound/wv", "file is not mono or stereo.");
 			s_pWVBuffer = nullptr;
@@ -498,6 +507,8 @@ bool CSound::DecodeWV(CSample &Sample, const void *pData, unsigned DataSize) con
 #endif
 
 		Sample.m_NumFrames = NumSamples;
+		Sample.m_Rate = SampleRate;
+		Sample.m_Channels = NumChannels;
 		Sample.m_LoopStart = -1;
 		Sample.m_LoopEnd = -1;
 		Sample.m_PausedAt = 0;

src/game/editor/editor.cpp

@@ -1379,12 +1379,17 @@ void CEditor::DoToolbarSounds(CUIRect ToolBar)
 		if(pSelectedSound->m_SoundId != m_ToolbarPreviewSound && m_ToolbarPreviewSound >= 0 && Sound()->IsPlaying(m_ToolbarPreviewSound))
 			Sound()->Stop(m_ToolbarPreviewSound);
 		m_ToolbarPreviewSound = pSelectedSound->m_SoundId;
+	}
+	else
+	{
+		m_ToolbarPreviewSound = -1;
+	}
 
+	if(m_ToolbarPreviewSound >= 0)
+	{
 		static int s_PlayPauseButton, s_StopButton, s_SeekBar = 0;
 		DoAudioPreview(ToolBarBottom, &s_PlayPauseButton, &s_StopButton, &s_SeekBar, m_ToolbarPreviewSound);
 	}
-	else
-		m_ToolbarPreviewSound = -1;
 }
 
 static void Rotate(const CPoint *pCenter, CPoint *pPoint, float Rotation)
@@ -8705,11 +8710,10 @@ void CEditor::OnClose()
 
 void CEditor::OnDialogClose()
 {
-	if(m_FilePreviewSound >= 0)
-	{
-		Sound()->UnloadSample(m_FilePreviewSound);
-		m_FilePreviewSound = -1;
-	}
+	Graphics()->UnloadTexture(&m_FilePreviewImage);
+	Sound()->UnloadSample(m_FilePreviewSound);
+	m_FilePreviewSound = -1;
+	m_FilePreviewState = PREVIEW_UNLOADED;
 }
 
 void CEditor::LoadCurrentMap()

src/game/editor/mapitems/sound.h

@@ -10,7 +10,7 @@ public:
 	explicit CEditorSound(CEditor *pEditor);
 	~CEditorSound();
 
-	int m_SoundId = 0;
+	int m_SoundId = -1;
 	char m_aName[IO_MAX_PATH_LENGTH] = "";
 
 	void *m_pData = nullptr;

src/game/server/gamecontext.cpp

@@ -3068,7 +3068,7 @@ void CGameContext::ConPause(IConsole::IResult *pResult, void *pUserData)
 void CGameContext::ConChangeMap(IConsole::IResult *pResult, void *pUserData)
 {
 	CGameContext *pSelf = (CGameContext *)pUserData;
-	pSelf->m_pController->ChangeMap(pResult->NumArguments() ? pResult->GetString(0) : "");
+	pSelf->m_pController->ChangeMap(pResult->GetString(0));
 }
 
 void CGameContext::ConRandomMap(IConsole::IResult *pResult, void *pUserData)
@@ -3513,7 +3513,7 @@ void CGameContext::OnConsoleInit()
 	Console()->Register("mapbug", "s[mapbug]", CFGFLAG_SERVER | CFGFLAG_GAME, ConMapbug, this, "Enable map compatibility mode using the specified bug (example: grenade-doubleexplosion@ddnet.tw)");
 	Console()->Register("switch_open", "i[switch]", CFGFLAG_SERVER | CFGFLAG_GAME, ConSwitchOpen, this, "Whether a switch is deactivated by default (otherwise activated)");
 	Console()->Register("pause_game", "", CFGFLAG_SERVER, ConPause, this, "Pause/unpause game");
-	Console()->Register("change_map", "?r[map]", CFGFLAG_SERVER | CFGFLAG_STORE, ConChangeMap, this, "Change map");
+	Console()->Register("change_map", "r[map]", CFGFLAG_SERVER | CFGFLAG_STORE, ConChangeMap, this, "Change map");
 	Console()->Register("random_map", "?i[stars]", CFGFLAG_SERVER, ConRandomMap, this, "Random map");
 	Console()->Register("random_unfinished_map", "?i[stars]", CFGFLAG_SERVER, ConRandomUnfinishedMap, this, "Random unfinished map");
 	Console()->Register("restart", "?i[seconds]", CFGFLAG_SERVER | CFGFLAG_STORE, ConRestart, this, "Restart in x seconds (0 = abort)");