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

Implement AudioStreamOGGOpus #47114

Closed
wants to merge 1 commit into from

Conversation

mortarroad
Copy link
Contributor

Brings back support for the opus audio codec, based on the existing dependency on libopusfile.

Although there are a bunch of allocations scattered throughout both libogg and libopusfile, they are only for extending buffers as far as I can tall, if those happen to be too small, and I assume most of them are executed when the start of the file is being parsed. (I.e. the same point, where our other codecs also allocate.)

Also, I doubt that allocations make a big difference here, anyway. There's a lot worse things that could happen, since desktop OSs aren't real time systems after all.
I'm not sure where this rule of no allocations even came from...?

In a little test, I could play ~240+ AudioStreamPlayer3Ds of 100 kbit/s files simultaneously without any stuttering.
In comparison, with Vorbis, I can play ~280+ streams at once.

If there is interest in this: I already have a backported version for 3.x prepared.

Comment on lines 55 to 56
virtual void _mix_internal(AudioFrame *p_buffer, int p_frames);
virtual float get_stream_sampling_rate();
Copy link
Member

Choose a reason for hiding this comment

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

Those are necessary in the master branch, but indeed if you did the work initially on 3.2 there might be some mismatch, as we weren't using the override keyword in 3.2.

@mortarroad mortarroad force-pushed the opus-stream branch 3 times, most recently from 9a915ec to ca6633e Compare March 17, 2021 19:57
@reduz
Copy link
Member

reduz commented Mar 17, 2021

I am against merging Opus in Godot. It provides very little benefit over what we have.

  • Slightly smaller size for audio files but..
  • Considerably more CPU usage
  • Makes binary larger, when we are looking towards reducing it (in fact, we are looking towards moving video codecs out, so Ogg/Vorbis/Theora/WebM/etc and other libraries will be removed).

Opus is mostly useful in our conext for VOIP as its a good general purpose replacement for Speex, but Godot will never supply VOIP out of the box given the large and diverse amount of ways to do it.

I recommend waiting for the GDNative changes in Godot 4.0 and creating a GDNative plugin.

@mortarroad
Copy link
Contributor Author

I disagree.
For music and sound effects, there is probably no big benefit in size, unless you have a lot of music.
For voice lines, however, I can easily get transparent quality at 32 kbit/s, and still acceptable quality at 24 kbit/s.
For vorbis, I need at least 80 kbit/s. That is more than 2x!

I doubt that performance will be an issue, realistically. And in such a case, the other codecs are still there.

That this increases the binary size a bit is true; a more light-weight opus decoder would of course be ideal.
(Currently I estimate the whole opus+ogg thing adds around 1 to 2 MB, judging from the binary size of opusdec.)

@DarkKilauea
Copy link
Contributor

For VOIP, it could be useful to have the stream support available so users can grab packets off the network and feed them into the stream still compressed. Otherwise, that would need to be implemented by the user themselves.

Since the library is already available, it makes sense to include this until the splitting of codecs into GDNative plugins is complete.

@Calinou
Copy link
Member

Calinou commented Mar 17, 2021

For voice lines, however, I can easily get transparent quality at 32 kbit/s, and still acceptable quality at 24 kbit/s.
For vorbis, I need at least 80 kbit/s. That is more than 2x!

Remember that voice lines can often be encoded at 22050 Hz with no loss of quality unless they're high-pitched (e.g. children voices). For higher-pitched voices, you could also try 32000 Hz. This is because of the Nyquist-Shannon sampling theorem.

@reduz
Copy link
Member

reduz commented Mar 17, 2021

@mortarroad Sure, but a game that requires such a huge amount of voice files where this will make a difference can very well use a GDNative add-on for Opus, likewise for VOIP. It does not make sense to add stuff to the engine that will be used rarely.

@mortarroad
Copy link
Contributor Author

Then, would it make sense to put opus support in 3.x instead, since the libraries are already there?
(That's what I had originally prepared this for, but github told me to merge against master.)

@mortarroad
Copy link
Contributor Author

@reduz Can I get an opinion about getting this into 3.x instead, since the libraries are already there?
If not, there's no need to keep this PR open.

@akien-mga
Copy link
Member

Can I get an opinion about getting this into 3.x instead, since the libraries are already there?

To answer this, if we don't plan to add Opus support back in 4.0, it would probably be confusing to re-add it in 3.4. It would be seen once again as a regression if support is lost going from 3.4 to 4.0.

@mortarroad mortarroad deleted the opus-stream branch May 22, 2021 22:58
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