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 batch interfaces for reading RtpSents and RtpAcks #42

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tonyherre
Copy link
Contributor

Add batch methods as discussed in #20.

Currently leaving the Events unchanged, as an alternative.

@Philipel-WebRTC
Copy link
Contributor

Looks good, I would even prefer if we only had the batched getters instead of the EventHandlers

Another thing I think would be nice to have is a signal that only gets triggered when some receive buffer goes from empty to not empty.

Also, what should happen if the application doesn't call the getters fast enough? Do we just silently drop packets?

@tonyherre
Copy link
Contributor Author

tonyherre commented May 31, 2024

Looks good, I would even prefer if we only had the batched getters instead of the EventHandlers

I don't feel super strongly, but came to the thought that one either has ~low frequency packets so just event-per-packet is best and easiest, or one has ~high frequency packets (paced to something smooth-ish) so calling readSentNotifications() will always return stuff so one won't need any Events. For either of these cases, needing to use both an Event + getBatch() is extra code & complexity for no benefit.
What scenarios did I missed which would benefit for event+getBatch? Something with lots of bursty packets, like high res, low framerate screenshare maybe? Is that what you were thinking about, Philip?

Also, what should happen if the application doesn't call the getters fast enough? Do we just silently drop packets?

Very good Q... I think we really should do this (and this is an advantage over raw Events which could just clog up the Event Loop ~forever potentially preventing the JS from recovering, as discussed). But do we need to expose that fact to the app, so it knows to speed up if it's not keeping up?
Maybe as a dictionary SentRtpBatchRead { sequence<SentRtp> sentNotifications, short missedSentNotificationCount} dict returned by readSentNotifications() etc?

@aboba
Copy link
Contributor

aboba commented May 31, 2024

@tonyherre In WebTransport, we have incomingMaxAge, outgoingMaxAge to enable dropping of incoming and outgoing packets that spend too long in the queue. Also, there are stats.

@tonyherre
Copy link
Contributor Author

tonyherre commented Jun 5, 2024

Problem: without the clear signal of attaching event handlers, how does the browser know the app will eventually call readSentNotifications() etc so that it should store RtpSent and RtpAcks objects?
Even the RtpTransport object might not be created before JS makes the first read() call, expecting to hear about what's been going on up to that point:

const pcWithCustomBWE = new RTCPeerConnection();
// negotiate, add tracks etc.
setTimeout(() => {
  // What's been sent?
  processSends(pcWithCustomBWE.rtpTransport.readSentNotifications(100));
}, 1000);

We might need a field in RTCConfiguration to start this tracking...

@Philipel-WebRTC
Copy link
Contributor

This is another reason why I think having only batched getters is better. It makes the API clearer.

@youennf
Copy link

youennf commented Jun 11, 2024

I tend to agree we should have only one API here.

Normally, setting event listeners should have no real side effects.
If we think there is a potential big power effects here, it might be better to have a clear signal from the web app that they are interested in this data, like calling getReader for instance.

explainer-use-case-2.md Outdated Show resolved Hide resolved
@pthatcher
Copy link
Collaborator

I think we should keep the new example from #42 and the updated example from #51 (with the event payloads removed). I'm fine with either naming scheme.

@pthatcher
Copy link
Collaborator

We can deal with the "is the max number optional?" either in this PR or in a separate one

@tonyherre
Copy link
Contributor Author

@tonyherre In WebTransport, we have incomingMaxAge, outgoingMaxAge to enable dropping of incoming and outgoing packets that spend too long in the queue. Also, there are stats.

These are great ideas, thanks! - forked out into #54 & #55.

@tonyherre tonyherre force-pushed the batched_uc2_methods branch 2 times, most recently from 48df72f to 405f964 Compare June 20, 2024 13:52
@tonyherre
Copy link
Contributor Author

I think we should keep the new example from #42 and the updated example from #51 (with the event payloads removed). I'm fine with either naming scheme.

Done, I think. Adopted the naming scheme from #51. Also removed the comment and examples which had data in the events themselves.

We can deal with the "is the max number optional?" either in this PR or in a separate one

I wasn't sure which way to go with this - what's the default if the app doesn't provide a max, it just gets the entire queue of many (thousands?) of packets in one chunk? Forcing the JS to specify some number feels like it might be good for avoiding surprises and doesn't add too much complexity, or?
Don't feel that strongly though, so happy to add it and discuss removing it in an issue or meeting etc.

PTAL all!

@tonyherre tonyherre requested a review from pthatcher June 20, 2024 13:57
aarongable pushed a commit to chromium/chromium that referenced this pull request Jun 28, 2024
See w3c/webrtc-rtptransport#42 and https://github.com/w3c/webrtc-rtptransport/blob/main/explainer-use-case-2.md.

Would be used to allow BWE JS implementations to listen to the actual
sent times of RTP packets, for comparison with readReceivedRtpAcks().

All guarded by the blink feature RTCRtpTransport.

Bug: 345101934
Change-Id: Icd63a13a7953a0b77fc589f88e739aa869847fc8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5646499
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Commit-Queue: Tony Herre <toprice@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1320843}
@tonyherre
Copy link
Contributor Author

Ping @pthatcher - Are you happy with how this has taken the updated example from #51?

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

Successfully merging this pull request may close these issues.

None yet

5 participants