Skip to content

Commit

Permalink
Fix ICE end-of-candidates messages
Browse files Browse the repository at this point in the history
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
  • Loading branch information
dbkr committed Aug 25, 2022
1 parent 9e1b126 commit 71cbf3d
Showing 1 changed file with 30 additions and 21 deletions.
51 changes: 30 additions & 21 deletions src/webrtc/call.ts
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,7 @@ export class MatrixCall extends TypedEventEmitter<CallEvent, CallEventHandlerMap
// possible
private candidateSendQueue: Array<RTCIceCandidate> = [];
private candidateSendTries = 0;
private sentEndOfCandidates = false;
private candidatesEnded = false;
private feeds: Array<CallFeed> = [];
private usermediaSenders: Array<RTCRtpSender> = [];
private screensharingSenders: Array<RTCRtpSender> = [];
Expand Down Expand Up @@ -1597,6 +1597,11 @@ export class MatrixCall extends TypedEventEmitter<CallEvent, CallEventHandlerMap
*/
private gotLocalIceCandidate = (event: RTCPeerConnectionIceEvent): Promise<void> => {
if (event.candidate) {
if (this.candidatesEnded) {
logger.warn("Got candidate after candidates have ended - ignoring!");
return;
}

logger.debug(
"Call " + this.callId + " got local ICE " + event.candidate.sdpMid + " candidate: " +
event.candidate.candidate,
Expand All @@ -1606,29 +1611,18 @@ export class MatrixCall extends TypedEventEmitter<CallEvent, CallEventHandlerMap

// As with the offer, note we need to make a copy of this object, not
// pass the original: that broke in Chrome ~m43.
if (event.candidate.candidate !== '' || !this.sentEndOfCandidates) {
if (event.candidate.candidate !== '') {
this.queueCandidate(event.candidate);

if (event.candidate.candidate === '') this.sentEndOfCandidates = true;
} else {
this.queueCandidate(null);
}
}
};

private onIceGatheringStateChange = (event: Event): void => {
logger.debug(`Call ${this.callId} ice gathering state changed to ${this.peerConn.iceGatheringState}`);
if (this.peerConn.iceGatheringState === 'complete' && !this.sentEndOfCandidates) {
// If we didn't get an empty-string candidate to signal the end of candidates,
// create one ourselves now gathering has finished.
// We cast because the interface lists all the properties as required but we
// only want to send 'candidate'
// XXX: We probably want to send either sdpMid or sdpMLineIndex, as it's not strictly
// correct to have a candidate that lacks both of these. We'd have to figure out what
// previous candidates had been sent with and copy them.
const c = {
candidate: '',
} as RTCIceCandidate;
this.queueCandidate(c);
this.sentEndOfCandidates = true;
if (this.peerConn.iceGatheringState === 'complete') {
this.queueCandidate(null);
}
};

Expand Down Expand Up @@ -2247,7 +2241,12 @@ export class MatrixCall extends TypedEventEmitter<CallEvent, CallEventHandlerMap
}
}

private queueCandidate(content: RTCIceCandidate): void {
/**
* Queue a candidate to be sent
* @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 {
// We partially de-trickle candidates by waiting for `delay` before sending them
// amalgamated, in order to avoid sending too many m.call.candidates events and hitting
// rate limits in Matrix.
Expand All @@ -2257,19 +2256,23 @@ export class MatrixCall extends TypedEventEmitter<CallEvent, CallEventHandlerMap
// currently proposes as the way to indicate that candidate gathering is complete.
// This will hopefully be changed to an explicit rather than implicit notification
// shortly.
this.candidateSendQueue.push(content);
if (content) {
this.candidateSendQueue.push(content);
} else {
this.candidatesEnded = true;
}

// Don't send the ICE candidates yet if the call is in the ringing state: this
// means we tried to pick (ie. started generating candidates) and then failed to
// send the answer and went back to the ringing state. Queue up the candidates
// to send if we successfully send the answer.
// Equally don't send if we haven't yet sent the answer because we can send the
// first batch of candidates along with the answer
if (this.state === CallState.Ringing || !this.inviteOrAnswerSent) return;
//if (this.state === CallState.Ringing || !this.inviteOrAnswerSent) return;

// MSC2746 recommends these values (can be quite long when calling because the
// callee will need a while to answer the call)
const delay = this.direction === CallDirection.Inbound ? 500 : 2000;
const delay = 0;//this.direction === CallDirection.Inbound ? 500 : 2000;

if (this.candidateSendTries === 0) {
setTimeout(() => {
Expand Down Expand Up @@ -2446,6 +2449,12 @@ export class MatrixCall extends TypedEventEmitter<CallEvent, CallEventHandlerMap
this.candidateSendQueue = [];
++this.candidateSendTries;
const content = { candidates: candidates.map(candidate => candidate.toJSON()) };
if (this.candidatesEnded) {
// If there are no more candidates, signal this by adding an empty string candidate
content.candidates.push({
candidate: '',
});
}
logger.debug(`Call ${this.callId} attempting to send ${candidates.length} candidates`);
try {
await this.sendVoipEvent(EventType.CallCandidates, content);
Expand Down

0 comments on commit 71cbf3d

Please sign in to comment.