2019-10-18 23:21 EDT


View Issue Details Jump to Notes ]
IDProjectCategoryView StatusLast Update
0002866FSSCPsoundpublic2015-12-19 18:51
ReporterFUBAR-BDHR 
Assigned ToFUBAR-BDHR 
PrioritynormalSeverityminorReproducibilityalways
Statuscode reviewResolutionopen 
Product Version3.6.19 
Target Version3.7.0Fixed in Version 
Summary0002866: Sounds played from event editor in FRED get extension added even if they have one
Descriptionevent_editor::OnPlay() calls audiostream_open() with ASF type forced to ASF_EVENTMUSIC which has a value of 1. This causes keep_ext in WaveFile::Open() to be true. For some reason the code does not look at this flag and assumes no extension and adds one. Normally this doesn't seem to cause an issue but as soon as you have a long enough file name you end up going over the 32 character limit due to the duplicate extension and getting an ERANGE string error.
Steps To ReproducePut a wav file into data\voice\special and rename it so it is 30 characters total length including extension. Open up FRED, create a message with this .wav file. Make sure you include the .wav extension in the filename. Hit the play button.
TagsNo tags attached.
Attached Files
  • patch file icon event_editor_sound.patch (1,210 bytes) 2013-05-08 04:04 -
    Index: audiostr.cpp
    ===================================================================
    --- audiostr.cpp	(revision 9673)
    +++ audiostr.cpp	(working copy)
    @@ -437,6 +437,7 @@
     	char filename[MAX_FILENAME_LEN];
     	const int NUM_EXT = 2;
     	const char *audio_ext[NUM_EXT] = { ".ogg", ".wav" };
    +	bool fred_forced_failed = false;
     
     	m_total_uncompressed_bytes_read = 0;
     	m_max_uncompressed_bytes_to_read = AS_HIGHEST_MAX;
    @@ -454,6 +455,12 @@
     			}
     		}
     
    +		// FRED forces keep_ext to true so there may not actually be an extension
    +		if ((rc < 0) && (Fred_running)) {
    +			fred_forced_failed = true;
    +			rc = cf_find_file_location_ext(filename, NUM_EXT, audio_ext, CF_TYPE_ANY, sizeof(fullpath) - 1, fullpath, &FileSize, &FileOffset);
    +		} 
    +
     		// not a supported extension format ... somebody screwed up their tbls :)
     		if (rc < 0)
     			goto OPEN_ERROR;
    @@ -467,8 +474,10 @@
     
     	if (rc < 0) {
     		goto OPEN_ERROR;
    -	} else {
    -		// set proper filename for later use (assumes that it doesn't already have an extension)
    +	} 
    +
    +	// set proper filename for later use (assumes that it doesn't already have an extension)
    +	if ((!keep_ext) || (fred_forced_failed)) {
     		strcat_s( filename, audio_ext[rc] );
     	}
     
    
    patch file icon event_editor_sound.patch (1,210 bytes) 2013-05-08 04:04 +
  • patch file icon 2866.patch (3,816 bytes) 2013-06-02 22:43 -
    Index: fred2/briefingeditordlg.cpp
    ===================================================================
    --- fred2/briefingeditordlg.cpp	(revision 9689)
    +++ fred2/briefingeditordlg.cpp	(working copy)
    @@ -1363,8 +1363,8 @@
     		return;
     	}
     
    -	// we use ASF_EVENTMUSIC here so that it will keep the extension in place
    -	m_voice_id = audiostream_open((char *)(LPCSTR) m_voice, ASF_EVENTMUSIC);
    +	// changed to use ASF_VOICE since that is what will be used in game and the file may or may not have an extension - FUBAR
    +	m_voice_id = audiostream_open((char *)(LPCSTR) m_voice, ASF_VOICE);
     
     	if (m_voice_id >= 0) {
     		audiostream_play(m_voice_id, 1.0f, 0);
    Index: fred2/cmdbrief.cpp
    ===================================================================
    --- fred2/cmdbrief.cpp	(revision 9689)
    +++ fred2/cmdbrief.cpp	(working copy)
    @@ -327,8 +327,8 @@
     		return;
     	}
     
    -	// we use ASF_EVENTMUSIC here so that it will keep the extension in place
    -	m_wave_id = audiostream_open((char *)(LPCSTR) m_wave_filename, ASF_EVENTMUSIC);
    +	// changed to use ASF_VOICE since that is what will be used in game and the file may or may not have an extension - FUBAR
    +	m_wave_id = audiostream_open((char *)(LPCSTR) m_wave_filename, ASF_VOICE);
     
     	if (m_wave_id >= 0) {
     		audiostream_play(m_wave_id, 1.0f, 0);
    Index: fred2/debriefingeditordlg.cpp
    ===================================================================
    --- fred2/debriefingeditordlg.cpp	(revision 9689)
    +++ fred2/debriefingeditordlg.cpp	(working copy)
    @@ -487,8 +487,8 @@
     		return;
     	}
     
    -	// we use ASF_EVENTMUSIC here so that it will keep the extension in place
    -	m_voice_id = audiostream_open((char *)(LPCSTR) m_voice, ASF_EVENTMUSIC);
    +	// changed to use ASF_VOICE since that is what will be used in game and the file may or may not have an extension - FUBAR
    +	m_voice_id = audiostream_open((char *)(LPCSTR) m_voice, ASF_VOICE);
     
     	if (m_voice_id >= 0) {
     		audiostream_play(m_voice_id, 1.0f, 0);
    Index: fred2/eventeditor.cpp
    ===================================================================
    --- fred2/eventeditor.cpp	(revision 9689)
    +++ fred2/eventeditor.cpp	(working copy)
    
    @@ -1470,8 +1478,8 @@
     		return;
     	}
     
    -	// we use ASF_EVENTMUSIC here so that it will keep the extension in place
    -	m_wave_id = audiostream_open((char *)(LPCSTR) m_wave_filename, ASF_EVENTMUSIC);
    +	// changed to use ASF_VOICE.  Extension should be stripped not preserved since playback will be stripped in FS2_Open - FUBAR
    +	m_wave_id = audiostream_open((char *)(LPCSTR) m_wave_filename, ASF_VOICE);
     
     	if (m_wave_id >= 0) {
     		audiostream_play(m_wave_id, 1.0f, 0);
    Index: sound/audiostr.cpp
    ===================================================================
    --- sound/audiostr.cpp	(revision 9689)
    +++ sound/audiostr.cpp	(working copy)
    @@ -461,14 +461,22 @@
     		cf_find_file_location(pszFilename, CF_TYPE_ANY, sizeof(fullpath) - 1, fullpath, &FileSize, &FileOffset);
     	}
     	// ... otherwise we just find the best match
    -	else {
    +	else {		
    +		// check that passed filename isn't too long for the extension
    +		if (strlen(filename) > MAX_FILENAME_LEN-5) {
    +			Warning( LOCATION, "Passed filename, '%s', is too long to support an extension!!\n\nMaximum length, minus the extension, is %i characters.\n", filename, MAX_FILENAME_LEN-5 );
    +			goto OPEN_ERROR;
    +		}
     		rc = cf_find_file_location_ext(filename, NUM_EXT, audio_ext, CF_TYPE_ANY, sizeof(fullpath) - 1, fullpath, &FileSize, &FileOffset);
     	}
     
     	if (rc < 0) {
     		goto OPEN_ERROR;
    -	} else {
    -		// set proper filename for later use (assumes that it doesn't already have an extension)
    +	} 
    +
    +	// set proper filename for later use (assumes that it doesn't already have an extension)
    +	// and don't add one if it is supposed to already have it. - FUBAR
    +	if (!keep_ext) {
     		strcat_s( filename, audio_ext[rc] );
     	}
     
    
    patch file icon 2866.patch (3,816 bytes) 2013-06-02 22:43 +

-Relationships
related to 0002864resolvedniffiwan Standalone ASSERTION: "handle >= 0" trying to unload head .ani file 
+Relationships

-Notes

~0015043

FUBAR-BDHR (developer)

Attaching patch that might fix the issue but can't test it in FS2 to make sure id doesn't break anything there due to 2684 not allowing me to load test missions.

~0015074

niffiwan (developer)

I'm not sure what the 1st patch hunk is there for. It'll load a file in certain circumstances, and then (if successful) it'll load the same file again?

Also, if there is no extension on the file, the "stristr" check will find that and log an error? If FRED forces event_editor::OnPlay() to use type ASF_EVENTMUSIC shouldn't a file extension be checked for and enforced at that point?

Why not just keep the 2nd hunk (i.e. only add an extension if !keep_ext) but also add a check to ensure the filename is <= MAX_FILENAME_LEN-4?

~0015075

FUBAR-BDHR (developer)

Well it was never a requirement that the files have an extension. Forcing the type the type to ASF_EVENTMUSIC was probably just a hack in FRED to make it work with or without an extension. Basically not having to worry about the type, stripping the extension, etc. There are tons of missions out there without extensions on messages.

If the possible redundant call to cf_find_file_location(); is an issue it could be put into and if (!fred_forced_failed).

Now the real issue is the way this whole thing was setup could cause what is played in FRED to be different that what is played in game if both an .ogg and .wav exist since FRED will always use the extension and FS2 will always do the best match for voice files.

~0015098

niffiwan (developer)

After some thought, I think we can break this into two problems with corresponding solutions.

1) ERANGE error
Don't add the 2nd extension if one already exists, and verify that the filename isn't too long if an extension is being added.

