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

Fix Capture and Record AudioEffect bugs for surround systems #92532

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

Conversation

BuzzLord
Copy link

@BuzzLord BuzzLord commented May 30, 2024

The AudioEffectCapture and AudioEffectRecord classes do not work properly when there is more than one audio channel active. This PR makes some changes to several AudioEffect classes to fix these bugs. Fixes #91133.

I added set_current_channel to AudioEffectInstance to keep AudioServer::_update_bus_effects clean, since I wanted that functionality for capture and record. It was already part of compressor, and seems like something that could be useful for other effect instances if they needed it. The default implementation is a no-op.

Added an active_channel property to both AudioEffectCapture and AudioEffectRecord. Capture uses it to return early from process so that only the active instance writes data to the buffer.

Record now stores all instances (that are registered) by channel, and when changing the active_channel, it changes the internal current_instance to reflect that.

The active_channel property is exposed in the editor for both classes. It doesn't currently update the available drop-down options to reflect the actual number of channels available in the AudioServer. The set_active_channel functions also don't explicitly check for the validity of the p_channel args, but they do work with invalid inputs (it just disables capture/recording).

@BuzzLord BuzzLord requested review from a team as code owners May 30, 2024 01:09
@@ -47,6 +47,7 @@ class AudioEffectInstance : public RefCounted {
public:
virtual void process(const AudioFrame *p_src_frames, AudioFrame *p_dst_frames, int p_frame_count);
virtual bool process_silence() const;
virtual void set_current_channel(int p_channel) {};
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
virtual void set_current_channel(int p_channel) {};
virtual void set_current_channel(int p_channel) {}

For format check

Copy link
Author

Choose a reason for hiding this comment

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

Fixed. Also added virtual to compressor and capture set_current_channel (I received email suggestions for these, but don't see them on github). I didn't add it to record as its set_current_channel is a bit more nuanced, and if someone wants to override it, they can refactor it at that time.

Copy link
Member

Choose a reason for hiding this comment

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

We always add both, not adding it won't stop overriding later you need final for that

Copy link
Author

Choose a reason for hiding this comment

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

Fair enough.

@Chaosus Chaosus added this to the 4.3 milestone May 30, 2024
@BuzzLord BuzzLord force-pushed the audioeffect-surround-bug branch 2 times, most recently from 3dca84f to c3f8750 Compare May 31, 2024 08:30
@BuzzLord
Copy link
Author

Is anyone able to take a look at this? I can appreciate it might be hard to test without an affected audio setup. I have a (crude) change to the Audio Dummy Driver that could help with testing, if the audio setup is too much of an issue. The change just exposes the speaker mode as a configurable parameter when the dummy driver is active in the editor.

@Mickeon
Copy link
Contributor

Mickeon commented Jun 21, 2024

Letting the tests run.
There's very few maintainers that are keenly aware of the audio side of Godot so this may take a while to be taken a look at.

@akien-mga akien-mga modified the milestones: 4.3, 4.4 Jul 31, 2024
@OpenFormEon
Copy link

Is anyone able to take a look at this? I can appreciate it might be hard to test without an affected audio setup. I have a (crude) change to the Audio Dummy Driver that could help with testing, if the audio setup is too much of an issue. The change just exposes the speaker mode as a configurable parameter when the dummy driver is active in the editor.

I have a game where the entire gameplay is recording and playing back audio via AudioEffectRecord from players' microphones, and many players have reported that the recording doesn't work. Input is working, because they can hear themselves when the bus isn't muted and AudioEffectSpectrumAnalyzer works just fine. Only the recording is failing. Do you think my game's issue could also be due to the bug you describe? If so, what's the best way to try out your fix in my version of Godot? Apologies, I'm unfamiliar with both Github and how to change the Godot Engine's own code.

@Nihlus
Copy link

Nihlus commented Aug 24, 2024

I'm also experiencing this - as long as the output is a stereo 2.0 channel, recording works fine, but any HiFi / Surround configuration makes AudioEffectCapture unusable for my use case (speech to text).

@BuzzLord
Copy link
Author

I have a game where the entire gameplay is recording and playing back audio via AudioEffectRecord from players' microphones, and many players have reported that the recording doesn't work. Input is working, because they can hear themselves when the bus isn't muted and AudioEffectSpectrumAnalyzer works just fine. Only the recording is failing. Do you think my game's issue could also be due to the bug you describe?

I do think this is the bug. AudioEffectRecord only records from the last channel (because of the bug), which on anything more than a stereo system will be silent (microphones normally only record stereo to the first channel). This should fix it.

The SpectrumAnalyzer is one of the only effects that is channel aware, which is why it works. You select the effect instance (which is per-channel) with AudioServer.get_bus_effect_instance(bus, effect), but there's a third channel argument that defaults to 0 (the main stereo channel).

If so, what's the best way to try out your fix in my version of Godot? Apologies, I'm unfamiliar with both Github and how to change the Godot Engine's own code.

Unfortunately I'm new here too, and am not sure what the best way to test would be. I setup a dev environment to build and test my own releases, but I don't think sharing executables is the way to go. Maybe one of the main contributors can chime in.

I'm also experiencing this - as long as the output is a stereo 2.0 channel, recording works fine, but any HiFi / Surround configuration makes AudioEffectCapture unusable for my use case (speech to text).

If this is the bug, the audio should sound very garbled and static-y. A work around I use in my own project (in GDScript) is to pull frames from the capture buffer in chunks of 512. Then before passing that array on, I loop over it, and check that both channels of every frame is exactly equal to 0.0. If so, I skip that chunk. This works because the AudioServer processes 512 frames at a time, per channel, so your good data should be in amongst 512 frame chunks of silence.

@BuzzLord
Copy link
Author

BuzzLord commented Sep 2, 2024

@OpenFormEon I was looking at the Godot Contribution Workflow docs and see that you can download the editor with this PR applied for testing, once the workflow/tests are run on it again, or by using the nightly.link website. Details available here.

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.

AudioEffectCapture with surround sound audio artifacts
7 participants