-
Notifications
You must be signed in to change notification settings - Fork 27
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
Upgrade webrtc-audio-processing lib to v1.3 #23
base: master
Are you sure you want to change the base?
Conversation
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.
Whoa, this must have been a ton of work! I've reviewed roughly the first half of the expanded diff, added some comments. I haven't worked with the library yet, so please take them with a grain of salt.
Thanks for taking care to update and extend tests, that's admirable you found energy to do that despite overall size of the change.
src/config.rs
Outdated
/// Analyze the output of the linear AEC instead of the capture frame. Has no effect if | ||
/// echo_canceller.export_linear_aec_output is false. | ||
pub analyze_linear_aec_output_when_available: bool, |
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.
Optional brainstorm: we can try to combine this with echo_canceller.export_linear_aec_output
into 3-state enum, so that it is not possible to represent state that has no effect. Caveat: would have to be in some higher-level structure and do more harm than good.
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.
Thank you for the brainstorm. I think I will wait to introduce a 3-state enum for now. I want to keep the API flat and less opinionated until I have deeper understanding of the internals and what are the recommended configurations. To improve a bit, I removed EchoCanceller::export_linear_aec_output
, and changed it to configure automatically based on NoiseSuppressor:: analyze_linear_aec_output
, after confirming the webrtc implementation that linear AEC output is only used by noise cancellation.
I gave up on this TODO. Bindgen cannot bind virtual functions (rust-lang/rust-bindgen#27). |
@skywhale are you able to build this on MacOS? I'm getting the following error:
|
Oh, this is caused by me not having |
Ah yes, meson and ninja are two build tools you need. I've only tested on my macbook so far; there may be some more work necessary on Linux. |
I'm getting more build errors on linux:
Possibly related? abseil/abseil-cpp#832 |
612b1f3
to
38cae2b
Compare
c40fcb4
to
3b9aa5a
Compare
This PR has been rebased on |
@skywhale I'm getting an error message trying to use this branch with bundled flag on macOS:
Any idea how to fix / debug this? Update: Fixed by adding abseil back |
Upgrade the underlying webrtc-audio-processing library to v1.3. This is a major update, which include significant changes to the AEC behaviors.
Tasks