Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Don't specify sample rates for voice messages #5802

Merged
merged 1 commit into from
Mar 29, 2021

Conversation

turt2live
Copy link
Member

Turns out the browser doesn't actually resample for us, instead opting to explode in sadness.

We'll leave the resampling to the opus encoder.

Fixes element-hq/element-web#16775

Turns out the browser doesn't actually resample for us, instead opting to explode in sadness.

We'll leave the resampling to the opus encoder.

Fixes element-hq/element-web#16775
@turt2live turt2live requested a review from a team March 26, 2021 00:25
Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

Hmm, presumably you haven't been able to test this? This would have just been specifying an ideal sample rate, so if the capture device didn't support that the browser will give you whatever's available. If you want to force it to resample I think you'd have to add an exact requirement.

From the rageshake, it looks like it's failing to connect the two nodes of different sample rates, so by not specifying a sample rate I'd have thought you'd still get whatever the capture device is spitting out, so you'd get the same error?

One solution might be to have the ideal constraint but then inspect the sample rate you actually get and init the audioContext with that.

That said, if this does work then fine :)

@turt2live
Copy link
Member Author

I indeed don't have a way of testing this: my audio stack is capable of producing all the suggested sample rates.

Ultimately, it's the same CPU to resample in the browser or in the encoder (practically), so I'm not too concerned about it. If we start getting complaints about high CPU usage then let's figure something out.

@turt2live
Copy link
Member Author

@dbkr can I get a checkmark so github isn't mad at me? 😇

Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

Can certainly try it, but to be clear my concern isn't CPU usage, it's that it won't fix the error.

@turt2live
Copy link
Member Author

oh, well, hmm. It should fix it because the sample rate goes through implicitly.

@turt2live turt2live merged commit dfd6551 into develop Mar 29, 2021
@turt2live
Copy link
Member Author

... at least that's how I'm understanding the docs this time around.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Voice messages: Fix sample rate selection
2 participants