Expose APIs for handling streamId alloc and re-use#463
Conversation
This commit is a preparation step for making webrtc able to properly determine max number of datachannels that can be used for a specific session. It also adds an event subscription for when a stream has been completely resetted, so that webrtc knows when it is possible to re-use a streamId.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #463 +/- ##
==========================================
+ Coverage 83.92% 84.03% +0.11%
==========================================
Files 54 54
Lines 4366 4384 +18
==========================================
+ Hits 3664 3684 +20
+ Misses 508 507 -1
+ Partials 194 193 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thank you @pando-emil sorry I missed this, I'll review both your PRs today. |
|
@JoTurk did you manage to review this? Just let me know if you have any questions! |
|
@pando-emil sorry i got busy with some stuff at my work, I'll check this PR. this morning. |
JoTurk
left a comment
JoTurk
left a comment
There was a problem hiding this comment.
Thank you and sorry i took time to get back to you.
| // NumInboundStreams returns the maximum number of inbound streams for this association. | ||
| // The number of inbound streams is determined by looking at the peer's INIT or INIT ACK chunk, | ||
| // so it is not available until the handshake completes. | ||
| func (a *Association) NumInboundStreams() uint16 { | ||
| a.lock.RLock() | ||
| defer a.lock.RUnlock() | ||
|
|
||
| return a.myMaxNumInboundStreams | ||
| } | ||
|
|
||
| // NumOutboundStreams returns the maximum number of outbound streams for this association. | ||
| // The number of outbound streams is determined by looking at the peer's INIT or INIT ACK chunk, | ||
| // so it is not available until the handshake completes. | ||
| func (a *Association) NumOutboundStreams() uint16 { | ||
| a.lock.RLock() | ||
| defer a.lock.RUnlock() | ||
|
|
||
| return a.myMaxNumOutboundStreams | ||
| } |
There was a problem hiding this comment.
I think this in-correct for the purpose of this PR because both values are from our direction
There was a problem hiding this comment.
The myMaxNumInboundStreams and myMaxNumOutboundStreams is dynamically changed to the min of our local max and the remotes max in initWithOutOfBandTokens() and handleInitAck(). The added tests in association_test.go should actually verify this.
But maybe we should change name of the variable, "myMax" gets a bit confusing...
There was a problem hiding this comment.
The myMaxNumInboundStreams and myMaxNumOutboundStreams is dynamically changed to the min of our local max and the remotes max in initWithOutOfBandTokens() and handleInitAck().
This isn't the problem, the problem is that myMaxNumInboundStreams and myMaxNumOutboundStreams are set to one direction, they are just internally values to hold the remote limits, they are confusing and named incorrectly and we should change them if we were to use them in a public API, And provide an option API to set our own max.
Lines 779 to 780 in 2e80e82
Lines 1720 to 1721 in 2e80e82
And how we make init chunks (which is incorrect btw, if we use anything other than MaxUint16 in the future):
Lines 1762 to 1763 in 2e80e82
Another issue is that they are inverted in this PR, NumInboundStreams should have been min(inbound, and the peer's outbound), but currently it's the remote peer's numInboundStreams.
I think if we were to expose APIs or options, we should have, localOS, localMIS, peerOS, peerMIS and drive the max values from min(localOS, peerMIS) and min(localMIS, peerOS).
There was a problem hiding this comment.
I wanted to keep the patch to a minimum to not mess with namings etc I don't know the original intentions behind. But I do agree with your comments. I'll try to rework the internal states and update the PR.
But I think the addition of an API to set the local max can be saved for a separate PR, that is a new feature.
|
@pando-emil I'm thinking about this again, because in interleaving i want to expose an API so the end user can tell if interleaving is enabled and negotiated or not, and since we're going to export multiple methods that are only meaningful after handshake I think we should have a metadata struct and a way to register a callback or even blocking function that returns all the assoc metadata. |
Added a few more tests validating the number of streams negotiation.
|
@JoTurk that sounds reasonable! I'll adopt to those changes as soon as they are merged. In the meantime I have updated with a reworked representation within association. |
|
@pando-emil the metadata API is merged and released btw if you want to update your PR to adds stats to it. |
2 participants
This commit is a preparation step for making webrtc able to properly determine max number of datachannels that can be used for a specific session.
It also adds an event subscription for when a stream has been completely resetted, so that webrtc knows when it is possible to re-use a streamId.
Corresponding PR in webrtc repo: pion/webrtc#3405