Fix ICE end-of-candidates messages #2622
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We were casting a POJO to an RTCIceCandidate for the dummy end-of-candidates candidate, but #2473 started calling .toJSON() on these objects. Store separately whether we've seen the end of candidates rather than adding on a dummy candidate object. A test for this will follow, but a) I want to get this fix out and b) I'm currently rewriting the call test file to add typing. Fixes element-hq/element-call#553
| * @param content The candidate to queue up, or null if candidates have finished being generated | ||
| * and end-of-candidates should be signalled | ||
| */ | ||
| private queueCandidate(content: RTCIceCandidate | null): void { |
There was a problem hiding this comment.
I wonder if we could avoid the step with null and set candidatesEnded right at the start?
There was a problem hiding this comment.
Not sure what you mean here - at the start of what?
There was a problem hiding this comment.
I was thinking we would set candidatesEnded = true right after receiving the last candidate and avoid calling queueCandidate() with null though since that might require a bit more work (if even possible) and you're on holiday, we can go with this as is
There was a problem hiding this comment.
oh I see - we need to schedule it to be sent too though, so this seemed the easiest way of re-using that code.
SimonBrandner
approved these changes
Aug 26, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We were casting a POJO to an RTCIceCandidate for the dummy
end-of-candidates candidate, but #2473
started calling .toJSON() on these objects.
Store separately whether we've seen the end of candidates rather than
adding on a dummy candidate object.
A test for this will follow, but a) I want to get this fix out and
b) I'm currently rewriting the call test file to add typing.
Fixes element-hq/element-call#553
Checklist
This change is marked as an internal change (Task), so will not be included in the changelog.