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

Add support for abs-capture-time RTP extension #3161

Merged
merged 6 commits into from
Nov 16, 2023
Merged

Conversation

lminiero
Copy link
Member

@lminiero lminiero commented Feb 8, 2023

The abs-capture-time RTP extension was added recently to Chrome (~M107). While I'm still not exactly clear on how/when it could help, apparently the purpose of this experimental extension is to evaluate and increase audio/video synchronization, since the sender basically puts the NTP value of when the packet they're sending was actually captured, independently on when it gets anywhere.

At the moment, this patch simply copies the value across if an incoming packet with such an extension is relayed to one ore more destination, meaning the Janus core doesn't do any adaptation of the value. The current version of the document, speaking of intermediate systems, says:

An intermediate system (e.g. mixer) MAY adjust these timestamps as needed. It MAY also choose to rewrite the timestamps completely, using its own NTP clock as reference clock, if it wants to present itself as a capture system for A/V-sync purposes.

which means we may have to rewrite it sometime in the future, especially considering the value is based on the sender's NTP clock, which may very well not be the same as the instance Janus is running on. As such, I'm not exactly sure if our current implementation has any real value or not, but I'm publishing it anyway so that interested parties can experiment with it.

It's also worth pointing out that, although we can parse, and send, 16-bit extensions including both clock value and offset, we currently only parse and send the clock value, ignoring the offset entirely: this is an arbitrary decision I made to simplify this first patch by looking at an existing capture coming from Chrome, where the offset was always 0, and so that's what I'm setting it to as well. Not sure if the offset part is not implemented at all in browsers, or if I was in a situation where 0 was indeed the right value: this is a draft PR anyway, so in case you know it's a value that actually makes sense to involve in the processing, it should be relatively simple to extend the support to the offset to.

This patch only adds support for this extension to the EchoTest plugin, which will automatically negotiate the extension if you send an SDP that offers it: notice that if you want to test it, you'll probably have to munge the SDP to add the extension, since at least in my versions of Chrome it's not offered by default. EchoTest support should be enough to start testing incoming and outgoing packets containing this extension: depending on how well it goes, I'll add it to other plugins too, e.g., the VideoRoom.

Feedback welcome!

@lminiero lminiero added the multistream Related to Janus 1.x label Feb 8, 2023
@lminiero
Copy link
Member Author

lminiero commented May 8, 2023

It doesn't look like this is of interest to anyone, since we got no feedback.

@lionelnicolas @danjenkins just pinging you as you added some reactions to the original post: is this something you tested and/or are interested in? Otherwise I'll just close it, as we personally don't have any use for it at the moment.

@danjenkins
Copy link
Contributor

Interested in yes. Forgot to test due to busy-ness and the fact I'm still running on 0.x branch... will make an effort to upgrade and test over the next two weeks!

@lionelnicolas
Copy link
Contributor

@lminiero We recently had to deal (especially @tmatth) with A/V sync issues when pulling a stream from janus to (pion|gortsplib)-based clients, especially when the webrtc publisher was degraded.

We are also still running 0.x and we haven't tested this PR, but this looks very interesting to implement A/V sync detection/correction.

@mvollrath
Copy link

Interested in this as basis for abs-capture-time support in streaming plugin.

@mvollrath
Copy link

I think you need to add abs-capture-time to janus_rtp_extension_id or it never gets offered.

@mvollrath
Copy link

Oh I see, probably not needed for echotest.

@mvollrath
Copy link

mvollrath commented Aug 11, 2023

FWIW the generic parts of this are working for me. The only place I've found to probe is, in Chrome, an undocumented captureTimestamp property of RTCRtpContributingSource (or RTCRtpSynchronizationSource). It has the timestamp as milliseconds from the NTP 1900 epoch.

@IbrayevRamil
Copy link
Contributor

IbrayevRamil commented Aug 14, 2023

Added PR related to this one.
@mvollrath it can be interesting for you as it adds support to streaming plugin and forwards abs-capture-time from RTP source
#3258

@lminiero lminiero marked this pull request as ready for review September 22, 2023 08:04
@lminiero
Copy link
Member Author

Merging.

