-
Notifications
You must be signed in to change notification settings - Fork 6.9k
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
fix(conference) Do not add audio track from screenshare to redux. #11312
Conversation
In audio-only screenshare mode when there is no local audio track from mic present, do not add the audio track from screenshare to redux. Adding the track to redux will sync the track mute state to that of /base/media and show that the mic is unmuted even when that is not the case. Fixes jitsi#10706.
Wait, we eventually need all tracks from web to be on Redux. Doesn't this take us ever so slightly away from that? |
They are in redux in the new multi-stream mode but in the legacy mode, the desktop audio stream for web clients is stored as a local var in conference.js. Only the audio track from the microphone is in /base/media but replacing it with the desktop audio track in this case shouldn't be allowed. |
@@ -1633,7 +1633,8 @@ export default { | |||
// In case there was no local audio when screen sharing was started the fact that we set the audio stream to | |||
// null will take care of the desktop audio stream cleanup. | |||
} else if (this._desktopAudioStream) { | |||
await this.useAudioStream(null); | |||
await room.replaceTrack(this._desktopAudioStream, null); |
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.
It seems that the problem was deeper then just the out of sync mute button.
While we do audio sharing, on the initial unmute the newly created mic track replaces the audio sharing one which
results in a inconsistent state i.e. screen audio is no longer shared and if we decide to stop screen sharing the screen share button state is frozen to always on.
I've got a pretty good idea of what the problem is and will attempt to fix it.
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.
Yes, I had noticed that yesterday looking at the code. Audio mute/unmute actions do not apply/disable the mixer effect at all and isn't this how it has always been implemented ? I wasn't sure if it was by design. Anyhow this PR addresses the issue mentioned in the bug report and that is why I went ahead and merged it. Can you please keep the multi-stream mode in mind when you are attempting to fix the issue that you described ? You can enable it via config.flags.sendMultipleVideoStreams=true, thanks!
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.
Thanks for the warning. Regarding the design choice, it switched back and forth a couple of times.
Initially the mute state was, as it is now, applied only to the mic audio, then we switched to it being applied to both screen audio and mic audio, then we switched back to the former.
In audio-only screenshare mode when there is no local audio track from mic present, do not add the audio track from screenshare to redux. Adding the track to redux will sync the track mute state to that of /base/media and show that the mic is unmuted even when that is not the case. Fixes #10706.