-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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 FLAC audio support #80160
Add FLAC audio support #80160
Conversation
4012c01
to
a68635c
Compare
Can you expand on this? We definitely should allow custom audio formats to be implemented via GDExtension (even if we decide to include flac support in core). |
I only found this mention by @DeleteSystem32: godotengine/godot-proposals#1190 (comment) |
That was for GDNative, but I've tried reimplementing it as a GDExtension a few days ago and it works fine, here. |
Thanks, I've edited the PR description to link to it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally (rebased on top of master
237bd0a), it works as expected.
The import options provided are consistent with Ogg Vorbis and MP3, which makes sense as these are designed to be used for music and ambient sounds.
Testing project: test_pr_80160.zip1
FLAC | Ogg Vorbis 412 Kb/s2 | MP3 320 Kb/s |
---|---|---|
The impact of this PR on binary size is 103 KB (70,337,792 -> 70,440,320 bytes) on a stripped Linux x86_64 release export template (with LTO). This is fairly significant for mobile/web platforms – I wonder if this PR's impact could be reduced somehow.
Footnotes
-
Music in the testing project by Marc A. Pullen. ↩
-
I didn't know Ogg Vorbis could go past 320 Kb/s, but it can when exported from Audacity 🙂 ↩
Note that dr_flac supports metadata access, in case we want to implement godotengine/godot-proposals#3307. |
2f02ec4
to
f0bd879
Compare
Co-authored-by: Vincent <[email protected]>
I don't think I support this. I'd guess maybe 0.1% of games will use this, and if it adds 100k that's an enormous dependency to bring on for such a small group of users. There's a working GDExtension for it right? If not, we should figure out how to support it in GDExtension. I think the necessary methods are exposed. |
Are we certain that FLAC is niche? It’s both popular enough and underütilized to me. [P.S.] It’s an entire audio codec after all, a few KiBs to a few dozen KiBs is expected. Though yes, not a hundred. |
In the context of game development, it is considered a niche format as you don't benefit from lossless audio that much:
The main use cases for FLAC in games are:
Adding 100 KB of binary size is only worth it if you're certain a majority of exported projects will be able to save at least 100 KB of file size. In practice, this isn't the case for a lot of projects (e.g. if they only use lossy audio, or no audio at all for non-game applications). |
For comparison, I did a quick release build on DeleteSystem32's GDExtension implementation. The stripped lib sizes are the following:
I must add however that I built it as it was provided (not even sure if it's well optimized), only changing to release and stripping. |
Out of curiosity, I decided to do a minimal release production build for the web (some modules ended up being built, but the difference is what matters more here). The command used was The resulting .wasm file got increased by 58.48KB (25,345,358 to 25,403,838 bytes). The resulting .zip file got increased by 19.8KB (6,036,845 to 6,056,647). One thing to be aware of is that builds for web are optimized for size by default, and |
Godot is not audiophile software, so I don't personally see the need to take on even 1k of extra binary size for audiophile codecs considering the fact that there is a working GDExtension for games that require it. Is there anyone out there who absolutely needs this to be in core? I've seen a few people say FLAC support would benefit them. Maybe one of those people could explain why the GDExtension doesn't satisfy their needs? This feels hypothetical right now and we don't generally merge PRs that solve hypothetical problems. I know I posted this earlier but useful context for anyone skimming: https://godotengine.org/article/will-your-contribution-be-merged-heres-how-tell/ |
It isn't just for audiophile purposes. In context of sound effects, using formats like MP3 or Vorbis isn't really good for when someone wants to play lots of sound effects at the same time. While FLAC doesn't decrease the file size at the same level, it is capable of decoding about 1.4-1.5x faster (at least that's what libFLAC on ffmpeg shows when decoding to PCM) therefore being less demanding than those. This is a possible use case for FLAC. A game with lots of sound effects (which certainly isn't just a few cases) could benefit by having those as FLAC instead. For that context, I don't think a GDExtension helps when it's over 30x larger than a built-in implementation. |
Some disorganized thoughts.
I'm sorry about the wall of text and I promise I'm not trying to be difficult, I just don't think this is a useful feature for most people who use Godot. The main benefit here seems to be compatibility with user-provided sounds. I know someone is working on a DAW in Godot and this seems like a good feature for them, but I think for that application a GDExtension is probably fine?
|
Out of curiosity, I decided to do the same test with a pack of 51 UI sound effects from Kenney, which does include some really short sound effects. In total, the total WAV size got to 2.3MiB, while the total FLAC size got to 1.1MiB. I can confirm, however, that some sound effects increased in size upon conversion. Those are really short effects (less than 0.1s duration). All of the following files are 44100Hz 2 channels:
|
Curious if you could try IMA-ADPCM. In Godot it's an import option and you'd just normal use the WAV. In ffmpeg you can just add I bet it will be substantially smaller than FLAC, so if your primary concern is some combination of speed and size it will outperform FLAC in both, while sounding mediocre. But again, if your mix is muddy that really isn't going to matter IMO. |
For the record, that's a known godot-cpp bug which will be solved eventually. godotengine/godot-cpp#1160 We shouldn't hyperfocus on size as the sole deciding factor anyway. 1k or 100k or even a megabyte can totally be justified for a feature that's going to be used by most Godot users. For a niche feature, the size matters but what's a lot more important is the maintenance burden, compilation time, etc. "Death by a thousand cuts" impacts both binary size and compilation time, and the more thirdparty libraries we add, the more we need to be aware of security vulnerabilities reported against them, or even just bugfixes we need to bring downstream. The actual use case for FLAC doesn't seem to have been well identified yet. I see engineers reasoning about whether it makes sense or not from a size or a decompression time point of view, but I don't see yet sound designers telling us that they're using FLAC in their professional pipeline and would need it in Godot. |
What roles do audio quality play in Music/Rhythm games? 🤔 |
Commercial rhythm games typically use ogg just as other games(take a look at arcaea), however community rhythm games definitely need FLAC support as they support import chart pack from outside. In my opinion audio quality doesn't matter that much, if a beat can only be recognized in lossless format, then it is just too soft to add a key on it. If you just want to listen the music you can always go and download a FLAC version. |
This implementation outputs a glitchy audio if the file has only one channel, and won't play at all if it's 3 or above; |
Coming here two months later just to say: forget anything I said about FLAC for sound effects. Turns out FFmpeg decodes FLAC with multithreading by default (as phoboslab claims). And I ended up testing exactly like that. QOA is a better format than FLAC except in quality, and better than IMA-ADPCM except in decoding speed, but both are reasonably good enough. |
@DeeJayLSP Should we close this PR then as you've reached a consensus that FLAC support is not valuable in core? |
For the purpose I was defending, FLAC isn't viable. After all, why use a sound effect in FLAC when you could use Vorbis at a reasonable bitrate to save space and have the same decoding speed? The other purpose pointed out here was rhythm games where FLAC music might need support, but for a very specific purpose like this it can be easily implemented through a non-core third-party module or a GDExtension (both already exist by the way). My conclusion is that FLAC as a core module isn't strictly needed. |
Okay then. Given the concerns from the audio team members as well, I'm going to close this issue. But I hope to see a GDExtension for FLAC for those who need it 🙂 |
There are two topics in this PR thread. That said, for this PR specifically, the implementation isn’t great. Over a hundred KBs, yet it’s still only half-baked.
Even if we deem FLAC Core-worthy, perhaps another better implementation can make this PR obsolete. |
@DeeJayLSP What was the fix? |
@fire AudioFrame[f1, f2] When it should be: AudioFrame[f1, f1] The fix consists on declaring a fixed-size buffer (let's call it You can essentially do the same as the
|
Fixes: godotengine/godot-proposals#1190
EDIT: Added reference to the working GDExtension