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 357 #900

Closed
wants to merge 4 commits into from
Closed

Fix 357 #900

wants to merge 4 commits into from

Conversation

penguinol
Copy link
Contributor

@penguinol penguinol commented Sep 5, 2022

@ibc
Copy link
Member

ibc commented Sep 5, 2022

Will check tomorrow. Thanks!

@jmillan
Copy link
Member

jmillan commented Sep 6, 2022

Thanks @penguinol!

Do you think you could write some test for the changes in `mediasoup_helpers.h ?

The affected methods (GetBaseDeltaUs and GetBaseTimeUs) are only mediasoup dependant, so they could reside in mediasoup code base, indeed in the same RTCP packet class. Testing them should be fairly easy.

@jmillan
Copy link
Member

jmillan commented Sep 6, 2022

@penguinol it seems that is the FeedbackRtpTransportPacket::GetReferenceTimestamp() the correct place to fix it.

IMHO those changes should be there instead.

EDIT:

GetBaseDeltaUs is not part of mediasoup and should stay in worker/deps/libwebrtc/libwebrtc/mediasoup_helpers.h

@sarumjanuch
Copy link
Contributor

Hello @penguinol, it is happened that we were looking at the same issue, without knowing each other. I wrote tests to ensure that mediasoup is parsing transport-cc RTCP packets the same way as libwebrtc does, currently there is a discrepancy in how mediasoup and libwebrtc looks at reference_time timestamp #899. I just want to ask you permission, to take your PR and combine it with the one that i prepared for tests to be passing. And also I am going to write tests for GetBaseDelta.

@penguinol
Copy link
Contributor Author

@sarumjanuch Great! Combine it to your test!

sarumjanuch pushed a commit to sarumjanuch/mediasoup that referenced this pull request Sep 6, 2022
Adopt changes to GetBaseDelta from versatica#900
Add tests for GetBaseDelta Wraparound.
sarumjanuch pushed a commit to sarumjanuch/mediasoup that referenced this pull request Sep 6, 2022
Adopt changes to GetBaseDelta from versatica#900
Add tests for GetBaseDelta Wraparound.
sarumjanuch pushed a commit to sarumjanuch/mediasoup that referenced this pull request Sep 6, 2022
Adopt changes to GetBaseDelta from versatica#900
Add tests for GetBaseDelta Wraparound.
@penguinol
Copy link
Contributor Author

Mereged with #899. So close this PR.

@penguinol penguinol closed this Sep 7, 2022
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.

Mediasoup worker crash (libwebrtc uint64_t/int64_t conversion related)
4 participants