Merge pull request #192 from WaywardHeart/audio-segfault

Fix a segfault in audio streams

src/audio/aldatasource.h

@@ -56,8 +56,7 @@ struct ALDataSource
 ALDataSource *createSDLSource(SDL_RWops &ops,
                               const char *extension,
 			                  uint32_t maxBufSize,
-			                  bool looped,
-			                  int fallbackMode);
+			                  bool looped);
 
 ALDataSource *createVorbisSource(SDL_RWops &ops,
                                  bool looped);

src/audio/alstream.cpp

@@ -90,18 +90,7 @@ void ALStream::close()
 
 void ALStream::open(const std::string &filename)
 {
-	checkStopped();
-
-	switch (state)
-	{
-	case Playing:
-	case Paused:
-		stopStream();
-	case Stopped:
-		closeSource();
-	case Closed:
-		openSource(filename);
-	}
+	openSource(filename);
 
 	state = Stopped;
 }
@@ -201,32 +190,26 @@ void ALStream::closeSource()
 
 struct ALStreamOpenHandler : FileSystem::OpenHandler
 {
-	SDL_RWops *srcOps;
 	bool looped;
 	ALDataSource *source;
-	int fallbackMode;
 	std::string errorMsg;
 
-	ALStreamOpenHandler(SDL_RWops &srcOps, bool looped)
-	    : srcOps(&srcOps), looped(looped), source(0), fallbackMode(0)
+	ALStreamOpenHandler(bool looped)
+	    : looped(looped), source(0)
 	{}
 
 	bool tryRead(SDL_RWops &ops, const char *ext)
 	{
-		/* Copy this because we need to keep it around,
-		 * as we will continue reading data from it later */
-		*srcOps = ops;
-
 		/* Try to read ogg file signature */
 		char sig[5] = { 0 };
-		SDL_RWread(srcOps, sig, 1, 4);
-		SDL_RWseek(srcOps, 0, RW_SEEK_SET);
+		SDL_RWread(&ops, sig, 1, 4);
+		SDL_RWseek(&ops, 0, RW_SEEK_SET);
 
 		try
 		{
 			if (!strcmp(sig, "OggS"))
 			{
-				source = createVorbisSource(*srcOps, looped);
+				source = createVorbisSource(ops, looped);
 				return true;
 			}
 
@@ -236,12 +219,12 @@ struct ALStreamOpenHandler : FileSystem::OpenHandler
 
 				if (HAVE_FLUID)
 				{
-					source = createMidiSource(*srcOps, looped);
+					source = createMidiSource(ops, looped);
 					return true;
 				}
 			}
 
-			source = createSDLSource(*srcOps, ext, STREAM_BUF_SIZE, looped, fallbackMode);
+			source = createSDLSource(ops, ext, STREAM_BUF_SIZE, looped);
 		}
 		catch (const Exception &e)
 		{
@@ -257,21 +240,24 @@ struct ALStreamOpenHandler : FileSystem::OpenHandler
 
 void ALStream::openSource(const std::string &filename)
 {
-	ALStreamOpenHandler handler(srcOps, looped);
-	shState->fileSystem().openRead(handler, filename.c_str());
-	source = handler.source;
-	needsRewind.clear();
-
-	// Try fallback mode, e.g. for handling S32->F32 sample format conversion
-	if (!source)
+	ALStreamOpenHandler handler(looped);
+	try
 	{
-		handler.fallbackMode = 1;
 		shState->fileSystem().openRead(handler, filename.c_str());
-		source = handler.source;
-		needsRewind.clear();
+	} catch (const Exception &e)
+	{
+		/* If no file was found then we leave the stream open.
+		 * A PHYSFSError means we found a match but couldn't
+		 * open the file, so we'll close it in that case. */
+		if (e.type != Exception::NoFileError)
+			close();
+		
+		throw e;
 	}
 
-	if (!source)
+	close();
+
+	if (!handler.source)
 	{
 		char buf[512];
 		snprintf(buf, sizeof(buf), "Unable to decode audio stream: %s: %s",
@@ -279,6 +265,9 @@ void ALStream::openSource(const std::string &filename)
 
 		Debug() << buf;
 	}
+	
+	source = handler.source;
+	needsRewind.clear();
 }
 
 void ALStream::stopStream()

src/audio/alstream.h

@@ -74,8 +74,6 @@ struct ALStream
 	uint64_t procFrames;
 	AL::Buffer::ID lastBuf;
 
-	SDL_RWops srcOps;
-
 	struct
 	{
 		ALenum format;

src/audio/audiostream.cpp

@@ -113,31 +113,26 @@ void AudioStream::play(const std::string &filename,
 	/* Requested audio file is different from current one */
 	bool diffFile = (filename != current.filename);
 
-	switch (sState)
+	if (diffFile || sState == ALStream::Closed)
 	{
-	case ALStream::Paused :
-	case ALStream::Playing :
-		stream.stop();
-	case ALStream::Stopped :
-		if (diffFile)
-			stream.close();
-	case ALStream::Closed :
-		if (diffFile)
+		try
 		{
-			try
-			{
-				/* This will throw on errors while
-				 * opening the data source */
-				stream.open(filename);
-			}
-			catch (const Exception &e)
-			{
-				unlockStream();
-				throw e;
-			}
+			/* This will throw on errors while
+			 * opening the data source */
+			stream.open(filename);
+		}
+		catch (const Exception &e)
+		{
+			unlockStream();
+			throw e;
+		}
+	} else {
+		switch (sState)
+		{
+			case ALStream::Paused :
+			case ALStream::Playing :
+				stream.stop();
 		}
-
-		break;
 	}
 
 	setVolume(Base, _volume);

src/audio/midisource.cpp

@@ -631,15 +631,9 @@ struct MidiSource : ALDataSource, MidiReadHandler
 			throw Exception(Exception::MKXPError, "Reading midi data failed");
 		}
 
-		try
-		{
-			readMidi(this, data);
-		}
-		catch (const Exception &)
-		{
-			SDL_RWclose(&ops);
-			throw;
-		}
+		SDL_RWclose(&ops);
+
+		readMidi(this, data);
 
 		synth = shState->midiState().allocateSynth();
 

src/audio/sdlsoundsource.cpp

@@ -24,10 +24,15 @@
 
 #include <SDL_sound.h>
 
+static int SDL_RWopsCloseNoop(SDL_RWops *ops) {
+	return 0;
+}
+
 struct SDLSoundSource : ALDataSource
 {
 	Sound_Sample *sample;
-	SDL_RWops &srcOps;
+	SDL_RWops srcOps;
+	SDL_RWops unclosableOps;
 	uint8_t sampleSize;
 	bool looped;
 
@@ -37,53 +42,53 @@ struct SDLSoundSource : ALDataSource
 	SDLSoundSource(SDL_RWops &ops,
 	               const char *extension,
 	               uint32_t maxBufSize,
-	               bool looped,
-	               int fallbackMode)
+	               bool looped)
 	    : srcOps(ops),
+	      unclosableOps(ops),
 	      looped(looped)
 	{
-		if (fallbackMode == 0)
-		{
-			sample = Sound_NewSample(&srcOps, extension, 0, maxBufSize);
-		}
-		else
-		{
-			// We're here because a previous attempt resulted in S32 format.
-
-			Sound_AudioInfo desired;
-			SDL_memset(&desired, '\0', sizeof (Sound_AudioInfo));
-			desired.format = AUDIO_F32SYS;
-
-			sample = Sound_NewSample(&srcOps, extension, &desired, maxBufSize);
-		}
-
+		/* A copy of srcOps with a no-op close function,
+		 * so we can reuse the ops if we need to change the format. */
+		unclosableOps.close = SDL_RWopsCloseNoop;
+		
+		sample = Sound_NewSample(&unclosableOps, extension, 0, maxBufSize);
+		
 		if (!sample)
 		{
-			SDL_RWclose(&ops);
+			SDL_RWclose(&srcOps);
 			throw Exception(Exception::SDLError, "SDL_sound: %s", Sound_GetError());
 		}
 
-		if (fallbackMode == 0)
+		bool validFormat = true;
+		
+		switch (sample->actual.format)
 		{
-			bool validFormat = true;
-
-			switch (sample->actual.format)
-			{
 			// OpenAL Soft doesn't support S32 formats.
 			// https://github.com/kcat/openal-soft/issues/934
 			case AUDIO_S32LSB :
 			case AUDIO_S32MSB :
 				validFormat = false;
-			}
+		}
 
-			if (!validFormat)
+		if (!validFormat)
+		{
+			// Unfortunately there's no way to change the desired format of a sample.
+			// https://github.com/icculus/SDL_sound/issues/91
+			// So we just have to close the sample (which closes the file too),
+			// and retry with a new desired format.
+			Sound_FreeSample(sample);
+			SDL_RWseek(&unclosableOps, 0, RW_SEEK_SET);
+			
+			Sound_AudioInfo desired;
+			SDL_memset(&desired, '\0', sizeof (Sound_AudioInfo));
+			desired.format = AUDIO_F32SYS;
+
+			sample = Sound_NewSample(&unclosableOps, extension, &desired, maxBufSize);
+
+			if (!sample)
 			{
-				// Unfortunately there's no way to change the desired format of a sample.
-				// https://github.com/icculus/SDL_sound/issues/91
-				// So we just have to close the sample (which closes the file too),
-				// and retry with a new desired format.
-				Sound_FreeSample(sample);
-				throw Exception(Exception::SDLError, "SDL_sound: format not supported by OpenAL: %d", sample->actual.format);
+				SDL_RWclose(&srcOps);
+				throw Exception(Exception::SDLError, "SDL_sound: %s", Sound_GetError());
 			}
 		}
 
@@ -95,8 +100,8 @@ struct SDLSoundSource : ALDataSource
 
 	~SDLSoundSource()
 	{
-		/* This also closes 'srcOps' */
 		Sound_FreeSample(sample);
+		SDL_RWclose(&srcOps);
 	}
 
 	Status fillBuffer(AL::Buffer::ID alBuffer)
@@ -162,8 +167,7 @@ struct SDLSoundSource : ALDataSource
 ALDataSource *createSDLSource(SDL_RWops &ops,
                               const char *extension,
 			                  uint32_t maxBufSize,
-			                  bool looped,
-			                  int fallbackMode)
+			                  bool looped)
 {
-	return new SDLSoundSource(ops, extension, maxBufSize, looped, fallbackMode);
+	return new SDLSoundSource(ops, extension, maxBufSize, looped);
 }

src/audio/vorbissource.cpp

@@ -53,7 +53,7 @@ static ov_callbacks OvCallbacks =
 
 struct VorbisSource : ALDataSource
 {
-	SDL_RWops &src;
+	SDL_RWops src;
 
 	OggVorbis_File vf;