-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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 RTP extension #2721
Conversation
if this works you should be seeing the available receive bandwidth on the candidate-pair |
uint32_t abs_ts = abs24; | ||
if(janus_rtp_header_extension_set_abs_send_time(pkt->data, pkt->length, | ||
stream->abs_send_time_ext_id, abs_ts) < 0) { | ||
JANUS_LOG(LOG_ERR, "[%"SCNu64"] Error setting abs-send-time value...\n", handle->handle_id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo the documentation on webrtc is misleading here. It says something about the NTP timestamp but this is not what libwebrtc does:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/webrtc/modules/rtp_rtcp/source/rtp_sender_egress.cc;l=226;drc=6a0a55907bf29c7d6bfedb0f8aa4405ed3e2dcf5
It takes the current monotonic clock and then does some shifting with it:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h;l=42;drc=1f5325b50d27ed221d4a26530908c4b68b834507
i'd strongly recommend the same. Mistakes in this are subtle but may lead to overshooting the available bandwidth
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be fixed now 👍
@@ -4784,6 +4800,14 @@ void janus_ice_relay_rtp(janus_ice_handle *handle, janus_plugin_rtp *packet) { | |||
extheader->length = 0; | |||
/* Iterate on all extensions we need */ | |||
char *index = extensions + 4; | |||
/* Check if we need to add the abs-send-time extension */ | |||
if(packet->video && handle->stream->abs_send_time_ext_id > 0) { | |||
*index = (handle->stream->abs_send_time_ext_id << 4) + 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 3 (24 bits), no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the length in the protocol field is always actual data length -1
when you write the extension.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case we're writing the first byte of the extension, so the ID of the extension and the length field:
The 4-bit length is the number minus one of data bytes of this header extension element following the one-byte header.
That seems to be working, thanks for the heads up 👍 |
Since we've been chatting about it here are two slides showing the current state of janus and the one where this is headed: |
I'm not sure this will work actually, as back in the days I remember that the REMB we got from browsers was "polluted" by the REMB we would send to browsers (e.g., to cap publishers below a certain bitrate). The screenshot below, for instance, is the available bandwidth reported by a subscriber who is also publishing media where Janus is capping the bitrate via a REMB of ~256kbps. |
@fippo the last link you shared seems to be broken. Since we are discussing it, could someone please explain the sorcery behind this line: static_cast<uint32_t>(((time_ms << 18) + 500) / 1000) & 0x00FFFFFF; (posting the libwebrtc because it is a oneliner) |
@atoppi fixed the link! I thought this magic comes from the spec/doc: https://webrtc.googlesource.com/src/+/refs/heads/main/docs/native-code/rtp-hdrext/abs-send-time/ (now where it comes from there...) but... @lminiero interesting! But publishers would be on a different peerconnection (unless multistream?) |
Yes, they are on a different PC (publishers and subscribers are separated in multistream too), but they seem to impact the estimation the browser does for subscriptions anyway. Not sure if that's something that happens for PCs of the same tab. |
@lminiero I did some testing for this branch and confirm for subscriber stream getStat()- candidate-pair - avaialbleOutgoingBitrate is available, with the dumped pcap i can confirm Janus is receiving REMB packets from the browser.
|
i've reproduced a similar behaviour as in #2721 (comment).
The difference is bigger if you go for higher layers, e.g. a REMB limit of 768kbps which will typically end up with an estimate of 1000kbps. Still a good test case! |
I think you're on to something there. I made a few more tests using the Streaming plugin and taking advantage of the VideoRoom demo Using the Streaming plugin, and the same "Meetecho" stream that is available on the official demos, the graph I get is this: That stream is about 250kbps in average (it may have some spikes since it's encoded by gstreamer and not a browser) and eventually that's the bandwidth it lands on. So it's not all the bandwidth you have available, but as you pointed out something that more closely reflects the incoming bitrate and goes beyond that a bit since it all seems fine. I got more interesting results when tinkering with the VideoRoom subscriber-mode, though, where I basically did this:
When I started with a publisher that had no REMB limitation, and created the external subscriber, I got a diagram like this: The diagram didn't change when I started sending the publisher a REMB to cap the bandwidth to 256kbps, probably because nothing was wrong in the abs-send-time it was receiving (still all packets sent properly) and so the estimate didn't change. If I started my subscriber when the publisher was already capped, instead, the graph would look like this: As you can see, the initial estimate was very low, and closer to the 256kbps the publisher was sending at. When I then removed the cap from the publisher, the estimate grew to get much closer to the actual bandwidth (~2.5mbps). As before, the estimate did not go down again when I restored the 256kbps cap for the publisher (as you can see, the estimate remains high). I verified the same happens when I created a subscriber in the same tab as the publisher, which seems to confirm that the limited estimate is not, as I thought, caused by some sort of REMB "pollution" from the publisher, but by the starting bitrate a subscriber receives. If the subscriber starts receiving a high bitrate stream right away, then the estimate starts higher too, otherwise it starts low and gives a less reliable value. This probably means that any automated switching mechanism should take this into account. |
That makes sense! I think the subscriber available incoming bitrate estimate will only adjust down if a delay increase is measured or there is packet loss. But this is one of the rather undocumented areas of the webrtc codebase |
I tried simulating network congestion adding 2% loss, 50 ms delay and 10 ms jitter on the interface. available incoming bitrate dropped with congestion and once network limitations have been removed |
My bad, it seems that after a very long time it ramped up to the correct value! |
Merging this, as it's a self contained change that's independent of the features that can be built on top of it. |
As the title says, this PR adds support for negotiating the abs-send-time RTP extension, and generating it for outgoing video packets if enabled. The main motivation for this comes from this thread on discuss-webrtc, which seems to say that if TWCC is enabled, BWE via REMB is not done, and so browsers don't have access to some stats that may be useful, e.g., to VideoRoom subscribers. From some versions, we were negotiating TWCC for subscribers as well, which was useless, since we do nothing with the feedback they provide, and so REMB would be better for those instead: the
abs-send-time
RTP extension is needed for REMB to work properly, so this patch takes care of that, allowing plugins to negotiate it.Instead of just relaying the values we receive (which would be problematic for several reasons) we originate the values for the extension ourselves when sending video packets (which is what the specification seems to say intermediaries should do anyway). We do nothing with incoming
abs-send-time
RTP extensions, and we don't do any BWE ourselves: the only purpose of this patch is allowing browsers to have access to that additional information that seems to be missing. At the moment, this extension is only negotiated for VideoRoom subscribers, and Streaming plugin subscribers: I can't think of any other plugin that may need it, but that should be an easy change anyway.I haven't tested if this gets those client side stats to work, as I don't have time. If you're interested, I'm sure you'll test and provide extensive feedback.