Bitrate negotiation on stream startup #425
Conversation
(offer/answer) from the player - Fixing bitrate values units and parsing.
| const minBitrate = 1000 * this.config.getNumericSettingValue(NumericParameters.WebRTCMinBitrate); | ||
| const maxBitrate = 1000 * this.config.getNumericSettingValue(NumericParameters.WebRTCMaxBitrate); | ||
|
|
||
| this.webSocketController.sendWebRtcOffer(offer, minBitrate, maxBitrate); |
There was a problem hiding this comment.
Rather than appending more parameters here (as I am certain we will want more in the future) can we instead pass an offerOptions, offerParams, offerConfig, or something like this as the additional parameter to this function/
| const minBitrate = 1000 * this.config.getNumericSettingValue(NumericParameters.WebRTCMinBitrate); | ||
| const maxBitrate = 1000 * this.config.getNumericSettingValue(NumericParameters.WebRTCMaxBitrate); | ||
|
|
||
| this.webSocketController.sendWebRtcAnswer(answer, minBitrate, maxBitrate); |
There was a problem hiding this comment.
Same comment as above about an options param.
| * Good for excluding default values or hidden internals. | ||
| * Return undefined to exclude the property completely. | ||
| */ | ||
| sendFilter(key: string, value: any) { |
There was a problem hiding this comment.
Good idea. Can comment have an example of how it should be used.
| @@ -1551,7 +1551,11 @@ export class WebRtcPlayerController { | |||
| 'Sending the offer to the Server', | |||
| 6 | |||
| ); | |||
| this.webSocketController.sendWebRtcOffer(offer); | |||
|
|
|||
| const minBitrate = 1000 * this.config.getNumericSettingValue(NumericParameters.WebRTCMinBitrate); | |||
There was a problem hiding this comment.
Consider naming as minBitrateBps, although technicaly bitrate implies bps, the word bitrate is also used for kilobits and megabits quite often so I think the extra clarity here helps.
| sendWebRtcOffer(offer: RTCSessionDescriptionInit, minBitrate: number, maxBitrate: number) { | ||
| const payload = new MessageSend.MessageWebRTCOffer(offer, minBitrate, maxBitrate); | ||
| sendWebRtcOffer(offer: RTCSessionDescriptionInit, extraParams: ExtraOfferParameters) { | ||
| const payload = new MessageSend.MessageWebRTCOffer(offer, extraParams.minBitrateBps, extraParams.maxBitrateBps); |
There was a problem hiding this comment.
Can the constructor of MessageSend.MessageWebRTCOffer and MessageSend.MessageWebRTCAnswer take the ExtraParameters object for the same reason (so constructor doesn't grow overlong as we add more).
| @@ -134,7 +149,7 @@ export class MessageWebRTCAnswer extends MessageSend { | |||
| /** | |||
| * @param answer - Generated Web RTC Offer | |||
| */ | |||
| constructor(answer: RTCSessionDescriptionInit, minBitrate: number, maxBitrate: number) { | |||
| constructor(answer: RTCSessionDescriptionInit, extraParams: ExtraOfferParameters) { | |||
There was a problem hiding this comment.
Offer params passed to answer?
There was a problem hiding this comment.
Also comment:
@ param answer - Generated Web RTC Offer
Seems wrong in the same way, talking about offer in answer constructor.
|
This PR closes this issue: #275 |
Relevant components:
Problem statement:
Setting the bitrate values before starting the stream did not work.
Solution
Added min/max bitrate values to the answer signalling message so the streamer can extract them and set them when needed.
Documentation
Test Plan and Compatibility
Will probably need a QA pass.