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

2x and 4x oversampling for WaveShaperNode #332

Merged
merged 1 commit into from
Feb 15, 2020

Conversation

khodzha
Copy link
Contributor

@khodzha khodzha commented Feb 2, 2020

Issue #205
Finally managed to defeat the noise I mentioned in the issue

I'm not sure if ditching FrameIterator and operating on raw vecs was a good thing 🤷‍♂️

Also spec mentions tail-time but I'm not sure how to handle that

@khodzha khodzha force-pushed the oversampling_speex branch 4 times, most recently from 0b45b68 to 6aac50e Compare February 2, 2020 20:22
Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

Looks good!

Should we wait for it to be released to crates.io? I don't want to use a git dependency for something that's not a rapidly-changing servo crate.

We could also publish a fork.

audio/Cargo.toml Outdated
@@ -18,6 +18,10 @@ servo-media-player = { path = "../player" }
servo-media-traits = { path = "../traits" }
smallvec = "0.6.1"

[dependencies.speexdsp]
git = "https://github.com/rust-av/speexdsp-rs"
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Ah, it's not released yet.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like they're willing to make one soon! rust-av/speexdsp-rs#10

fn apply_curve(buf: &mut [f32], curve: &Vec<f32>) {
buf.iter_mut().for_each(|sample| {
let len = curve.len();
let curve_index: f32 = ((len - 1) as f32) * (*sample + 1.) / 2.;
Copy link
Member

Choose a reason for hiding this comment

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

worth moving (len - 1) / 2 and len out of the closure. Probably will be done by the compiler anyway, but doesn't hurt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed, I'll fix it up tomorrow

@khodzha khodzha force-pushed the oversampling_speex branch 2 times, most recently from 182bfdd to 7acd6ca Compare February 13, 2020 07:39
Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

This is ready to land once speexdsp releases

@lu-zero
Copy link

lu-zero commented Feb 14, 2020

I prepared a crate with the stand alone resampler, if you could please confirm that the API exposed is fine for you I'll publish it.

@Manishearth
Copy link
Member

@khodzha ^^

@khodzha khodzha force-pushed the oversampling_speex branch 2 times, most recently from 9ab1116 to 79094ce Compare February 14, 2020 21:39
@khodzha
Copy link
Contributor Author

khodzha commented Feb 14, 2020

Updated PR to use this crate, API seems ok to me 🤷‍♂️

@lu-zero
Copy link

lu-zero commented Feb 14, 2020

https://crates.io/crates/speexdsp-resampler published !

@@ -79,33 +93,68 @@ impl AudioNodeEngine for WaveShaperNode {
AudioNodeType::WaveShaperNode
}

fn process(&mut self, mut inputs: Chunk, _info: &BlockInfo) -> Chunk {
fn process(&mut self, mut inputs: Chunk, info: &BlockInfo) -> Chunk {
debug_assert!(inputs.len() == 1);

if inputs.blocks[0].is_silence() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Manishearth could you confirm whether tail-time here could be implemented like this:
if we hit two silent blocks in row after non-silent block, we explicit_silence() them and run them through the waveshaping so all the tail-time from resampler would be spitted out

Copy link
Member

Choose a reason for hiding this comment

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

That sounds correct.

@khodzha
Copy link
Contributor Author

khodzha commented Feb 15, 2020

Added tail-time handling and properly handled the case when waveshaped silence is nonsilent

@Manishearth
Copy link
Member

@bors-servo r+

thanks!

@bors-servo
Copy link
Contributor

📌 Commit a8a1f91 has been approved by Manishearth

@bors-servo
Copy link
Contributor

⌛ Testing commit a8a1f91 with merge f992e21...

bors-servo pushed a commit that referenced this pull request Feb 15, 2020
2x and 4x oversampling for WaveShaperNode

Issue #205
Finally managed to defeat the noise I mentioned in the issue

I'm not sure if ditching FrameIterator and operating on raw vecs was a good thing 🤷‍♂️

Also spec mentions tail-time but I'm not sure how to handle that
@bors-servo
Copy link
Contributor

☀️ Test successful - checks-travis
Approved by: Manishearth
Pushing f992e21 to master...

@bors-servo bors-servo merged commit a8a1f91 into servo:master Feb 15, 2020
@khodzha khodzha deleted the oversampling_speex branch February 15, 2020 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants