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

Audio: Write to device's buffer directly without temporary buffers. #90013

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

Conversation

kus04e4ek
Copy link
Contributor

@kus04e4ek kus04e4ek commented Mar 29, 2024

Also, a little bit of a clean-up.

TODO:

  • Implement for the input buffer;
  • Test on MacOS/iOS. (I don't have them, so would appreciate some help)

@@ -36,7 +36,6 @@
#include "core/templates/safe_refcount.h"
#include "servers/audio_server.h"

#include <mmsystem.h>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe it's needed for MSVC?

@@ -30,11 +30,6 @@

#include "audio_driver_opensl.h"

#include <string.h>
Copy link
Contributor Author

@kus04e4ek kus04e4ek Mar 29, 2024

Choose a reason for hiding this comment

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

Probably not needed? Android builds always use clang

@@ -54,7 +54,8 @@ class AudioDriverWeb : public AudioDriver {

int buffer_length = 0;
int mix_rate = 0;
int channel_count = 0;
int channels = 0;
Copy link
Contributor Author

@kus04e4ek kus04e4ek Mar 29, 2024

Choose a reason for hiding this comment

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

Changed the name to match other audio drivers.

Comment on lines 100 to 109
virtual SpeakerMode get_speaker_mode() const = 0;
virtual int get_total_channels() const = 0;
Copy link
Contributor Author

@kus04e4ek kus04e4ek Mar 29, 2024

Choose a reason for hiding this comment

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

It could be better to implement new SpeakerMode values instead of get_total_channels?


if (k == cs - 1 && stride == (uintptr_t)cs * 2 - 1) {
// Downmix.
_write_sample(p_buffer, idx, (int32_t)(((int64_t)vl2 + (int64_t)vr2) / 2));
Copy link
Contributor Author

@kus04e4ek kus04e4ek Mar 29, 2024

Choose a reason for hiding this comment

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

Maybe it would be better to pass _write_sample as an argument or make it a virtual method in AudioDriver? It looks like all existing drivers don't need it though

drivers/pulseaudio/audio_driver_pulseaudio.cpp Outdated Show resolved Hide resolved
drivers/wasapi/audio_driver_wasapi.cpp Outdated Show resolved Hide resolved
drivers/coreaudio/audio_driver_coreaudio.cpp Outdated Show resolved Hide resolved
@fire
Copy link
Member

fire commented Mar 29, 2024

How would you suggest testing this?

@kus04e4ek
Copy link
Contributor Author

kus04e4ek commented Mar 29, 2024

How would you suggest testing this?

Just using AudioStreamPlayer and switching driver in the settings.
On Linux, ALSA and PulseAudio always use 16-bit integers.
On Android, OpenSL always uses 16-bit integers.
On MacOS and iOS, CoreAudio always uses floats 16-bit integers.
On Web, Worklet always uses floats.
On Windows, XAudio2 always uses 16-bit integers, WASAPI uses the most fitting format, format can be obtained only through debugging, there might be different formats for different output devices.
MovieWriter always uses 32-bit integers. Also setting channels can be useful for testing too.
AudioDriverDummy uses no buffer, but still processes audio.

I checked downsampling by modifying PulseAudio code like this:

Patch

diff --git a/drivers/pulseaudio/audio_driver_pulseaudio.cpp b/drivers/pulseaudio/audio_driver_pulseaudio.cpp
index 2e7a3155da1..166ee9472a8 100644
--- a/drivers/pulseaudio/audio_driver_pulseaudio.cpp
+++ b/drivers/pulseaudio/audio_driver_pulseaudio.cpp
@@ -200,6 +200,8 @@ Error AudioDriverPulseAudio::init_output_device() {
 		return err;
 	}
 
+	pa_channel_map_init_mono(&pa_map);
+
 	if (pa_map.channels <= 8) {
 		channels = pa_map.channels;
 	} else {

@kus04e4ek
Copy link
Contributor Author

kus04e4ek commented Apr 19, 2024

Web audio is broken on master too. (#90906)
MovieWriter is not recording any video (#90932) and corrupts audio when using any SpeakerMode other than Stereo (can't reproduce it anymore?) on master too.
Apart from that (and the fact that I didn't test on MacOS and iOS), this PR seems fully functional.

Comment on lines +275 to +301
case AudioDriver::BUFFER_FORMAT_INTEGER_32:
// (1u << (32 - 1)) - 1 can't be stored in float correctly.
((int32_t *)p_buffer)[p_idx] = (double)p_sample * ((1u << (32 - 1)) - 1);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is different from the implementation that was before:

float l = CLAMP(buf[from + j].left, -1.0, 1.0);
int32_t vl = l * ((1 << 20) - 1);
int32_t vl2 = (vl < 0 ? -1 : 1) * (ABS(vl) << 11);
*dest = vl2;

But I'm not sure why it was like this, all I can find on the Internet is how to convert from float to 16-bit integer and it's just to multiply a float to match the range of the number (eg. -1.0 is the minimum value that can fit in 16-bit integer and 1.0 is the maximum), so I expect it to be the same with 32-bit integers?
Also, used everywhere ((1u << (*bits* - 1)) - 1), though it would be better to use (1u << (*bits* - 1)) for negative numbers and ((1u << (*bits* - 1)) - 1) for positive ones. But I think it's really hard to hear the difference so kept it like this for a small optimization (which would be good as audio callbacks really should run as fast as possible).

Comment on lines +102 to +131
case AudioDriver::BUFFER_FORMAT_INTEGER_24: {
int32_t sample = int32_t(((int8_t *)p_buffer)[p_idx * 3 + 2]) << 16;
sample |= int32_t(((int8_t *)p_buffer)[p_idx * 3 + 1]) << 8;
sample |= int32_t(((int8_t *)p_buffer)[p_idx * 3 + 0]);

return (float)sample / (1u << (24 - 1));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A sign will probably be wrong, but unsure how to fix that.

Comment on lines +266 to +296
case AudioDriver::BUFFER_FORMAT_INTEGER_24: {
int32_t sample = p_sample * ((1u << (24 - 1)) - 1);
int32_t sign = p_sample < 0 ? -1 : 1;

((int8_t *)p_buffer)[p_idx * 3 + 2] = sign * (ABS(sample) >> 16);
((int8_t *)p_buffer)[p_idx * 3 + 1] = sign * (ABS(sample) >> 8);
((int8_t *)p_buffer)[p_idx * 3 + 0] = sample;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as the above.

Comment on lines 431 to +447
const inb = GodotRuntime.heapSub(HEAPF32, p_in_buf, p_in_size);
const input = event.inputBuffer;
const input_channels = input.numberOfChannels;
if (GodotAudio.input) {
const inlen = input.getChannelData(0).length;
for (let ch = 0; ch < 2; ch++) {
for (let ch = 0; ch < input_channels; ch++) {
const data = input.getChannelData(ch);
for (let s = 0; s < inlen; s++) {
inb[s * 2 + ch] = data[s];
for (let sample = 0; sample < data.length; sample++) {
inb[sample * input_channels + ch] = data[sample];
Copy link
Contributor Author

@kus04e4ek kus04e4ek Apr 19, 2024

Choose a reason for hiding this comment

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

Might not be a good idea to pass all of the channels of the input buffer, but thought that downmixing code should be kept in the same place for all AudioDrivers.

@kus04e4ek kus04e4ek force-pushed the audio-fix branch 2 times, most recently from e86d4df to b1d4f24 Compare April 19, 2024 13:29
@Faless
Copy link
Collaborator

Faless commented Apr 19, 2024

Web audio is broken on master too.

Can you expand on this? If you use threaded export audio should work fine.

@kus04e4ek
Copy link
Contributor Author

Web audio is broken on master too.

Can you expand on this? If you use threaded export audio should work fine.

Comment on lines +43 to +44
if env["platform"] == "windows" and not env.msvc:
SConscript("backtrace/SCsub")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved out of Sounds drivers section

Comment on lines +12 to 24
if env["platform"] in ["ios", "macos"]:
SConscript("coreaudio/SCsub")

if env["platform"] == "linuxbsd":
if env["alsa"]:
SConscript("alsa/SCsub")
if env["pulseaudio"]:
SConscript("pulseaudio/SCsub")

if env["platform"] == "windows":
if env["xaudio2"]:
SConscript("xaudio2/SCsub")
SConscript("wasapi/SCsub")
Copy link
Contributor Author

@kus04e4ek kus04e4ek Apr 29, 2024

Choose a reason for hiding this comment

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

I think it's better this way, as no empty files will be compiled. Maybe it would be better to add checks like this to other drivers? Also should I delete checks like #ifdef ALSA_ENABLED in files?


int latency = Engine::get_singleton()->get_audio_output_latency();
buffer_size = closest_power_of_2(latency * mix_rate / 1000);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think power of 2 is required, can't find anything about it online

Comment on lines -85 to +92
WAVEFORMATEX wave_format = { 0 };
WAVEFORMATEX wave_format;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixes g++ warning

Comment on lines +79 to +81
if (unlikely(audio_context.input_channels == 0)) {
audio_context.input_channels = godot_audio_get_input_channels();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't really know if there is a better way to initialize input_channels

unsigned int input_position = 0;
unsigned int input_size = 0;

void audio_server_process(int p_frames, int32_t *p_buffer, bool p_update_mix_time = true);
void audio_server_process(int p_frames, void *p_buffer, bool p_active = true, bool p_update_mix_time = true);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really sure about p_active, it tells whether or not to process audio or just fill buffer with 0. I think it should be an argument, because AudioDriverOpenSL and AudioDriverCoreAudio pass false if callback is unable to lock mutex, it doesn't feel right to have a function like is_active because of that. input_process has something simmilar to this, if you pass nullptr as a buffer (it's needed for AudioDriverWASAPI), internall buffer will be filled with 0, we can't pass nullptr in audio_server_process, as we still need to fill a buffer. It's just a simplification to not call memset and to not reapeat the same code

@kus04e4ek
Copy link
Contributor Author

Marked as a draft, cause I want to separate some changes into different PRs/commits

@nyabinary
Copy link

Marked as a draft, cause I want to separate some changes into different PRs/commits

Any updates on this?

@kus04e4ek
Copy link
Contributor Author

Marked as a draft, cause I want to separate some changes into different PRs/commits

Any updates on this?

Sadly no, have no time right now :/

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