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.

Sign up for GitHub

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

Jump to bottom

transport-cc parsing to be compliant with libwebrtc, make transport-cc reference time to be unsigned, therefore fix 357. #899

Merged
merged 14 commits into from
Sep 19, 2022
Merged

transport-cc parsing to be compliant with libwebrtc, make transport-cc reference time to be unsigned, therefore fix 357. #899

merged 14 commits into from
Sep 19, 2022

Conversation

sarumjanuch
Copy link
Contributor

@sarumjanuch sarumjanuch commented Aug 30, 2022 •
edited
Loading

Changes and tests to make sure mediasoup is parsing transport-cc RTCP feedback packages the same way as libwebrtc does.
Metadata in commit was created by parsing buffers in libwebrtc project.
Also this PR includes changes #900 made by @penguinol.
This PR makes sure that there could be no negative reference time (event though https://datatracker.ietf.org/doc/html/draft-holmer-rmcat-transport-wide-cc-extensions-01#section-3.1 mandates that reference time is 24 bit signed integer, libwebrtc is ignoring that, and with with PR we too), and also handles wraparound, to be consistent with latest libwebrtc code. Therefore it rolls back mediasoup changes in libwebrtc related to #357, and #357 should never happen again.

ibc
ibc requested changes Aug 30, 2022
View reviewed changes
worker/test/src/RTC/RTCP/TestFeedbackRtpTransport.cpp Outdated
}
};

for (const auto &packetMeta: feedbackPacketsMeta) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please run make lint in worker/ directory. It should complain. make format will fix it.

@ibc
Copy link
Member

ibc commented Aug 31, 2022

There are still lint errors: https://github.com/versatica/mediasoup/runs/8097882496?check_suite_focus=true

make format should fix them.

@ibc
Copy link
Member

ibc commented Aug 31, 2022

Note that you can use // clang-format off and on to delimit C++ blocks for which lint should not be checked. Check RequestChannel.cpp file if you find it useful within this PR.

Eugene Voityuk added 2 commits September 6, 2022 14:50
Fix Formatting.
112e1d8
Merge remote-tracking branch 'origin/bwe-tests' into bwe-tests
040c92a 
# Conflicts:
#	worker/test/src/RTC/RTCP/TestFeedbackRtpTransport.cpp
@sarumjanuch sarumjanuch mentioned this pull request Sep 6, 2022
Closed
Eugene Voityuk added 3 commits September 6, 2022 19:11
Get rid of negative reference_time, make reference timestamp to be re…
3379c43 
…presented as in libwebrtc.
Fix tests.
2b53714 
Adopt changes to GetBaseDelta from versatica#900
Add tests for GetBaseDelta Wraparound.
Fix tests.
b4b0ac8 
Adopt changes to GetBaseDelta from versatica#900
Add tests for GetBaseDelta Wraparound.
@sarumjanuch sarumjanuch changed the title transport-cc parsing tests transport-cc parsing to be compliant with libwebrtc Sep 6, 2022
Eugene Voityuk added 2 commits September 6, 2022 21:24
Revert "Fix tests."
3977253 
This reverts commit b4b0ac8.
Fix tests.
31229ce 
Adopt changes to GetBaseDelta from versatica#900
Add tests for GetBaseDelta Wraparound.
ibc
ibc approved these changes Sep 8, 2022
View reviewed changes
Copy link
Member

@ibc ibc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. Just a cosmetic change requested. Also, if this PR fixes an existing issue in GH (I think there is a reported crash, am I right?) reference it in the PR description so it will be automatically closed. Tell me when those things are done and let's merge then.

worker/include/RTC/RTCP/FeedbackRtpTransport.hpp Show resolved Hide resolved
@sarumjanuch sarumjanuch changed the title transport-cc parsing to be compliant with libwebrtc transport-cc parsing to be compliant with libwebrtc, make reference_time to be unsigned. Sep 19, 2022
@sarumjanuch
Copy link
Contributor Author

sarumjanuch commented Sep 19, 2022 •
edited by ibc
Loading

Nice. Just a cosmetic change requested. Also, if this PR fixes an existing issue in GH (I think there is a reported crash, am I right?) reference it in the PR description so it will be automatically closed. Tell me when those things are done and let's merge then.
@ibc Seems like done, please run tests, because I am not sure that clang formatter is not playing games with me again.

@sarumjanuch sarumjanuch changed the title transport-cc parsing to be compliant with libwebrtc, make reference_time to be unsigned. transport-cc parsing to be compliant with libwebrtc, make transport-cc reference time to be unsigned. Sep 19, 2022
@ibc
Copy link
Member

ibc commented Sep 19, 2022

Running!

@ibc
Copy link
Member

ibc commented Sep 19, 2022

Please re-request review once tests pass.

@sarumjanuch sarumjanuch requested review from jmillan and ibc and removed request for ibc and jmillan September 19, 2022 11:38
@ibc
Copy link
Member

ibc commented Sep 19, 2022

Nice. Just a cosmetic change requested. Also, if this PR fixes an existing issue in GH (I think there is a reported crash, am I right?) reference it in the PR description so it will be automatically closed. Tell me when those things are done and let's merge then.

Will merge. Can you check this please? I think there is an open issue or PR (here in GH) that aims to fix an issue that may be fixed by this PR, but maybe I'm wrong.

@ibc
Copy link
Member

ibc commented Sep 19, 2022

Oh, I request a change: please add a item in Changelog.md following the style of other items.

@sarumjanuch sarumjanuch changed the title transport-cc parsing to be compliant with libwebrtc, make transport-cc reference time to be unsigned. transport-cc parsing to be compliant with libwebrtc, make transport-cc reference time to be unsigned, therefore fix #357. Sep 19, 2022
@sarumjanuch sarumjanuch changed the title transport-cc parsing to be compliant with libwebrtc, make transport-cc reference time to be unsigned, therefore fix #357. transport-cc parsing to be compliant with libwebrtc, make transport-cc reference time to be unsigned, therefore fix https://github.com/versatica/mediasoup/issues/357. Sep 19, 2022
@sarumjanuch sarumjanuch changed the title transport-cc parsing to be compliant with libwebrtc, make transport-cc reference time to be unsigned, therefore fix https://github.com/versatica/mediasoup/issues/357. transport-cc parsing to be compliant with libwebrtc, make transport-cc reference time to be unsigned, therefore fix 357. Sep 19, 2022
@sarumjanuch
Copy link
Contributor Author

Will merge. Can you check this please? I think there is an open issue or PR (here in GH) that aims to fix an issue that may be fixed by this PR, but maybe I'm wrong.

Yes, i did that, also added in title.

@sarumjanuch
Copy link
Contributor Author

Oh, I request a change: please add a item in Changelog.md following the style of other items.

doing right now

@ibc ibc merged commit 6c828a7 into versatica:v3 Sep 19, 2022
@jmillan jmillan mentioned this pull request Sep 29, 2022
Closed
piranna pushed a commit to dyte-in/mediasoup that referenced this pull request Feb 9, 2023
@sarumjanuch @piranna
transport-cc parsing to be compliant with libwebrtc, make transport-c…
fb48b59 
…c reference time to be unsigned, therefore fix 357. (versatica#899)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmillan jmillan jmillan approved these changes

@ibc ibc Awaiting requested review from ibc

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sarumjanuch @ibc @jmillan