Estimate bandwidth in connection and don't limit by REMBs #1726
Conversation
…imateBandwidthInConnection
…imateBandwidthInConnection
| @@ -57,32 +58,25 @@ void BandwidthEstimationHandler::disable() { | |||
| } | |||
|
|
|||
| void BandwidthEstimationHandler::notifyUpdate() { | |||
| ELOG_DEBUG("NotifyUPDATE"); | |||
There was a problem hiding this comment.
should we use a more descriptive message?
| @@ -229,19 +223,39 @@ void BandwidthEstimationHandler::pickEstimator() { | |||
| } | |||
|
|
|||
| void BandwidthEstimationHandler::sendREMBPacket() { | |||
| sink_ssrc_ = 0; | |||
There was a problem hiding this comment.
why do we need to use a class variable?
There was a problem hiding this comment.
you are right. I kept if for consistency but it does make more sense to have it as a local variable.
| }); | ||
|
|
||
| if (sink_ssrc_ == 0) { | ||
| ELOG_WARN("No SSRC available to send REMB"); |
There was a problem hiding this comment.
I wonder if there could be cases where we should not notify with a warning here.
| @@ -252,6 +266,7 @@ void BandwidthEstimationHandler::sendREMBPacket() { | |||
|
|
|||
| void BandwidthEstimationHandler::OnReceiveBitrateChanged(const std::vector<uint32_t>& ssrcs, | |||
| uint32_t bitrate) { | |||
| ELOG_WARN("Onreceive bitrate %lu", bitrate); | |||
There was a problem hiding this comment.
consider using ELOG_DEBUG here
| @@ -68,6 +77,77 @@ const Stream = (altConnectionHelpers, specInput) => { | |||
| that.local = true; | |||
| } | |||
|
|
|||
| const setMaxVideoBW = (maxVideoBW) => { | |||
| if (that.local) { | |||
| const translated = (maxVideoBW * 1000 * 0.90) - (50 * 40 * 8); | |||
There was a problem hiding this comment.
I think we could add a reference here to know where this formula comes from
| @@ -14,7 +14,7 @@ const BaseStack = (specInput) => { | |||
| }; | |||
| that.getNegotiationLogs = () => logs.reduce((a, b) => `${a}'\n'${b}`); | |||
|
|
|||
| log.debug(`message: Starting Base stack, spec: ${JSON.stringify(specBase)}`); | |||
| log.warning(`message: Starting Base stack, spec: ${JSON.stringify(specBase)}`); | |||
There was a problem hiding this comment.
consider using log.debug here again
|
|
||
| for (std::size_t i = 0; i < source_ssrcs_.size(); i++) { | ||
| ELOG_DEBUG("Setting REMBFeedSSRC %u to ssrc %u, size %u", i, source_ssrcs_[i], source_ssrcs_.size()); | ||
| remb_packet_.setREMBFeedSSRC(i, source_ssrcs_[i]); |
| const nativeSenderParameters = []; | ||
| const requestedLayers = Object.keys(videoSenderLicodeParameters).length || | ||
| defaultSimulcastSpatialLayers; | ||
| const isScreenshare = that.hasScreen(); |
| that.maxVideoBW = translated; | ||
| } else { | ||
| that.maxVideoBW = maxVideoBW; | ||
| } |
There was a problem hiding this comment.
should we reapply the encoder parameters here?
There was a problem hiding this comment.
We don't always want to apply it immediately. For instance, if you specify a maxVideoBW in room.publish we then generate the initial configuration and then include those in the transceiver.
| nativeStream.transceivers.push(transceiver); | ||
| const parameters = transceiver.sender.getParameters() || {}; | ||
| parameters.encodings = streamInput.generateEncoderParameters(); | ||
| console.warn('parameters to set', parameters); |
There was a problem hiding this comment.
Consider removing this line
Description
This PR fundamentally changes the way
maxVideoBWworks:[] It needs and includes Unit Tests
Changes in Client or Server public APIs
The APIs are the same but
maxVideoBWin the publisher now has a slightly different meaning, particularly in simulcast. Now that bandwidth will limit the top layer instead of limiting the whole stream (the addition of all the layers bitrates). When not in simulcast the result is more or less the same since we only have one layer.If more control over layer bitrate is needed, apps should use
updateSimulcastLayersBitrateto set the bitrate for each layer.[] It includes documentation for these changes in
/doc.