Skip to content
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

Fix bandwidth probation dead state #1031

Merged
merged 3 commits into from
Apr 14, 2023

Conversation

vpalmisano
Copy link
Contributor

@vpalmisano vpalmisano commented Mar 24, 2023

Running some tests with high packet loss (~20%), in most cases the server stops sending any video packet and we never exit from that state, even if the network link improves.

Looking at the ALR detector (that should be activated when there are no RTP packets sent over the transport), it seems that it is falling into a dead state:

  1. in NetworkControlUpdate GoogCcNetworkController::OnProcessInterval we periodically call start_time_ms = alr_detector_->GetApplicationLimitedRegionStartTime() setting the output value to probe_controller_->SetAlrStartTimeMs(start_time_ms)
  2. the AlrDetector state is set by AlrDetector::OnBytesSent, so when the transport stops sending packets, we could end up in this state:
} else if (alr_budget_.budget_ratio() < stop_budget_level_ratio_ &&
           alr_started_time_ms_) {
  state_changed = true;
  alr_started_time_ms_.reset();
}
  1. at this point GoogCcNetworkController::OnProcessInterval calls probe_controller_->SetAlrStartTimeMs(<empty value>) with this empty value set into the ProbeController, and the ProbeController::Process never calls InitiateProbing, so no more probing is created;
  2. the bandwidth estimation is never tested again, so we never resume sending packets.

With this PR we use a alr_timeout_ value (set to 3s by default). After that timeout, the ALR resumes sending probes.

@ibc
Copy link
Member

ibc commented Mar 24, 2023

Is this something available in latest libwebrtc version? You are basically changing libwebrtc. Also, we have an ongoing PR #922 upgrading to latest libwebrtc so I don't see a point in doing it in current very old version.

@ibc
Copy link
Member

ibc commented Mar 24, 2023

@sarumjanuch you may want to see this.

@vpalmisano
Copy link
Contributor Author

Is this something available in latest libwebrtc version?

Yes, it seems the same of the latest libwebrtc code: https://webrtc.googlesource.com/src/+/refs/heads/main/modules/congestion_controller/goog_cc/alr_detector.cc

@sarumjanuch
Copy link
Contributor

I am aware of this. In my branch it is handled. This is from 18.11.2022:

image

@ibc
Copy link
Member

ibc commented Mar 25, 2023

Ok, let me take a proper look on Monday and merge it in v3. But it will generate conflicts in your branch @sarumjanuch, is it ok?

@sarumjanuch
Copy link
Contributor

Don't worry at all. My branch is already deviated a lot, both, from ms and libwebrtc.

@jmillan jmillan self-requested a review April 14, 2023 08:29
@jmillan jmillan merged commit 640fa34 into versatica:v3 Apr 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants