feat: re-acquire mic track when effects require specific audio constraints#99
feat: re-acquire mic track when effects require specific audio constraints#99antsukanova wants to merge 28 commits into
Conversation
antsukanova
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e903658ce5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54f77fd9d6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0769169e4f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
bbaldino
left a comment
bbaldino
left a comment
There was a problem hiding this comment.
Couple comments. Some impressive catches by codex as well 👀
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed4b73969e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
antsukanova
requested review from
SomeBody16,
edvujic,
k-wasniowski and
szotrabh
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c0479d79f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 160e7ed94d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
antsukanova
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f09c510685
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08c970b4eb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34689a22a9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea33c2da60
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69e7aadc78
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 908a40b6fe
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
bbaldino
left a comment
bbaldino
left a comment
There was a problem hiding this comment.
This is tough...the logic is getting pretty complex here. Left some some comments.
| return false; | ||
| } | ||
|
|
||
| // The effect may have been removed or the track may have ended while |
There was a problem hiding this comment.
If this effect is removed, that will fire another event, right?
I'm wondering if this is worth adding. If it is, then we could technically be doing this check after every await point (since the effect could've been removed at any of those suspension points).
There was a problem hiding this comment.
Yes, removing the effect fires disposed which removes our listeners — but that only stops future events from firing. It doesn't stop a handler that's already mid-execution waiting on getUserMedia.
One guard here is enough because getUserMedia is the only await where real time passes.
But believe we need to keep it due to my rapid enable/disable race flow: https://github.com/webex/webrtc-core/pull/99/changes/BASE..908a40b6feaf71a422155872f3e356fe341b7c64#r3208557639
There was a problem hiding this comment.
There are other await points here though, right? (Can't easily check it now). Any await/suspension point could happen for any duration of time, we can't know for sure.
There was a problem hiding this comment.
I have a bit changes in logic here and now it has simplified flow, so we have only one if (!this.effects.includes(effect)) { just after await getUserMedia({ to check if effect was disposed during getUserMedia.
We have other awaits here, but they feel ok to skip this check and not overcomplicate. Ideally, I wish to remove this workaround altogether if we fix applyContrains.
| } | ||
|
|
||
| // The effect may have been removed or the track may have ended while | ||
| // getUserMedia was running. Discard the new track so it doesn't keep |
There was a problem hiding this comment.
I don't follow what's meant by "getUserMedia was running" here?
There was a problem hiding this comment.
Got your confusion, then suggest here a bit clearer comment about "While we were awaiting getUserMedia", thanks
| if (!this.effects.includes(effect) || currentTrack.readyState === 'ended') { | ||
| newTrack.stop(); | ||
| logger.log(`Effect was disposed during track re-acquisition, discarding new track.`); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
This is another re-check of the logic we have at the start of the method, right? Wondering how paranoid we need to be.
There was a problem hiding this comment.
This is the only guard that catches the rapid enable/disable race, both calls pass the entry guard because at the moment each starts the effect is still there and the track is still live. Only after the first call completes and stops currentTrack does the state change, and by then the second call is already past the entry guard waiting on getUserMedia. So I would consider keeping it for even that reason, I had that in the SDK smaple app
There was a problem hiding this comment.
Should we look at serializing these calls explicitly, like we do with sld/srd? like streams could have a serialization queue that some operations need to pass through so we could minimize awkward races like this?
There was a problem hiding this comment.
Good analogy, thank you for mentioning that! But the two cases are seems a bit different in terms of impact. With SLD/SRD, I believe race breaks the PC in a way you can't recover from. Here the worst that happens is the mic keeps slightly wrong settings until the user toggles again, and it fixes itself automatically (but webex-web-client handles that already).
Also I have checked webex-web-client and it already blocks rapid switches at the Redux level, so regular users can't actually hit this. SDK developers building their own UI could technically trigger it, but they'd need to click within a ~200ms window while the mic is reopening.
For now I'd lean towards keeping it simple and skipping the queue, but genuinely open to your take if you feel it's worth adding, happy to discuss.
| newTrack.enabled = currentTrack.enabled; | ||
| this.inputStream.removeTrack(currentTrack); | ||
| this.inputStream.addTrack(newTrack); | ||
| this.addTrackHandlers(newTrack); |
There was a problem hiding this comment.
Do we need a corresponding 'removeTrackHandlers' call for currentTrack?
There was a problem hiding this comment.
removeTrackHandlers is already there on line 134~, called before currentTrack.stop() — which one are you referring to?
| this.changeOutputTrack(this.inputTrack); | ||
| // If another effect is still loading, drop it too so it can't | ||
| // slip back into the chain once we've fallen back to raw mic. | ||
| this.loadingEffects.clear(); |
There was a problem hiding this comment.
Could this cause a problem when multiple effects are added/loading? Might there be a case where we don't want to impact another effect?
There was a problem hiding this comment.
In practice LocalAudioStream today only uses noise reduction effects and they don't chain together. But even if a second effect were loading, clearing it here is the right call I think. Effects depend on each other's output track, and we've just torn down the chain and switched to raw mic. A loading effect that completes after this would try to wire itself to a track that no longer exists. Dropping it and letting the caller retry is cleaner than letting a half-loaded effect slip into a broken chain. But again now it is only one effect per audio
There was a problem hiding this comment.
A loading effect that completes after this would try to wire itself to a track that no longer exists. Dropping it and letting the caller retry
would the caller be aware that this had happened though?
There was a problem hiding this comment.
Yes. The caller gets notified through two signals: the addEffect promise rejects with an ADD_EFFECT_FAILED error (the base class detects the cleared loading map and throws), and the stream also emits Ended. So nothing happens silently here, the caller can catch the rejection and react, or listen for Ended to know the stream is gone.
But this path is actually theoretical for audio today. Nobody calls addEffect on an audio stream while another effect is already loading.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5ab8833f7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c6a0386a0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df4388a855
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a772121c12
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
4 participants
Summary
Adds constraint handling to
LocalAudioStreamso audio effects can request specificMediaTrackConstraintson the microphone track. Since Chrome silently ignoresapplyConstraints()for audio processing properties (chromium#40555809), the handler re-acquires the track viagetUserMedia(). Original track settings are saved and restored when the effect releases its constraints on disable/dispose.Keeping the old track alive while calling
getUserMedia()causes Chrome to reuse the existing hardware capture session and silently ignore hardware-level constraints such asechoCancellation. The old track is stopped beforegetUserMedia()to force a fresh session and ensure all constraints are honored.What's changed
addConstraintHandlers— new private method onLocalAudioStreamwired up fromaddEffect(); listens forConstraintsRequired/ConstraintsReleasedevents from the effect and callsreacquireInputTrackreacquireInputTrack— stops the current track before callinggetUserMedia()with the merged baseline + effect constraints; restores mute state and wires the new track into the effect chainAppliableAudioConstraints— extended withsampleRate,sampleSize, andchannelCountfilterToSupportedConstraints— new helper that strips keys not recognized by the browser viagetSupportedConstraints()before passing baseline settings togetUserMedia()effects/loadingEffects/changeOutputTrack— visibility changed fromprivatetoprotectedinLocalStreamto allow access fromLocalAudioStreamgetUserMedia()or effect wiring fails, all effects are disposed andEndedis emitted; if the effect is disposed whilegetUserMedia()is in-flight, the new track is discarded andEndedis emitted to prevent a silent dead streamTesting
autoGainControl: false,noiseSuppression: false,echoCancellation: truegetUserMedia()→ new track discarded,EndedemittedSee code change in web-media-effects
You can use the sdk linked branch with these changes to test: agc-fix-branch-test