WIP: Unify device updates #1207
Conversation
|
| } | ||
| if (peer && audio) { | ||
| peer.applyMediaConstraints('audio', audio) | ||
| peer.applyMediaConstraints('audio', audio).then(() => { |
There was a problem hiding this comment.
peer.applyMediaConstraints is an RTCPeer class method that returns a promise. The actual constraints update happens inside this async method: https://github.com/signalwire/signalwire-js/blob/main/packages/webrtc/src/RTCPeer.ts#L391
I think we should move the emit logic over there, because
- We are emitting the event without waiting for the promise to resolve, which means the event may be emitted before constraints are applied.
- The promise may fail; in that case, we should not emit the event.
There was a problem hiding this comment.
If it is not urgent, I already have a PR open to support media APIs, so I could move this work into that PR.
There was a problem hiding this comment.
I don't think that moving it to https://github.com/signalwire/signalwire-js/blob/main/packages/webrtc/src/RTCPeer.ts#L391 will make any difference at all.
The emitDeviceUpdatedEventHelper is called inside the then callback, meaning the event will only be emitted after the peer.applyMediaConstraints promise is resolved, and only when the promise is resolved.
Moving the "emit logic" to peer.applyMediaConstraints won't be a problem since it will be just like the current implementation(Assuming the emit will be called after L391). But that will make peer.applyMediaConstraints less cohesive, because it will start behaving more like a "applyMediaConstraintsAndNotify".
IMO, unless there is a problem with this implementation, we should merge THIS PR instead of adding the logic to another PR, because having a smaller, punctual PR is better for reference in the future. Plus, the work is here already, why should we do it again?
Unless you are seeing other issues in this PR
There was a problem hiding this comment.
Sorry, I missed the ".then" part.
I was asking to move this logic to that PR only to avoid conflicts and the e2e tests coverage. Currently, we do not have e2e test coverage here, but the new PR has the coverage for these events.
Anyways, I can review the PR, and if that's work for you, we can merge this one.
| }, | ||
| }) | ||
| } | ||
| emitDeviceUpdatedEventHelper({ |
There was a problem hiding this comment.
Since we have this helper function now, do you need the emitDeviceUpdatedEvents method? It seems to be just calling the helper function.
There was a problem hiding this comment.
One more approach we can take is to declare this class method as a public method but mark it as internal (SDK follows this pattern and mark variable as /** @internal */), and then simply call the class method since you have access to the class instance.
This way, you can also avoid passing the emitFn since you will call the class method.
Up to you. Whatever approach you like to follow.
There was a problem hiding this comment.
Thanks, the /** @internal */ approach sounds good to me.
| } | ||
| if (peer && audio) { | ||
| peer.applyMediaConstraints('audio', audio) | ||
| peer.applyMediaConstraints('audio', audio).then(() => { |
There was a problem hiding this comment.
I like the idea of keeping the "applyConstraints" and "notify" separately. However, if you look at the definition of the applyMediaConstraints inside the RTCPeer class, you will see that it does not apply the constraints in two scenarios:
- If the "sender" or the "sender.track" is not available.
- If the "sender.track.readyState" is not live.
So, it is possible that we emit the event even though there was no changes made because of the above two conditions.
Should we make the method name "applyMediaConstraintsAndNotify" as you suggested in other comment?
There was a problem hiding this comment.
This is a good point!
Both are valid edge cases, if we don't update the constraints, we will emit an update event with the payload somewhat like "previous == current". Is that helpful for the developer using the SDK? Probably not much.
But I guess the real issue is... Why peer. applyMediaConstraints silently fail to apply the request? Shouldn't it throw an error?
There was a problem hiding this comment.
I guess that's how constraints are applied in the RTC context with a fallback strategy. Even if you see the updateCamera or updateMicrophone methods, these method calls the recursive method updateConstraints. This basically tries to apply the new constraints, but if that fails, the SDK silently falls back to the previous constraints.
Is this a good/bad approach? IMO, it's just a design decision, there are pros/cons with both approaches and we can always discuss if we need to update it.
Also, the worker you see here is not that straightforward; vertoEventWorker. At a time, the SDK can handle multiple RTC peers, and each peer has this worker. The server sends the event specifying the callID that can be targeted at any peer. That peer may or may not have the tracks available yet, or there can be a bunch of other details that we need to consider.
| @@ -368,30 +368,39 @@ export default class RTCPeer<EventTypes extends EventEmitter.ValidEventTypes> { | |||
| try { | |||
| const sender = this._getSenderByKind(kind) | |||
| if (!sender || !sender.track) { | |||
| return this.logger.info( | |||
| 'No sender to apply constraints', | |||
| // TODO this is breaking chance we need to eval if we want to have it on VideoSDK | |||
There was a problem hiding this comment.
| // TODO this is breaking chance we need to eval if we want to have it on VideoSDK | |
| // TODO: this is a breaking change. We need to evaluate if we want to have it on VideoSDK |
Description
unify device updates events from the server with local updates.
Type of change
Code snippets
In case of new feature or breaking changes, please include code snippets.