sipsorcery-org / sipsorcery

A WebRTC, SIP and VoIP library for C# and .NET. Designed for real-time communications apps.
https://sipsorcery-org.github.io/sipsorcery
Other
1.42k stars 431 forks source link

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

Closed theimowski closed 5 months ago

theimowski commented 6 months ago

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 commented 6 months ago

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 commented 5 months ago

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.