Fix: Races that could cause double publication or mute state desyncs #3632
Conversation
BillCarsonFr
added
the
PR-Task
| * This starts audio and video tracks. They will be reused when calling `requestPublish`. | ||
| */ | ||
| startTracks: () => Behavior<LocalTrack[]>; | ||
| startTracks: () => Behavior<void>; |
There was a problem hiding this comment.
Why does it return in the first place if it is a void behavior?
Would a void promise or just a void return make more sense?
| const startTracks = (): Behavior<void> => { | ||
| trackStartRequested.resolve(); | ||
| return tracks$; | ||
| return constant(undefined); | ||
| }; |
There was a problem hiding this comment.
Yes lets remove the return type entirely or add a comment that this should return tracks$
| // XXX Why is that? | ||
| // else { | ||
| // try { | ||
| // await publisher?.stopPublishing(); | ||
| // } catch (error) { | ||
| // setLivekitError(new UnknownCallError(error as Error)); | ||
| // } | ||
| // } |
There was a problem hiding this comment.
The commented section now is redundant with else if (isPublishing)
But the whole stopPublishing block is for when shouldJoinAndPublish becomes false. (I think we dont need this case now) but i would expect, that setting the subject to false stops the publications.
There was a problem hiding this comment.
Thx I had questions about that. Like do we really want to stop / restart publishing? or even start/stop tracks.
Or is it more that we need a destroy publisher that should clean itself?
| // const hasTracks = tracks.length > 0; | ||
| // let trackState: TrackState = TrackState.WaitingForUser; | ||
| // if (hasTracks && shouldStartTracks) trackState = TrackState.Ready; | ||
| // if (!hasTracks && shouldStartTracks) trackState = TrackState.Creating; |
There was a problem hiding this comment.
I think this also can get a generic comment.
But I prefer just removing the core entirely. Maybe linking this PR instead as reference.
| const enableTracks = async (): Promise<void> => { | ||
| if (audio && video) { | ||
| // Enable both at once in order to have a single permission prompt! | ||
| await lkRoom.localParticipant.enableCameraAndMicrophone(); | ||
| } else if (audio) { | ||
| await lkRoom.localParticipant.setMicrophoneEnabled(true); | ||
| } else if (video) { | ||
| await lkRoom.localParticipant.setCameraEnabled(true); | ||
| } | ||
| return; | ||
| }; |
There was a problem hiding this comment.
const enableTracks = ():void => {
if (audio && video) {
// Enable both at once in order to have a single permission prompt!
void lkRoom.localParticipant.enableCameraAndMicrophone();
} else if (audio) {
void lkRoom.localParticipant.setMicrophoneEnabled(true);
} else if (video) {
void lkRoom.localParticipant.setCameraEnabled(true);
}
};
We can just explicitly void the promises.
There was a problem hiding this comment.
right there used to have a .then() block on the enable track but was removed here 610af43
I guess I can just voids
|
@toger5 I have addressed your comments and they were not blocking so I am merging now as you are OOO and I'd like it to sit a bit on dev. |
Problem:
Livekit's
LocalParticipantkind of offer two different levels of API.createTracksandpublishTrack, then accessing directly the tracks to callmute/unmuteetc..enableCameraAndMicrophone/setCameraEnabled/setMicrophoneEnabled. These one will automatically creates the tracks, manage them, remember if there is any pending publications. For example callingsetCameraEnabled(false)will check if there is already a track or if there is a pending publication and waits for it to complete.Our
Publisher.tswants control on track creation and publication, so it is using the Low-Level APIs to createTracks.But it is also using the High-Level ones in the mutestates handler with
setCameraEnabled/setMicrophoneEnabled.This mixed usage of API levels cause problems. For example the
Publisherwill create a track, but meanwhile the mutestate will callsetCameraEnabled(true), the high level is not aware of the tracks that thePublisherhave created so it will create a new one. Then later thePublisherwill publish the one it manages.This can be observed in rage-shake as per this log
publishing a second track with the same source:cameraProposal
We have to stop mixing high-level and low-level.
This PR is an attempt at using only the High-Level API.
It is sometimes a bit odd because the
Publisherwant to have more control than what the API allows (like create track and wait to publish)We could eventually create an alternative PR to use the low-level API, but this will then require us to re-built a proper track management system (like tracking pending publications)