@lminiero lminiero merged commit 271d319 into master Nov 16, 2023
8 checks passed
@lminiero lminiero deleted the capture-time branch November 16, 2023 15:31
mwalbeck pushed a commit to mwalbeck/docker-janus-gateway that referenced this pull request Dec 8, 2023
This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [meetecho/janus-gateway](https://github.com/meetecho/janus-gateway) | minor | `v1.1.4` -> `v1.2.1` |

---

### Release Notes

<details>
<summary>meetecho/janus-gateway (meetecho/janus-gateway)</summary>

### [`v1.2.1`](https://github.com/meetecho/janus-gateway/blob/HEAD/CHANGELOG.md#v121---2023-12-06)

[Compare Source](meetecho/janus-gateway@v1.2.0...v1.2.1)

-   Added support for abs-capture-time RTP extension \[[PR-3161](meetecho/janus-gateway#3161)]
-   Fixed truncated label in datachannels (thanks [@&#8203;veeting](https://github.com/veeting)!) \[[PR-3265](meetecho/janus-gateway#3265)]
-   Support larger values for SDP attributes (thanks [@&#8203;petarminchev](https://github.com/petarminchev)!) \[[PR-3282](meetecho/janus-gateway#3282)]
-   Fixed rare crash in VideoRoom plugin (thanks [@&#8203;tmatth](https://github.com/tmatth)!) \[[PR-3259](meetecho/janus-gateway#3259)]
-   Don't create VideoRoom subscriptions to publisher streams with no associated codecs
-   Added suspend/resume participant API to AudioBridge \[[PR-3301](meetecho/janus-gateway#3301)]
-   Fixed rare crash in AudioBridge
-   Fixed rare crash in Streaming plugin when doing ICE restarts \[[Issue-3288](meetecho/janus-gateway#3288)]
-   Allow SIP and NoSIP plugins to bind media to a specific address (thanks [@&#8203;pawnnail](https://github.com/pawnnail)!) \[[PR-3263](meetecho/janus-gateway#3263)]
-   Removed advertised support for SIP UPDATE in SIP plugin
-   Parse RFC2833 DTMF to custom events in SIP plugin (thanks [@&#8203;ywmoyue](https://github.com/ywmoyue)!) \[[PR-3280](meetecho/janus-gateway#3280)]
-   Fixed missing Contact header in some dialogs in SIP plugin (thanks [@&#8203;ycherniavskyi](https://github.com/ycherniavskyi)!) \[[PR-3286](meetecho/janus-gateway#3286)]
-   Properly set mid when notifying about ended tracks in janus.js
-   Fixed broken restamping in janus-pp-rec
-   Other smaller fixes and improvements (thanks to all who contributed pull requests and reported issues!)

### [`v1.2.0`](https://github.com/meetecho/janus-gateway/blob/HEAD/CHANGELOG.md#v120---2023-08-09)

[Compare Source](meetecho/janus-gateway@v1.1.4...v1.2.0)

-   Added support for VP9/AV1 simulcast, and fixed broken AV1/SVC support \[[PR-3218](meetecho/janus-gateway#3218)]
-   Fixed RTCP out quality stats \[[PR-3228](meetecho/janus-gateway#3228)]
-   Default link quality stats to 100
-   Added support for ICE consent freshness \[[PR-3234](meetecho/janus-gateway#3234)]
-   Fixed rare race condition in VideoRoom \[[PR-3219](meetecho/janus-gateway#3219)] \[[PR-3247](meetecho/janus-gateway#3247)]
-   Use speexdsp's jitter buffer in the AudioBridge \[[PR-3233](meetecho/janus-gateway#3233)]
-   Fixed crash in Streaming plugin on mountpoints with too many streams \[[Issue-3225](meetecho/janus-gateway#3225)]
-   Support for batched configure requests in Streaming plugin (thanks [@&#8203;petarminchev](https://github.com/petarminchev)!) \[[PR-3239](meetecho/janus-gateway#3239)]
-   Fixed rare deadlock in Streamin plugin \[[PR-3250](meetecho/janus-gateway#3250)]
-   Fix simulated leave message for longer string ID rooms in TextRoom (thanks [@&#8203;adnanel](https://github.com/adnanel)!) [PR-3243](meetecho/janus-gateway#3243)]
-   Notify about count of sessions, handles and PeerConnections on a regular basis, when event handlers are enabled \[[PR-3221](meetecho/janus-gateway#3221)]
-   Fixed broken Insertable Streams for recvonly streams when answering in janus.js
-   Added background selector and blur support to Virtual Background demo
-   Other smaller fixes and improvements (thanks to all who contributed pull requests and reported issues!)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi4zNC4wIiwidXBkYXRlZEluVmVyIjoiMzcuODEuNCIsInRhcmdldEJyYW5jaCI6Im1hc3RlciJ9-->

Reviewed-on: https://git.walbeck.it/walbeck-it/docker-janus-gateway/pulls/122
Co-authored-by: renovate-bot <[email protected]>
Co-committed-by: renovate-bot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
multistream Related to Janus 1.x please-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants