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

Added support for RTP abs-capture-time header #761

Merged
merged 1 commit into from
Jan 27, 2022

Conversation

oto313
Copy link
Contributor

@oto313 oto313 commented Jan 26, 2022

Based on other RTP headers, I added support also for http://www.webrtc.org/experiments/rtp-hdrext/abs-capture-time . This is useful for getting to know when exactly was RTP media frame captured.

We already talked about this modification: https://mediasoup.discourse.group/t/forwarding-ntp-timestamp-from-source/3427/3

I made some changes and I test it with wireshark that it passes through this RTP header. You told me that I should add test into TestRtpPacket.cpp file, But I did not make any changes to RtpPacket class. So any additional test will be pointless. Should some other things be modificated?

@oto313 oto313 closed this Jan 26, 2022
@oto313 oto313 reopened this Jan 26, 2022
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.

Thanks.

@ibc
Copy link
Member

ibc commented Jan 26, 2022

Running CI checks before merging.

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.

clang linter complains: https://github.com/versatica/mediasoup/runs/4951301699?check_suite_focus=true

Please run make format in worker/ folder, and ensure everything is ok by running npm run lint and npm test, and then push changes.

@oto313 oto313 force-pushed the feature/abs-capture-time-support branch from 8d55957 to 8a8b5d1 Compare January 26, 2022 14:05
@oto313
Copy link
Contributor Author

oto313 commented Jan 26, 2022

Should be ok now.

@nazar-pc
Copy link
Collaborator

Can you also make similar changes to Rust side, please?

@oto313
Copy link
Contributor Author

oto313 commented Jan 26, 2022

I tried to add support also for rust, but I never programmed in rust. In producer.rs I did not find any equivalent of MangleRtpPacket (like in Producer.cpp). So I dont know if the changes are sufficient. Also I did not tested it

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.

Do you approve @nazar-pc?

@ibc
Copy link
Member

ibc commented Jan 26, 2022

I tried to add support also for rust, but I never programmed in rust. In producer.rs I did not find any equivalent of MangleRtpPacket (like in Producer.cpp). So I dont know if the changes are sufficient. Also I did not tested it

The Rust part is equivalent to the Node part. C++ code is just for the worker.

Copy link
Collaborator

@nazar-pc nazar-pc left a comment

Choose a reason for hiding this comment

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

Looks fine to me

@nazar-pc
Copy link
Collaborator

Looks like tests need to be updated accordingly

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.

Yup, tests failing. This is in both Node tests and Rust tests.

@oto313 oto313 force-pushed the feature/abs-capture-time-support branch from 7cc19f1 to eca21d5 Compare January 26, 2022 20:26
@oto313 oto313 force-pushed the feature/abs-capture-time-support branch from eca21d5 to 7cd9b6a Compare January 26, 2022 21:09
@oto313
Copy link
Contributor Author

oto313 commented Jan 26, 2022

Ok I fixed tests both node and rust (good to know that you must run "npm run typescript:build" before running tests - my test was successful but with old code)

@oto313
Copy link
Contributor Author

oto313 commented Jan 26, 2022

Ok last error in rust: Failed to create Pipe transport: Response { reason: "uv_udp_bind() failed [transport:udp, ip:'127.0.0.1', port:60000]: permission denied. I dont know how to fix that.

@nazar-pc
Copy link
Collaborator

I just restarted it. Port is hardcoded, maybe it happened to be used on that particular run (never happened before, but probable).

@ibc
Copy link
Member

ibc commented Jan 27, 2022

Merging this. Thanks a lot.

@ibc ibc merged commit 77fca4f into versatica:v3 Jan 27, 2022
@ibc
Copy link
Member

ibc commented Jan 27, 2022

Released in mediasoup 3.9.6.

@oto313 oto313 deleted the feature/abs-capture-time-support branch January 27, 2022 12:22
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.

None yet

3 participants