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: Enable QOA compression by default #95815

Merged
merged 1 commit into from
Sep 2, 2024

Conversation

DeeJayLSP
Copy link
Contributor

@DeeJayLSP DeeJayLSP commented Aug 19, 2024

Modifies ResourceImporterWAV's compression mode to use QOA as default instead of disabling.

QOA is lightweight, has a reasonable quality and supports almost every significant feature of AudioStreamWAV that is used on imported audio files, so I think it's a good idea to leave it enabled by default.

@clayjohn
Copy link
Member

Do you know what features aren't supported? Are they anything that Godot users could be relying on? I'm just trying to figure out if this is technically a compatibility breaking PR.

A change in quality vs. size is fine, but if it breaks scripts, or a feature stops working, then we would need to discuss

@DeeJayLSP
Copy link
Contributor Author

DeeJayLSP commented Aug 19, 2024

This merely changes an import option default to another value. The only situations I can see this "breaking" is if the user:

  • Imports a WAV file using default parameters.

Then one of the options below:

  • Uses the AudioStreamWAV:save_to_wav() function.

This returns an ERR_UNAVAILABLE because QOA streams can't be saved as WAV.

  • Attempts to use AudioStreamWAV:data as regular PCM data.

For both, users would need to change the compress mode from QOA to Disabled.

AudioStreamWAVs created from code (which is how most people using the options above are likely using them) don't get affected at all because AudioStreamWAV defaults are already different than those set by ResourceImporterWAV.

@clayjohn
Copy link
Member

Thank you for clarifying! It sounds like this PR does break compatibility. But only in one particular case that is likely rare.

@Mickeon
Copy link
Contributor

Mickeon commented Aug 19, 2024

Can't comment on the specific line for some reason, but I believe that we should highlight QOA compression being the default in the description. The change would make it a bit unorthodox, since the default would not be the first option out of the 3.

@DeeJayLSP
Copy link
Contributor Author

DeeJayLSP commented Aug 19, 2024

It sounds like this PR does break compatibility. But only in one particular case that is likely rare.

I wouldn't consider this, since the exact same "breakage" can be currently achieved by changing the import params from Disabled to QOA.

Existing projects with already imported WAV files don't get affected at all.

@DeeJayLSP
Copy link
Contributor Author

DeeJayLSP commented Aug 19, 2024

The change would make it a bit unorthodox, since the default would not be the first option out of the 3.

Not every importer has all the default options on the first.

A good example of this is fonts: all of its import options with dropdowns (Antialiasing, Hinting and Subpixel Positioning) have the second option as default.

@Calinou Calinou added this to the 4.x milestone Aug 19, 2024
@DeeJayLSP DeeJayLSP changed the title ResourceImporterWAV: Compress WAV files to QOA by default ResourceImporterWAV: Enable QOA compression by default Aug 19, 2024
@Calinou
Copy link
Member

Calinou commented Aug 20, 2024

I'm neutral on whether this is a good idea. On one hand, it'll reduce exported project install sizes a bit, but it also means we opt into using lossy compression by default for audio. When importing WAV, you may have the expectation that your audio is lossless by default (unlike Ogg Vorbis and MP3).

In a sense, this would be similar to using Lossy compression for images by default. We only use lossy VRAM compression for textures detected to be used in 3D by default, not 2D images which are typically used for UI.

@DeeJayLSP
Copy link
Contributor Author

DeeJayLSP commented Aug 20, 2024

I decided to check what other engines do with WAV files, and the most significant ones seem to use some type of compression by default (load-time Vorbis decompression in Unity's case, and the proprietary Bink Audio format in Unreal).

Before QOA was added, Godot didn't have a way to compress WAV files other than IMA-ADPCM, which greatly degrades sound quality and misses a few features (such as seeking, backward playback and resampling, all supported by QOA).

Personally, I think the usage of an optimal/compressed option like QOA should be encouraged. The option to disable compression remains there.

@clayjohn
Copy link
Member

I just double checked and it appears that .import files save all properties which means you are right and this will just change behaviour for new files. In which case it won't break compatibility.

@akien-mga
Copy link
Member

Can't comment on the specific line for some reason, but I believe that we should highlight QOA compression being the default in the description.

I agree with this. BTW the current description is a bit misleading, as it describes WAV as an uncompressed format but we're now compressing it by default:

WAV is an uncompressed format, which can provide higher quality compared to Ogg Vorbis and MP3. It also has the lowest CPU cost to decode. This means high numbers of WAV sounds can be played at the same time, even on low-end deviceS.

(Also note the capital "S" type at "deviceS", also worth fixing.)

@DeeJayLSP
Copy link
Contributor Author

DeeJayLSP commented Aug 20, 2024

I agree with this. BTW the current description is a bit misleading, as it describes WAV as an uncompressed format but we're now compressing it by default:

It seems all the other audio ResourceImporters describe the format instead of the importer itself.

I added a note to that description to explain that it will be compressed on import.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

IMO this is worth a try now. IIRC we discussed doing this back when reviewing #91014, and opted to make it opt-in initially for 4.3, and consider making it default later. Later can be now :) and we have the whole 4.4 cycle to find whether this causes issues for some users and reconsider if need be.

Could be good to have some metrics on how this actually affects project size and playback latency and quality on a WAV heavy project.

@DeeJayLSP
Copy link
Contributor Author

Rebased on top of master after recent merges.

@Mickeon Mickeon modified the milestones: 4.x, 4.4 Aug 31, 2024
@akien-mga akien-mga merged commit 36f95ef into godotengine:master Sep 2, 2024
19 checks passed
@akien-mga
Copy link
Member

Thanks!

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.

5 participants