PCTransportManager #909
PCTransportManager #909
Conversation
…ient-sdk-js into lukas/encapsulate-pctransport
🦋 Changeset detectedLatest commit: d857e98 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
size-limit report 📦
|
lukasIO
requested review from
davidzhao and
boks1971
and removed request for
davidzhao
There was a problem hiding this comment.
First look, looks good! Much easier to follow. Left some minor comments.
I will read this again. One thing I noticed is a bunch more await. Example
- if (!this.publisher) {
- this.configure(joinResponse);
+ if (!this.pcManager) {
+ await this.configure(joinResponse);
Were those places not handling things properly or the new code needs that behaviour?
src/room/Room.ts
Outdated
| return; | ||
| } | ||
| const previousAnswer = this.engine.pcManager.subscriber.getLocalDescription(); | ||
| const previousOffer = this.engine.pcManager.subscriber.getRemoteDescription(); |
There was a problem hiding this comment.
In publisher only case, will this be undefined? Now that subscriber PC is created always, wondering if these will be empty/undefined.
There was a problem hiding this comment.
True, that would be undefined for publisher only. What do you think is the right way to handle this? Fall back to publisher if subscriber is undefined?
There was a problem hiding this comment.
Yeah, I think it should include the publisher parts. previousAnswer and previousOffer are subscriber specific things needed for migration. The code below returns if there is no previousAnswer. I think it should proceed to send the message, but just not fill out the fields that are nil/undefined. Server will see nil in those fields and ignore them, but will use publish_tracks and data_channels (publisher only data channels) from the message.
There was a problem hiding this comment.
LocalParticipant.dataChannelsInfo() I believe should just populate with publisher side data channels as subscriber is not there. That should be fine.
| return answer; | ||
| } | ||
|
|
||
| updateConfiguration(config: RTCConfiguration, iceRestart?: boolean) { |
There was a problem hiding this comment.
could there be separate configs for publisher, subscriber?
There was a problem hiding this comment.
Previously we never had a way to set different configs either, so I assumed we want to keep the behaviour.
Can you think of a scenario where separate configs would be useful?
There was a problem hiding this comment.
Can't think of any, but just wanted to check. We do have simulation scenarios where we change transport type. Wondering if we will have something down the line which will set different transport for different peer connection. So, was thinking about keeping it flexible. Can change if it is required down the line.
| resolve(); | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Maybe add Publisher or Subscriber to method names. Ended up doing that in transport manager on server side as I found it easier to understand from the caller (example). I agree that it is implicit for several functions, but adding to the method name reduced cognitive load for me :-) . For example, in this file, I believe addTransceiver, addTrack, removeTrack are all on publisher. Making them something like addPublisherTransceiver, addPublisherTrack` etc. makes it very clear (at least to me :-) )
There was a problem hiding this comment.
should have read all your comments before writing mine. Yeah, that makes sense to me naming wise. As mentioned above the thought was to hide that fact within the pcManager. With your initial feeling that it's easier to reason about with publisher/subscriber in the method names, I'm happy to do that change. (the whole purpose is to make the whole thing easier to reason about, so thanks for that feedback)
| const previousState = this.state; | ||
|
|
||
| const connectionStates = this.requiredTransports.map((tr) => tr.getConnectionState()); | ||
| if (connectionStates.every((st) => st === 'connected')) { |
There was a problem hiding this comment.
do we care about disconnected state at all? I know for Safari, it's not particularly reliable.
is it intentional we ignore disconnected as a state?
There was a problem hiding this comment.
Good question, previously we only reacted to failed. Might make sense though to include it. In that case, would you see value in an additional state for Disconnected in the PCTransportState enum or should we treat it as closed ?
There was a problem hiding this comment.
I think ignoring it as you do is probably the right behavior, it'll always turn from disconnected to failed. so it's better to not introduce a new behavior as part of an already-large change.
| @@ -1551,12 +1551,12 @@ class Room extends (EventEmitter as new () => TypedEmitter<RoomEventCallbacks>) | |||
| } | |||
|
|
|||
| private sendSyncState() { | |||
There was a problem hiding this comment.
I'm wondering if this could move into RTCEngine or TransportManager. I understand it'll need a list of Tracks, but perhaps that could be a function that Room provides to RTCEngine?
There was a problem hiding this comment.
addressed this in f57d7c1
Hardly any new behaviour needed. The changes are just surfacing the fact that some of the underlying methods had been async before already. I don't think this was actually causing any issues, but even just for error handling, it felt better to await things where possible |
Second phase for connection logic refactor.
This PR introduces a PCManager. The primary idea behind it is to