Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ResourceImporterWAV: Use dr_wav to read file data #96545

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

DeeJayLSP
Copy link
Contributor

@DeeJayLSP DeeJayLSP commented Sep 4, 2024

Changes

In ResourceImporterWAV::import(), replaces the entire data read step with one that uses dr_wav.

Formats already supported by Godot and loop info detection should remain supported without regressions.

Extra WAV encoding formats like A-law, μ-law, MS ADPCM and IMA ADPCM can now be imported.

Note: All of them will be decoded on import, so any loss will be retained and passed to the compress modes, but at least a few WAV files won't be rejected anymore.

Note 2: Even if Godot is able to compress and play audio as IMA ADPCM, the way it's compressed within Godot doesn't match the usual compression done in WAV files, so those couldn't be imported anyway, unless a compatibility breaking change is done to modify how Godot encodes and plays it.

AIFF support

dr_wav is also able to read AIFF files, so ResourceImporterWAV was enabled to recognize it too.

The importer was renamed from Microsoft WAV to Microsoft WAV/Apple AIFF to reflect that.

wave, aif, aiff and aifc file extensions have been added as recognized extensions alongside wav.

Others

Since both AudioStreamWAV and ResourceImporterWAV use a 32-bit integer variable to store the amount of PCM frames, I added a check that denies import if the number of frames exceed INT32_MAX. This doesn't break compatibility, as attempting to import a file bigger than that would actually cause issues to the importer.

@@ -35,6 +35,15 @@
#include "core/io/resource_saver.h"
#include "scene/resources/audio_stream_wav.h"

#define DRWAV_IMPLEMENTATION
#define DR_WAV_NO_STDIO
#define DR_WAV_LIBSNDFILE_COMPAT
Copy link
Contributor Author

@DeeJayLSP DeeJayLSP Sep 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Despite not using libsndfile, the way Godot converts streams from/to f32 follows similar calculations, therefore not defining this leads to some test errors (save/reimport datas being different).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Horray

@DeeJayLSP
Copy link
Contributor Author

Added a check to detect if the container is RF64 or W64.

Even if they could be imported in theory, this is the best non-compatibility breaking way to deny getting a stream that is too big for AudioStreamPlaybackWAV. And it's not like anyone would export to those formats with a .wav extension.

@DeeJayLSP
Copy link
Contributor Author

DeeJayLSP commented Sep 6, 2024

Modified the check again, this time it actually checks the amount of samples (maximum of 2147483647).

Normally, the importer uses an int to store the total amount of samples, therefore using a limit shouldn't actually break compatibility (in fact, probably fixes a potential overflow on import).

@fire
Copy link
Member

fire commented Sep 6, 2024

Does the dr mp4 problem affect dr_wav? Not sure of the exact problem.

@DeeJayLSP
Copy link
Contributor Author

Does the dr mp4 problem affect dr_wav? Not sure of the exact problem.

The problem with dr_mp3 is that it doesn't read a VBR header that informs an amount of samples to skip at the start.

Doesn't apply to WAV.

@DeeJayLSP
Copy link
Contributor Author

Added .aifc extension to the list as it's supported too.

else if (loop.type == drwav_smpl_loop_type_backward)
loop_mode = AudioStreamWAV::LOOP_BACKWARD;
loop_begin = loop.firstSampleByteOffset;
loop_end = loop.lastSampleByteOffset;
Copy link
Contributor Author

@DeeJayLSP DeeJayLSP Sep 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "Byte" in the names here is misleading, it's actually the offsets of loop begin/end in frames, not bytes.

I have tested this considering the old read code actually copied a sample value directly, not a byte offset.

Works the same way as the previous implementation just fine.

@DeeJayLSP DeeJayLSP requested a review from a team as a code owner September 11, 2024 04:02
@DeeJayLSP
Copy link
Contributor Author

Updated ResourceImporterWAV documentation to point out it also supports AIFF.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants