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

Support for abs-send-time (willing to contribute) #1081

Closed
theimowski opened this issue Mar 4, 2024 · 2 comments · Fixed by #1084
Closed

Support for abs-send-time (willing to contribute) #1081

theimowski opened this issue Mar 4, 2024 · 2 comments · Fixed by #1084

Comments

@theimowski
Copy link
Contributor

Hi, as a follow-up on my recent PR that fixed REMB (de)serialisation, I'd like to add support for abs-send-time.

For my use case, I'd like to have bandwidth estimation for video I'm streaming from SIPSorcery to Browser, and rely on REMB to do so - from local experiments it looks like REMB is used even without abs-send-time, but it's much less meaningful.

Here is a quick and dirty change I made locally to send abs-send-time RTP header extension:

  1. Add RTPHeaderExtension to SDPMediaAnnouncement with id 2
  2. Add AbsSendTime method that produces payload for the header extension (copied from Pion)
  3. Specify Header Extension fields for each RTP packet sent in SendRtpRaw

I'd like to clean this up and contribute back to SIPSorcery now, hence following questions:

  1. Should I specify the header extension in SDP at all times, or should it be opt-in (opt-out?), e.g. using a new bool in RTCOfferOptions?
  2. There's a RTPHeaderExtensionData class which currently is used only for parsing byte buffers - should I use it to build a "model" of the data and only then serialise it to bytes?
  3. (related to above two) - how do I go about adding this header extension to RTP packets - should it be added conditionally (when option is specified), and do I need to use the RTPHeaderExtension... constructs to build the rtpPacket.Header ?
  4. unit tests - I haven't seen any relating to RTPHeader serialisation, should I add create such for the above?
@sipsorcery
Copy link
Member

Thanks for the offer and I for one would certainly welcome any improvements around bandwidth estimation. Although might be worth noting that in my experiments the dotnet managed video pipline struggles at 1080p @ 30fps. The bottleneck will typically be CPU rather than bandwidth. But if you do have a specific sceanrio where bw is a problem it will defintiely help to be able to get a good estimate and lower the frame rate.

Should I specify the header extension in SDP at all times, or should it be opt-in (opt-out?), e.g. using a new bool in RTCOfferOptions?

I think so, yes. Receivers can ignoe extensions they don't recognise.

There's a RTPHeaderExtensionData class which currently is used only for parsing byte buffers - should I use it to build a "model" of the data and only then serialise it to bytes?

I'd say use if it helps but don't feel obliged. It's a generic way to get data from unrecognisd extensions. In your case the extension will be recognised.

(related to above two) - how do I go about adding this header extension to RTP packets - should it be added conditionally (when option is specified), and do I need to use the RTPHeaderExtension... constructs to build the rtpPacket.Header ?

Whatever makes most sense. You could add an overload to the RTPHeader class but my immediate insticnt would be to add a new method to RTPHeader along the lines of AddRembExtension.

unit tests - I haven't seen any relating to RTPHeader serialisation, should I add create such for the above?

Ideally yes, even one or two unit tests are very helpful.

@theimowski
Copy link
Contributor Author

Thanks for the answers.

As for the bottlenecks - my use case is desktop sharing, so it's less "dynamic" and therefore I am able to get reasonable results with 3440x1440 at variable frame rate ~20.

The REMB itself is not ideal though (even with the abs-send-time extension) - from my experiments it seems it can often trap itself in a "local minimum" and underestimate the available bandwidth - for example when I throttle frame rate, it then doesn't "probe" for more bandwidth and gets satisfied with lowered bitrate.
Possibly TWCC would yield better results, but requires a bit more effort to implement on the sender side - and I need to make a trade-off due to limited time.

I'll work on the PR and submit it soon.

sipsorcery pushed a commit that referenced this issue Mar 21, 2024
* Add abs-send-time to each RTP packet

* Add a=rtcp-fb with goog-remb to SDP

* Move Header Extensions declaration to SDPMediaAnnouncement constructor

* UnixEpoch not available on lower frameworks

* Move AbsSendTime to RTPHeader class

* Comments for AbsSendTime

* Send abs-send-time only if remote track supports it

* Add unit test

* Revert whitespace changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants