Increase should never decrease

[mengelbart]

When the controller decides to increase the rate, it should not decrease, even if the measured transmission rate was low. The rate could be lower because there was not enough data to send.

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (450ac84) 79.73% compared to head (d095871) 79.64%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #221      +/-   ##
==========================================
- Coverage   79.73%   79.64%   -0.09%     
==========================================
  Files          68       68              
  Lines        3587     3587              
==========================================
- Hits         2860     2857       -3     
- Misses        602      604       +2     
- Partials      125      126       +1     

Flag	Coverage Δ
go	79.56% <100.00%> (-0.09%)	:arrow_down:
wasm	77.61% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[mengelbart]

Draft because we might want to add a test case.

And I don't know what's wrong with that failing test...

[alexpokotilo]

I've checked and sometimes this branch: return int(math.Min(float64(c.target+increase), 1.5*float64(c.latestReceivedRate))) really produced value less than c.target. So in case if we are in "increase" state this fix prevents bandwidth from looping over latestReceivedRate if latestReceivedRate is not enough to increase bandwidth

[alexpokotilo]

We have a problem with a.variance calculation in *func (a exponentialMovingAverage) update(value float64) currently we have

a.variance = (1 - decreaseEMAAlpha) * (a.variance + decreaseEMAAlpha*x*x) => a.variance = a.variance + decreaseEMAAlpha*x*x - decreaseEMAAlpha*a.variance - decreaseEMAAlpha*decreaseEMAAlpha*x*x => a.variance = (1 - decreaseEMAAlpha)a.variance + decreaseEMAAlphax*x *(1 - decreaseEMAAlpha)`

but variance should be a.variance = decreaseEMAAlpha* (x*x) + (1-decreaseEMAAlpha)* a.variance

another problem that we never reset latestDecreaseRate and potentially could always fall into

if c.latestDecreaseRate.average > 0 && float64(c.latestReceivedRate) > c.latestDecreaseRate.average-3*c.latestDecreaseRate.stdDeviation &&
		float64(c.latestReceivedRate) < c.latestDecreaseRate.average+3*c.latestDecreaseRate.stdDeviation {

condition. But standard clearly stats that we MUST reset latestDecreaseRate and go back to "multiplicative increase state".

If R_hat(i) increases above three standard deviations of the average max bitrate, we assume that the current congestion level has changed, at which point we reset the average max bitrate and go back to the multiplicative increase state.

Now if we ever reach "func (c rateController) decrease() int" function we will never reset latestDecreaseRate and hence if "c.latestReceivedRate" will be close to c.latestDecreaseRate.average calculated previously(an hour ago, a day ago) we will reset target bitrate to 1.5 c.latestDecreaseRate even if we are in "increase" state.

So we must reset c.latestDecreaseRate to not set target bitrate to 1.5 * c.latestReceivedRate in "increase" state if our current c.latestReceivedRate fall in clamps of latestDecreaseRate calculated on last overuse state.

[alexpokotilo]

Draft because we might want to add a test case. And I don't know what's wrong with that failing test...

Checker just want you to set go version to 1.19. d095871

Interceptor's main go.mod has 1.15. I don't know how last commit two weeks ago passed this check

last commit to interceptor main has these tests "Test / test-i386 (1.21) / Go i386 1.21 (push) Failing after 43s" failed as well. I don't think this is related to this fix somehow

[alexpokotilo]

I've submitted new request #226 containing this fix

[joshforbes]

We're currently using a forked version of Pion with this change. As we reworked our video streaming code, we needed this change to make bandwidth estimation work smoothly (@danielslee can provide more details if necessary). Just wondering if there is a path to getting this merged? I see that 226 got closed but we didn't need the other changes in 226 - just this one.

Thanks! 🙏