Skip to content

WIP: Unify device updates #1207

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+165 −32
Open

WIP: Unify device updates #1207

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
550b382
emit server device updates
jpsantosbh Apr 17, 2025
ddb9c67
new constraints event
jpsantosbh Apr 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 7 additions & 23 deletions packages/webrtc/src/BaseConnection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ import {
INVITE_VERSION,
VIDEO_CONSTRAINTS,
} from './utils/constants'
import { emitDeviceUpdatedEventHelper } from './utils/helpers'

export type BaseConnectionOptions = ConnectionOptions & BaseComponentOptions

Expand Down Expand Up @@ -693,29 +694,12 @@ export class BaseConnection<
prevAudioTrack,
prevVideoTrack,
}: EmitDeviceUpdatedEventsParams) {
if (newTrack.kind === 'audio') {
this.emit('microphone.updated', {
previous: {
deviceId: prevAudioTrack?.id,
label: prevAudioTrack?.label,
},
current: {
deviceId: newTrack.id,
label: newTrack.label,
},
})
} else if (newTrack.kind === 'video') {
this.emit('camera.updated', {
previous: {
deviceId: prevVideoTrack?.id,
label: prevVideoTrack?.label,
},
current: {
deviceId: newTrack.id,
label: newTrack.label,
},
})
}
emitDeviceUpdatedEventHelper({
Copy link
Collaborator

@iAmmar7 iAmmar7 Apr 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we have this helper function now, do you need the emitDeviceUpdatedEvents method? It seems to be just calling the helper function.

Copy link
Collaborator

@iAmmar7 iAmmar7 Apr 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@jpsantosbh jpsantosbh Apr 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, the /** @internal */ approach sounds good to me.

prevAudioTrack,
prevVideoTrack,
newTrack,
emitFn: this.emit.bind(this),
})
}

/**
Expand Down
36 changes: 35 additions & 1 deletion packages/webrtc/src/utils/helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
import { getLogger } from '@signalwire/core'
import { getUserMedia as _getUserMedia } from './getUserMedia'
import { assureDeviceId } from './deviceHelpers'
import { ConnectionOptions } from './interfaces'
import {
ConnectionOptions,
EmitDeviceUpdatedEventHelperParams,
} from './interfaces'
import { sdpHasAudio, sdpHasVideo } from './sdpHelpers'

// FIXME: Remove and use getUserMedia directly
Expand Down Expand Up @@ -100,3 +103,34 @@ export const filterIceServers = (
urls: disableUdpIceServers ? filterOutUdpUrls(server.urls) : server.urls,
}))
}

export const emitDeviceUpdatedEventHelper = ({
prevAudioTrack,
prevVideoTrack,
newTrack,
emitFn,
}: EmitDeviceUpdatedEventHelperParams) => {
if (newTrack.kind === 'audio') {
emitFn('microphone.updated', {
previous: {
deviceId: prevAudioTrack?.id,
label: prevAudioTrack?.label,
},
current: {
deviceId: newTrack.id,
label: newTrack.label,
},
})
} else if (newTrack.kind === 'video') {
emitFn('camera.updated', {
previous: {
deviceId: prevVideoTrack?.id,
label: prevVideoTrack?.label,
},
current: {
deviceId: newTrack.id,
label: newTrack.label,
},
})
}
}
10 changes: 9 additions & 1 deletion packages/webrtc/src/utils/interfaces.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import type { VideoPositions } from '@signalwire/core'
import type { EventEmitter, VideoPositions } from '@signalwire/core'
import {
BaseConnectionState,
VideoRoomDeviceEventParams,
Expand Down Expand Up @@ -108,6 +108,14 @@ export interface EmitDeviceUpdatedEventsParams {
prevVideoTrack?: MediaStreamTrack | null
}

export interface EmitDeviceUpdatedEventHelperParams
extends EmitDeviceUpdatedEventsParams {
emitFn: <E extends EventEmitter.EventNames<BaseConnectionEvents>>(
event: E,
...args: EventEmitter.EventArgs<BaseConnectionEvents, E>
) => boolean
}

export type UpdateMediaOptionsParams = Pick<
ConnectionOptions,
'video' | 'audio' | 'negotiateVideo' | 'negotiateAudio'
Expand Down
24 changes: 20 additions & 4 deletions packages/webrtc/src/workers/vertoEventWorker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
} from '@signalwire/core'

import { BaseConnection } from '../BaseConnection'
import { emitDeviceUpdatedEventHelper } from '../utils/helpers'

type VertoEventWorkerOnDone = (args: BaseConnection<any>) => void
type VertoEventWorkerOnFail = (args: { error: Error }) => void
Expand Down Expand Up @@ -129,11 +130,28 @@ export const vertoEventWorker: SDKWorker<
break
}
const { audio, video } = params.mediaParams

const prevAudioTrack = peer?.localAudioTrack?.clone()
const prevVideoTrack = peer?.localVideoTrack?.clone()
if (peer && video) {
peer.applyMediaConstraints('video', video)
peer.applyMediaConstraints('video', video).then(() => {
emitDeviceUpdatedEventHelper({
prevAudioTrack,
prevVideoTrack,
newTrack: peer?.localVideoTrack!,
emitFn: instance.emit,
})
})
}
if (peer && audio) {
peer.applyMediaConstraints('audio', audio)
peer.applyMediaConstraints('audio', audio).then(() => {
Copy link
Collaborator

@iAmmar7 iAmmar7 Apr 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. We are emitting the event without waiting for the promise to resolve, which means the event may be emitted before constraints are applied.
  2. The promise may fail; in that case, we should not emit the event.

Copy link
Collaborator

@iAmmar7 iAmmar7 Apr 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it is not urgent, I already have a PR open to support media APIs, so I could move this work into that PR.

Copy link
Collaborator Author

@jpsantosbh jpsantosbh Apr 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@iAmmar7 iAmmar7 Apr 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@iAmmar7 iAmmar7 Apr 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. If the "sender" or the "sender.track" is not available.
  2. 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?

Copy link
Collaborator Author

@jpsantosbh jpsantosbh Apr 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

@iAmmar7 iAmmar7 Apr 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

emitDeviceUpdatedEventHelper({
prevAudioTrack,
prevVideoTrack,
newTrack: peer?.localAudioTrack!,
emitFn: instance.emit,
})
})
}
break
}
Expand Down Expand Up @@ -164,6 +182,4 @@ export const vertoEventWorker: SDKWorker<
})
yield sagaEffects.fork(catchableWorker, action)
}

getLogger().trace('vertoEventWorker ended')
}
Loading