2) FRED being able to play audio files without an extension AND Avoid confusion if both a .wav & .ogg are present
event_editor::OnPlay() should check for the presence of an extension in the passed filename & set ASF_EVENTMUSIC appropriately. This should also ensure the same file plays in FRED & FSO. If there's no extension then FSO & FRED will find the same file (.ogg if it exists, otherwise .wav). If there is an extension, then it's fine as well. If an invalid extension is added (or mispelled), or the filename is wrong then the sound won't play which should prompt the FREDDER to investigate further.

How does that sound? :)

~0015099

FUBAR-BDHR (developer)

Well the 1st one should already be happening in FS2. The only time I saw where the extension would get added and possibly pushed over the limit was if the type was ASF_EVENTMUSIC. That was fixed by the if (!keep_ext) around the strcat_s. Of course an extra check for it never hurts in case the above function doesn't do what it's supposed to do.

The 2nd one is a little more complicated. It's not just event_editor::OnPlay() it's every call from FRED. Now all those could all have the extension checks added but what type do you call audiostream_open() with? The files used can be voice files which aren't tabled, sound files which may or may not be tabled, of music files which may or may not be tabled. In all cases FRED doesn't require an extension but the tabled version may.

Now I can see 2 possible ways to fix the second one. Edit ever place in FRED and add redundant code to either add an extension if one doesn't exist or strip all extensions. You still don't really have a valid type to call audiostream_open() with though. Add a new type like ASF_FRED and handle this in audiostream_open(). Of course handling the extensions there is probably how V should have done it in the first place for all calls not just from FRED so everything would be consistent and extensions would be optional in all tables. And as usual fixing it totally would break backward compatibility so that's probably not an option. Of course the 3rd option is go with the hack and revamp the whole sound file handling in wxFRED with proper.

The part about the wrong filename would not be a good idea. From what I've seen most of the time the files entered there don't exist. People seem to like using that filed for comments for voice acting so there are a lot of things like "alien music bit" or "surrender or we fire" in those fields. Even when they are actual file names they may not exist because the voice acting isn't done at the time the messages are made.

~0015115

FUBAR-BDHR (developer)

Well it seems this one was even more complex but the solution for most of it was a lot easier.

It turns out that depending on where the voice is being played from it may or may not even go through audiostr.cpp in game. Briefings and debriefings do but messages played via events don't unless they are training messages. It does appear that everywhere voice files are played that the extension is stripped before loading. This means that forcing ASF_EVENTMUSIC in FRED actually did the opposite of what would happen in FS2.

So anyway the solution is just to change the types in FRED to ASF_VOICE. That along with a warning to prevent the ERANGE error (if it hits this outside of FRED there are bigger fish to fry) and the previous not adding an extension if it's supposed to have one should cover this.

Of course there is still is the possibility of an edge case where you will get 2 different results for entries in music.tbl used in messages. This would require that files exist with both .wav and .ogg extensions and are used as messages. With ASF_EVENTMUSIC keeping the extension and everything else dropping it I don't see a way to easily keep this from happening.

~0016807

Yarn (developer)

This PR, which has already been merged, should at least prevent the error caused by adding a second extension: https://github.com/Yarn366/fs2open.github.com/commit/b1e7a2a1bf5e72224ca369b950ed6a04007613d4
+Notes

-Issue History
Date Modified Username Field Change
2013-05-08 03:13 FUBAR-BDHR New Issue
2013-05-08 03:13 FUBAR-BDHR File Added: event_editor_sound.patch
2013-05-08 03:14 FUBAR-BDHR File Deleted: event_editor_sound.patch
2013-05-08 04:04 FUBAR-BDHR File Added: event_editor_sound.patch
2013-05-08 04:05 FUBAR-BDHR Note Added: 0015043
2013-05-08 04:23 FUBAR-BDHR Assigned To => FUBAR-BDHR
2013-05-08 04:23 FUBAR-BDHR Status new => assigned
2013-05-08 04:24 FUBAR-BDHR Status assigned => code review
2013-05-20 01:26 niffiwan Relationship added related to 0002684
2013-05-20 01:26 niffiwan Relationship deleted related to 0002684
2013-05-20 01:26 niffiwan Relationship added related to 0002864
2013-05-20 05:00 niffiwan Note Added: 0015074
2013-05-20 06:15 FUBAR-BDHR Note Added: 0015075
2013-05-30 05:49 niffiwan Note Added: 0015098
2013-05-30 15:45 FUBAR-BDHR Note Added: 0015099
2013-06-02 22:43 FUBAR-BDHR File Added: 2866.patch
2013-06-02 22:57 FUBAR-BDHR Note Added: 0015115
2015-12-19 18:51 Yarn Note Added: 0016807
+Issue History