-
Notifications
You must be signed in to change notification settings - Fork 18
Allow developers to control the audio codecs #1190
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.
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
Changes from 12 commits
a817a45
1d8c330
ff9f615
51ab08a
0670610
950e8df
c65f62d
0941f1d
94623fc
20bcb3e
c7a4220
07c55d2
db83282
93d1fb0
78a1f78
81a1b4c
8e924e0
8d0f26e
035d424
dc0c11f
51c5eea
6e37d34
00b65d9
aad67a7
41817e3
b8704ad
255f0cf
37e545d
a514e78
5f99076
5d5b474
43d5abe
eb67f33
b54aa5a
4d49452
9b38f4a
7e5704c
9e7335b
a1ec706
88534ea
eb4b374
aede240
97afc46
d9f1610
bd5996f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ import { | |
| VertoBye, | ||
| VertoSubscribe, | ||
| VideoRoomSubscribedEventParams, | ||
| isPlaybackRate | ||
| } from '@signalwire/core' | ||
| import { MakeRoomOptions } from '../video' | ||
| import { createFabricRoomSessionObject } from './FabricRoomSession' | ||
|
|
@@ -154,12 +155,39 @@ export class WSClient extends BaseClient<{}> implements WSClientContract { | |
| return room | ||
| } | ||
|
|
||
| private buildOutboundCall(params: DialParams & { attach?: boolean }) { | ||
| const [pathname, query] = params.to.split('?') | ||
| private _validateDialParams(params: DialParams) { | ||
|
||
| const [pathname] = params.to.split('?') | ||
|
|
||
| if (!pathname) { | ||
| throw new Error('Invalid destination address') | ||
| } | ||
|
|
||
| if (params.opusMaxPlaybackRate) { | ||
| if (!isPlaybackRate(params.opusMaxPlaybackRate)) { | ||
| throw new Error('Invalid opusMaxPlaybackRate') | ||
| } | ||
| if (typeof params.audio === 'object') { | ||
| if ( | ||
| params.audio?.sampleRate && | ||
| params.audio?.sampleRate !== params.opusMaxPlaybackRate | ||
| ) { | ||
| throw new Error( | ||
| 'Mismatching parameters: opusMaxPlaybackRate, audio.sampleRate' | ||
| ) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (params.opusMaxAverageBitrate && params.opusMaxAverageBitrate <= 0) { | ||
| throw new Error('Invalid opusMaxPlaybackRate') | ||
jpsantosbh marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
| } | ||
|
|
||
| private buildOutboundCall(params: DialParams & { attach?: boolean }) { | ||
| this._validateDialParams(params) | ||
|
|
||
| const [, query] = params.to.split('?') | ||
|
|
||
| let video = false | ||
| let negotiateVideo = false | ||
|
|
||
|
|
@@ -186,6 +214,8 @@ export class WSClient extends BaseClient<{}> implements WSClientContract { | |
| attach: params.attach ?? false, | ||
| disableUdpIceServers: params.disableUdpIceServers || false, | ||
| userVariables: params.userVariables || this.wsClientOptions.userVariables, | ||
| opusMaxPlaybackRate: params.opusMaxPlaybackRate, | ||
| opusMaxAverageBitrate: params.opusMaxAverageBitrate, | ||
| }) | ||
|
|
||
| // WebRTC connection left the room. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,10 +5,10 @@ import { | |
| filterIceServers, | ||
| } from './utils/helpers' | ||
| import { | ||
| sdpStereoHack, | ||
| sdpBitrateHack, | ||
| sdpMediaOrderHack, | ||
| sdpHasValidCandidates, | ||
| updateSDPForOpus, | ||
| } from './utils/sdpHelpers' | ||
| import { BaseConnection } from './BaseConnection' | ||
| import { | ||
|
|
@@ -71,6 +71,8 @@ export default class RTCPeer<EventTypes extends EventEmitter.ValidEventTypes> { | |
| this.options | ||
| ) | ||
|
|
||
| this._validateOptions() | ||
|
|
||
| this._onIce = this._onIce.bind(this) | ||
| this._onEndedTrackHandler = this._onEndedTrackHandler.bind(this) | ||
|
|
||
|
|
@@ -86,6 +88,16 @@ export default class RTCPeer<EventTypes extends EventEmitter.ValidEventTypes> { | |
| this.rtcConfigPolyfill = this.config | ||
| } | ||
|
|
||
| private _validateOptions() { | ||
| if ( | ||
| this.options.useStereo === true && | ||
| typeof this.options.audio === 'object' && | ||
| (this.options.audio.channelCount ?? 2) != 2 | ||
jpsantosbh marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| ) { | ||
| throw new Error('Mismatch params: useStereo, audio.channelCount') | ||
| } | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems to be a utility function not associated with the class instance. Should we move this to a utility file? Also, should we throw an error from the constructor or when the actual API where this is used is called? If you are expecting this to be passed by the developer, you will need to expose the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The definition of the "utility function".
The actual API that creates this instance. For CF SDK, that would be the All the above were questions: if you think that you don't need this, then we can leave it here as it is. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, no need to expose the param to the developer that is not the goal of this PR. I'm still confused about external "utility" idea... This is the same case as above. This isn't a generic "utility" this is a "guard" for the assumptions/implementation defined in this class alone. |
||
|
|
||
| get options() { | ||
| return this.call.options | ||
| } | ||
|
|
@@ -800,14 +812,13 @@ export default class RTCPeer<EventTypes extends EventEmitter.ValidEventTypes> { | |
| } | ||
|
|
||
| private _setLocalDescription(localDescription: RTCSessionDescriptionInit) { | ||
| const { | ||
| useStereo, | ||
| googleMaxBitrate, | ||
| googleMinBitrate, | ||
| googleStartBitrate, | ||
| } = this.options | ||
| if (localDescription.sdp && useStereo) { | ||
| localDescription.sdp = sdpStereoHack(localDescription.sdp) | ||
| const { googleMaxBitrate, googleMinBitrate, googleStartBitrate } = | ||
| this.options | ||
| if (localDescription.sdp) { | ||
jpsantosbh marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| localDescription.sdp = updateSDPForOpus( | ||
| localDescription.sdp, | ||
| this.options | ||
| ) | ||
| } | ||
| if ( | ||
| localDescription.sdp && | ||
|
|
@@ -832,8 +843,11 @@ export default class RTCPeer<EventTypes extends EventEmitter.ValidEventTypes> { | |
| } | ||
|
|
||
| private _setRemoteDescription(remoteDescription: RTCSessionDescriptionInit) { | ||
| if (remoteDescription.sdp && this.options.useStereo) { | ||
| remoteDescription.sdp = sdpStereoHack(remoteDescription.sdp) | ||
| if (remoteDescription.sdp) { | ||
| remoteDescription.sdp = updateSDPForOpus( | ||
| remoteDescription.sdp, | ||
| this.options | ||
| ) | ||
| } | ||
| if (remoteDescription.sdp && this.instance.localDescription) { | ||
| remoteDescription.sdp = sdpMediaOrderHack( | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.