Fix vague error message when trying to import a WAV file that is too large #91077
Conversation
bs-mwoerner
force-pushed
the
wav_2gb_message
branch
from
0d01931
to
165539a
Compare
| @@ -197,6 +197,8 @@ Error ResourceImporterWAV::import(const String &p_source_file, const String &p_s | |||
| break; | |||
| } | |||
|
|
|||
| ERR_FAIL_COND_V_MSG(chunksize > INT_MAX, ERR_INVALID_DATA, "WAV file '" + p_source_file + "' is too large (> 2 GiB of data). Try using a compressed format such as Ogg Vorbis or MP3 instead."); | |||
There was a problem hiding this comment.
Or try split the resource and shorten the audio length?
There was a problem hiding this comment.
We could truncate the stream, but I'm not sure this would benefit anyone. I can think of scenarios where such a huge file might end up in the project tree, but I can't think of any where you'd actually want Godot to import it. Doing a partial import of such a file might just produce gigabytes of unused data in the exported game.
There was a problem hiding this comment.
Oh sorry, I mean we can suggest the user to truncate the stream manually instead of import it as a whole (if they really want the wav format).
There was a problem hiding this comment.
Ah, sorry, I misread your comment. How about "WAV file 'xxx' is too large (> 2 GiB of data). Try shortening or splitting it or use a compressed format such as Ogg Vorbis or MP3 instead."?
There was a problem hiding this comment.
@bs-mwoerner: you got an "OK". I guess we can continue with getting this merged?
Maybe we can add something to the docs of AudioStream (or AudioStreamWAV), that audio file must be <= 4 GiB ?!
There was a problem hiding this comment.
@MJacred You're right, I missed the "thumbs up". I've updated the commit with the new wording.
The limit is actually 2 GiB of actual data, because the limiting factor here isn't the WAV format itself but the way Godot stores binary resources, so the stream after importing can't be longer than INT_MAX bytes. We can add that to the docs (I've never comitted to the documentation before but there's a first time for everything.) Would it make sense to include the doc update in this pull request?
There was a problem hiding this comment.
It's been a while since I originally checked this, but I may have stopped looking prematurely when I saw that ResourceFormatSaverBinaryInstance::write_variant() treated sizes > 2 GiB as negative lengths. At second glance, it seems to mostly call methods that reinterpret this as an unsigned int, so the overflows should cancel out. In that case, the problem may indeed just be the WAV stream itself.
There was a problem hiding this comment.
Would it make sense to include the doc update in this pull request?
Yes :)
bs-mwoerner
force-pushed
the
wav_2gb_message
branch
from
165539a
to
fbb228e
Compare
…s with more than 2 GiB of data. Fixed a possible overflow in AudioStreamWAV::set_data() when trying to set data with a size close to INT_MAX.
bs-mwoerner
force-pushed
the
wav_2gb_message
branch
from
fbb228e
to
0fd4335
Compare
Fixes #90957.
This is certainly an edge case (we're talking about more than 3 hours of audio in a single stream), but I suppose the error message could be a bit nicer than "Condition "p_size < 0"".
I also fixed an edge case in AudioStreamWAV, where the size calculation would overflow when adding the padding bytes to a size that's already close to INT_MAX.
I looked into truly supporting 4 GiB WAVs, but that doesn't seem feasible, because it runs into problems with saving the binary resource later, so that would require some more fundamental changes.