Make transport-cc feedback work similarly to libwebrtc #1088
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks, will check next week. |
jmillan
reviewed
Jun 20, 2023
jmillan
reviewed
Jun 20, 2023
|
@penguinol, do you think you can write few tests confirming this fix? The scenarios to test are clear. |
ibc
reviewed
Jun 20, 2023
Co-authored-by: Iñaki Baz Castillo <[email protected]>
…soup into fix_tcc_feedback
The input of |
|
I'll come back to this PR this week. |
jmillan
reviewed
Jan 2, 2024
jmillan
reviewed
Jan 2, 2024
Co-authored-by: José Luis Millán <[email protected]>
I'll try to add some tests next week. |
Modify nowMs as a parameter for testing purposes.
…soup into fix_tcc_feedback
fix situation of [1 2 4 5] [3 6 7]
|
@jmillan i've add some tests |
Co-authored-by: Iñaki Baz Castillo <[email protected]>
use std::addressof
ibc
reviewed
Jan 30, 2024
|
In addition to requested/proposed changes, please merge v3 branch into yours and add a |
|
This random failure in a CI test was fixed in v3 branch recently, please merge v3 branch into yours. https://github.com/versatica/mediasoup/actions/runs/7710876267/job/21015131079?pr=1088#step:7:57 |
ibc
reviewed
Jan 31, 2024
Co-authored-by: Iñaki Baz Castillo <[email protected]>
ibc
reviewed
Jan 31, 2024
jmillan
reviewed
Jan 31, 2024
ibc
reviewed
Jan 31, 2024
ibc
reviewed
Jan 31, 2024
Co-authored-by: José Luis Millán <[email protected]> Co-authored-by: Iñaki Baz Castillo <[email protected]>
ibc
reviewed
Jan 31, 2024
Co-authored-by: Iñaki Baz Castillo <[email protected]>
…soup into fix_tcc_feedback
|
@penguinol I'm pushing pending changes to this branch. |
|
Merging. Thanks a lot @penguinol!! |
ibc
changed the title
|
This PR introduces a bug that causes a crash: #1336 |
ibc
added a commit
that referenced
this pull request
Feb 20, 2024
- Fixes #1336 ### Details - Regression introduced in PR #1088, which has many issues. - Basically there are cases in which `this->transportCcFeedbackPacket` is reset so it's a complete new packet and hence its `baseSet` is `false`. - However in `TransportCongestionControlServer::FillAndSendTransportCcFeedback()` in which we just call `this->transportCcFeedbackPacket.SetBase()` once at the top, but then we enter a loop in which the packet maybe full so it's sent (or other things) and hence we **reset** the packet while in the loop. And then at the top of the loop `this->transportCcFeedbackPacket.AddPacket()` is called so the assertion fails because its `SetBase()` was not called on that fresh packet. - Also add a missing `break` in a `case` block in that loop. - Also set proper packet count in all cases in which we reset `this->transportCcFeedbackPacket`. ### TODO This is not done yet. The issue is mainly identified but I don't know yet how to properly fix it without doing other wrong things. Basically I'm not sure if the whole logic is ok after PR #1088: - As said before we only set the base of the packet once at the beginning of the loop, and such a `SetBase()` is called with `this->transportCcFeedbackWideSeqNumStart` and a computed `timestamp`. - However, within the loop below, the packet may become full and hence `SendTransportCcFeedback()` is called, which updates `++this->transportCcFeedbackPacketCount` and `this->transportCcFeedbackWideSeqNumStart`, and **also** resets the packet (so missing base again). - So in order to fix the crash, we must call `SetBase()` again, but which which values? Because the values given initially at the top of `TransportCongestionControlServer::FillAndSendTransportCcFeedback()` were the value of `this->transportCcFeedbackWideSeqNumStart` at that time and a timestamp. But that `this->transportCcFeedbackWideSeqNumStart` has now changed, so then what? - Basically every time `this->transportCcFeedbackPacket.reset()` is called (and there are various cases in the whole `TransportCongestionControlServer.cpp` file) **we must** call ``SetBase()` on the fresh packet **with** proper values. And which are those proper values if we are within that loop?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fix #1059