Fix echo cancelation hack quality issues, re-enable by default #4275
Conversation
| @@ -133,6 +133,7 @@ | |||
| "react-use-css-breakpoints": "^1.0.3", | |||
| "resize-observer-polyfill": "^1.5.1", | |||
| "screenfull": "^4.0.1", | |||
| "sdp-transform": "^2.14.1", | |||
There was a problem hiding this comment.
No change in package-lock.json should be needed because this is the version already being used by mediasoup. Hard to tell for sure because my local NPM rewrote the entire file (https://nitayneeman.com/posts/catching-up-with-package-lockfile-changes-in-npm-v7/). This is something we will need to tackle at some point..
| @@ -89,7 +89,8 @@ export const SCHEMA = { | |||
|
|
|||
| preferences: { | |||
| type: "object", | |||
| additionalProperties: false, | |||
| // Allow removed preferences to pass validation | |||
| additionalProperties: true, | |||
There was a problem hiding this comment.
This isn't strictly necisary, we could instead keep around the old preference, but honestly I see little downside to only validating the defined properties and then ignoring the rest. We can eventually write code to clean up orphaned properties.
There was a problem hiding this comment.
Yeah, I don't remember if there were specific implications we were concerned about that led us to disallowing additional properties. The one thing I can think of is a bit contrived -- this effectively makes the schema mutable as opposed to append-only. Which means we could theoretically delete a property, and then add it back at some later date with a different type, which would break backwards compatibility. There's also a concern of the down-stream use of the stored data. If we allow additional properties, it may open a hole for unexpected data to flow through to other systems. This is a common enough issue that it's listed in security best practices: https://apisecurity.io/encyclopedia/content/oasv3/datavalidation/schema/v3-schema-object-additionalproperties-true.htm
In this particular case it should be safe, since this is scoped to preferences alone, and I don't think we pass the preference structure around.
| const parsedSdp = sdpTransform.parse(answer.sdp); | ||
| for (let i = 0; i < parsedSdp.media.length; i++) { | ||
| for (let j = 0; j < parsedSdp.media[i].fmtp.length; j++) { | ||
| parsedSdp.media[i].fmtp[j].config += `;stereo=1;cbr=0;maxaveragebitrate=510000;`; |
There was a problem hiding this comment.
Given that we are overriding the decoder stereo preference, maybe we should also override the sender sprop-stereo preference? And this is totally a question more than a change request.
There was a problem hiding this comment.
I already forget how the webrtc negotiation process works since it has fallen out of mental cache, but if I remember correctly its the answer that is going to determine what both the "sender" and "receiver" will do. In any case I confirmed in the webrtc inspector that the bitrate is indeed increased and the output is definitely audibly stereo.
I had noticed that the previous hack we had in place to fix echo cancellation in Chrome significantly reduced audio quality and broke stereo audio panning, so I disabled it by default #4227 and added an additional preference for "aggressive echo cancellation".
This PR fixes the quality issues with the hack and gets stereo working, so I am comfortable re-enabling it by default in Chrome. Don't think we need a separate preference anymore either (though it is still only enabled if you actually have echo cancellation enabled, which is different from the state prior to #4227).
We should still monitor the upstream bug (https://bugs.chromium.org/p/chromium/issues/detail?id=687574), and strip this out once a fix lands (to avoid the extra work of encoding another audio stream), but this should help a lot with echo in the meantime (unless most of the benefit in echo reduction we were actually seeing was a result of crushing the outbound audio so much that it just picked up less..)
The recommendation still remains, wear headphones and disable echo cancellation!