Bug fix | Race on initial publishing causing mute state to be desync#3622
Bug fix | Race on initial publishing causing mute state to be desync#3622BillCarsonFr wants to merge 3 commits intolivekitfrom
Conversation
There was a problem hiding this comment.
I think this is the main place we need to use mute/unmute?
**
* Observe changes in the mute states and update the LiveKit room accordingly.
* @param scope
* @private
*/
private observeMuteStates(scope: ObservableScope): void {
const lkRoom = this.connection.livekitRoom;
this.muteStates.audio.setHandler(async (desired) => {
try {
await lkRoom.localParticipant.setMicrophoneEnabled(desired);
} catch (e) {
this.logger.error("Failed to update LiveKit audio input mute state", e);
}
return lkRoom.localParticipant.isMicrophoneEnabled;
});
this.muteStates.video.setHandler(async (desired) => {
try {
await lkRoom.localParticipant.setCameraEnabled(desired);
} catch (e) {
this.logger.error("Failed to update LiveKit video input mute state", e);
}
return lkRoom.localParticipant.isCameraEnabled;
});
}
| let scope: ObservableScope; | ||
|
|
||
| beforeEach(() => { | ||
| scope = new ObservableScope(); | ||
| }); | ||
|
|
||
| afterEach(() => scope.end()); |
There was a problem hiding this comment.
You might also find the testScope utility to be convenient
I wonder if I don't prefer that we always use the high level API, it is doing quite a bunch of things. |
toger5
left a comment
toger5
left a comment
There was a problem hiding this comment.
Our race is:
- publishTrack (
await lkRoom.localParticipant.publishTrack(track)) - enableAudio
might be called at the same time once at the mute state handler and once in the createAndSetupTracks method.
Is this race also fixed by this PR?
| this.logger.error("Failed to create tracks", error); | ||
| }); | ||
| } | ||
| // TODO why throw here? should we just do nothing? |
There was a problem hiding this comment.
Yes I agree.
If we do the conditional check in here anyways the throw motivates/requests, that we do an additional check to never call this method if both are false. So the throw requests us to do it twice which is odd.
Removing the throw and adding a docstring to the function:
This function will no-op in case both (audio and video) are not enabled (muted)
seems like the way to go. We might still want to log in case we end up not creating tracks that could help us debug that case.
| // or only use the low level create/publish APIs and have our own pending publication protection. | ||
| // Maybe we could change the livekit api to pre-load tracks without publishing them yet? | ||
| // Are we sure this is needed at all? What are the gains? | ||
| let isEnabled: boolean; |
There was a problem hiding this comment.
this should probably be request_enabled or should_enable
|
|
||
| if (!isEnabled) { | ||
| // TODO should we also drop it? | ||
| // I believe the high-level LiveKit APIs will recreate a track? |
There was a problem hiding this comment.
That definitly needs checking. what does enableVideo/enabledAudio do if there is a muted track?
- unmute
- create new track
|
@BillCarsonFr What is the status of this one. ha this be fixed somehwere else? |
We can close, we are using the highlevel API now #3632 |
3 participants
No description provided.