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

Add support for QOA (Quite OK Audio) format #88646

Closed
wants to merge 2 commits into from

Conversation

DeeJayLSP
Copy link
Contributor

@DeeJayLSP DeeJayLSP commented Feb 21, 2024

Implements support for the Quite OK Audio format (QOA). Closes godotengine/godot-proposals#9133. More details on that proposal.

QOA was developed to (not exactly just this) be a better alternative to IMA-ADPCM for use in games according to the article announcing it (which unironically uses Godot as an example).

Briefly, this adds a way for developers to compress sound effects without relying on expensive decoding (MP3, Ogg Vorbis) or destroyed quality (IMA-ADPCM).

A deprecation notice could be added later for when someone tries to import WAV as IMA-ADPCM (getting godotengine/godot-proposals#4264, where QOA was first mentioned, closed too). Adding a converter from WAV was suggested there.

The final release template binaries (at least on Linux) have a size increase of only 24KiB, but it gets compensated if at least 0.18-0.35s of WAV sound gets converted to QOA.

qoa.h had to be patched to suppress a few warnings.


Approaches

1. QOA files can be imported directly

  • Place a QOA file into the project folder and it will get imported directly.

2. WAV files can be imported to use QOA internally

  • Go to the FileSystem dock, click on a WAV file, open the Import tab and switch the importer from Microsoft WAV to Quite OK Audio. The file will be converted and imported as QOA.
  • A file that was previously imported as Microsoft WAV will retain all of its custom properties when changing to Quite OK Audio.

Caveats

  • Lots of lines from ResourceImporterWAV are duplicated into ResourceImporterQOA. Something required to make the converter work. A general WAV converter could be made into a header and included on both WAV and QOA importers later without breaking compatibility;
  • It uses AudioStreamPlaybackResampled, like MP3 and Vorbis. It's said that AudioStreamPlaybackWAV uses linear interpolation for resampling because it's cheaper to calculate, therefore being better for resampling lots of sound effects, while Resampled uses cubic, slightly more expensive, but some comments state that cubic should be fast enough nowadays. There are two open PRs related to resampling: one to make AudioStreamWAV inherit from Resampled and another that optimizes Resampled's cubic hermite resampling. Merging the former automatically dismisses this caveat;

Issues to address in case this is accepted:

  • WAVs get imported as Quite OK Audio instead of Microsoft WAV by default;
    • Fixed by overriding get_priority to return 0.9 instead of the default value of 1.0;
  • Reviewer demands;
    • Possible flaws in the code? Note that I made sure to fix every minor issue with it;
    • Does the way the patch is done satisfy reviewers?

@DeeJayLSP
Copy link
Contributor Author

DeeJayLSP commented Feb 22, 2024

I know image files in Godot have their importer somehow enforced to be Texture2D at first. Maybe the first issue could be fixed like this?

@Mickeon
Copy link
Contributor

Mickeon commented Feb 22, 2024

I believe it has something to do with get_import_order. Try overriding that.

@DeeJayLSP
Copy link
Contributor Author

DeeJayLSP commented Feb 22, 2024

get_import_order is used to determine the import order of resource types and has no effect in the order of being chosen.

For example, if an importer for a .xy file has an import order of 0, and one for a .zw file has an order of 1, the editor will import all .xy files before .zw.

Regardless, this helped me find the actual function we need to override, which is get_priority, which actually affects order of being chosen. I have overriden that to return 0.9 so ResourceImporterWAV gets a higher priority. This is not how image importing does it but works anyway.

With this fixed, I think the only thing left are some reviews.

@DeeJayLSP DeeJayLSP marked this pull request as ready for review February 22, 2024 21:40
@DeeJayLSP DeeJayLSP requested review from a team as code owners February 22, 2024 21:40
@AThousandShips AThousandShips added this to the 4.x milestone Feb 24, 2024
@fire
Copy link
Member

fire commented Feb 25, 2024

Is it possible for the priorities to be adjusted so they're integers (in a float)? Thoughts?

</member>
<member name="data" type="PackedByteArray" setter="set_data" getter="get_data" default="PackedByteArray()">
Contains the audio data in bytes.
You can load a file without having to import it beforehand using the code snippet below. Keep in mind that this snippet loads the whole file into memory and may not be ideal for huge files (hundreds of megabytes or more).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
You can load a file without having to import it beforehand using the code snippet below. Keep in mind that this snippet loads the whole file into memory and may not be ideal for huge files (hundreds of megabytes or more).
You can load a file without having to import it beforehand using the code snippet below:

The "Keep in mind [...]" thing could go below the codeblocks.
Also I feel like putting this example inside data instead of the leading description of the class is somewhat weird.

Copy link
Contributor Author

@DeeJayLSP DeeJayLSP Feb 25, 2024

Choose a reason for hiding this comment

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

This is copypasted from AudioStreamMP3.xml. If we should modify it, the changes should go there first.

Copy link
Contributor

@Mickeon Mickeon Feb 29, 2024

Choose a reason for hiding this comment

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

I have no suggestions documentation-wise, then. It's all copy-pasted from the others. It... definitely needs to be cleaned up at some point.

There's also some codestyle oddities in the QOA implementation itself, but I am to assume it's also all copypasted from the source and I would not touch them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, not everything is copypasted. I would like to know what are the oddities.

@DeeJayLSP
Copy link
Contributor Author

DeeJayLSP commented Feb 25, 2024

Is it possible for the priorities to be adjusted so they're integers (in a float)? Thoughts?

Seems like get_priority() can't return 0 or lower, or else the importer window breaks wherever the importer shows (ResourceFormatImporter::get_importer_by_extension checks if priority is higher than 0, or else returns an invalid importer).

For this to work properly we would have to make ResourceImporterWAV's return 2. Should it be done this way?

@fire
Copy link
Member

fire commented Feb 26, 2024

Wave cannot be deprecated since it is the majority of audio inputs from libraries

I don’t recommend removing the music code from QOA.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally, it works as expected.

In terms of quality, I can't spot the difference with the uncompressed WAV source on my Audeze Maxwell headphones. There's no doubt this sounds much better than IDA-ADPCM.

With a 38-second 44,100 Hz, 16-bit stereo audio file, the uncompressed WAV is 6.5 MB and the corresponding imported QOA file (in .godot/imported) is 1.3 MB. This is a reduction of over 5 MB, which is much greater than the file size added to export templates by this PR. Even if you only had a few seconds of mono audio to compress to QOA, you'd still easily save the ~25 KB added by this PR.

My only concern is with the different import mode required, as we don't use this approach for any other audio format (unless the source format is different, like Ogg Vorbis and MP3). I don't have a good answer to this though, as refactoring it sounds difficult.

There's also the alternative of decoding Ogg Vorbis/MP3 to uncompressed audio at load-time which could work well, at the cost of requiring more memory (it would have to be something you enable on import on specific sound effects).

Testing project: test_pr_88646.zip1

Footnotes

  1. Music in the testing project by Marc A. Pullen.

@DeeJayLSP
Copy link
Contributor Author

DeeJayLSP commented Mar 5, 2024

Something I was writing before reviews

I added a greatly optimized version of the reference player's logic inside AudioStreamPlaybackQOA. This makes the diff smaller and reduces a lot of unnecessary callbacks.

The patches in qoa.h are now solely meant to remove unused functions and fix errors/warnings when including it instead of things like "add reference player".

@DeeJayLSP
Copy link
Contributor Author

DeeJayLSP commented Mar 6, 2024

My only concern is with the different import mode required

Technically, this is an importer for QOA files. It has just been enabled to import from WAV files too, with more parameters, because at the present moment QOA converters are uncommon (currently, FFmpeg master supports QOA decoding but not encoding).

There's also the alternative of decoding Ogg Vorbis/MP3 to uncompressed audio at load-time

As far as I understood, one of the motivations behind QOA's creation was to avoid having to do this.

Considering QOA is already cheap to decode, I believe it's better than sacrificing even a few KBs of memory.

@DeeJayLSP
Copy link
Contributor Author

Tested a web release template build (platform=web target=template_release production=yes, enabling and disabling qoa).

The size penalty of the resulting zip was only 4.7KiB.

@DeeJayLSP
Copy link
Contributor Author

DeeJayLSP commented Mar 29, 2024

Ping-Pong and Backward loops successfully implemented.

The only thing left for a full feature parity with WAV is a stereo variable. Which I don't know why it exists in first place when you can simply use the number of channels, so I see no reason to use it. WAV itself doesn't expose a channels variable. I have changed QOA so it uses stereo instead of number of channels.

Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

The last remaining criticism I had of the pull regarding missing Ping-Pong and Backward loops was solved, so I don't have any arguments against merging QOA. Maybe @reduz or the audio team can look, but we don't have an active audio team.

Approving the enhancement, didn't review the technical implementation deeply.

@akien-mga akien-mga requested a review from reduz April 10, 2024 14:48
@DeeJayLSP
Copy link
Contributor Author

DeeJayLSP commented Apr 15, 2024

I have changed the stream to use a stereo bool instead of number of channels and removed loop count. Just to enforce parity with AudioStreamWAV.

Feature-wise, the only thing missing is format, which isn't applicable to QOA at all. And I don't think anyone uses it in code (the only way it could possibly break compatibility when switching importers).

- Remove loop count, it's not present on WAV;
- Rename sign to increment;
- Remove channels in favor of stereo to match WAV;
@DeeJayLSP
Copy link
Contributor Author

Superseded by #91014

@akien-mga
Copy link
Member

Superseded by #91014.

@akien-mga akien-mga closed this May 2, 2024
@akien-mga akien-mga removed this from the 4.x milestone May 2, 2024
@DeeJayLSP DeeJayLSP deleted the qoa branch May 2, 2024 16:48
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.

Add support for QOA (Quite OK Audio) format
7 participants