feat: sync call's "on hold" and "mute" states (#37386)

Co-authored-by: gabriellsh <[email protected]>

Author: pierre-lehnen-rc

.changeset/six-adults-lie.md

@@ -0,0 +1,6 @@
+---
+'@rocket.chat/media-signaling': minor
+'@rocket.chat/media-calls': minor
+---
+
+Fixes an error where Voice Calls could fail to negotiate webrtc params if both clients requested a renegotiation at the same time

ee/packages/media-calls/src/internal/SignalProcessor.ts

@@ -1,20 +1,20 @@
 import type { IMediaCall, IUser } from '@rocket.chat/core-typings';
 import { Emitter } from '@rocket.chat/emitter';
-import {
-	isPendingState,
-	type ClientMediaSignal,
-	type ClientMediaSignalRegister,
-	type ClientMediaSignalRequestCall,
-	type ServerMediaSignal,
-	type ServerMediaSignalRejectedCallRequest,
+import { isPendingState } from '@rocket.chat/media-signaling';
+import type {
+	ClientMediaSignal,
+	ClientMediaSignalRegister,
+	ClientMediaSignalRequestCall,
+	ServerMediaSignal,
+	ServerMediaSignalRejectedCallRequest,
 } from '@rocket.chat/media-signaling';
 import { MediaCalls } from '@rocket.chat/models';
 
 import type { InternalCallParams } from '../definition/common';
 import { logger } from '../logger';
 import { mediaCallDirector } from '../server/CallDirector';
 import { UserActorAgent } from './agents/UserActorAgent';
-import { getNewCallTransferredBy } from '../server/getNewCallTransferredBy';
+import { buildNewCallSignal } from '../server/buildNewCallSignal';
 import { stripSensitiveDataFromSignal } from '../server/stripSensitiveData';
 
 export type SignalProcessorEvents = {
@@ -147,44 +147,7 @@ export class GlobalSignalProcessor {
 			await mediaCallDirector.renewCallId(call._id);
 		}
 
-		const transferredBy = getNewCallTransferredBy(call);
-
-		if (isCaller) {
-			this.sendSignal(uid, {
-				callId: call._id,
-				type: 'new',
-				service: call.service,
-				kind: call.kind,
-				role: 'caller',
-				self: {
-					...call.caller,
-				},
-				contact: {
-					...call.callee,
-				},
-				...(call.callerRequestedId && { requestedCallId: call.callerRequestedId }),
-				...(call.parentCallId && { replacingCallId: call.parentCallId }),
-				...(transferredBy && { transferredBy }),
-			});
-		}
-
-		if (isCallee) {
-			this.sendSignal(uid, {
-				callId: call._id,
-				type: 'new',
-				service: call.service,
-				kind: call.kind,
-				role: 'callee',
-				self: {
-					...call.callee,
-				},
-				contact: {
-					...call.caller,
-				},
-				...(call.parentCallId && { replacingCallId: call.parentCallId }),
-				...(transferredBy && { transferredBy }),
-			});
-		}
+		this.sendSignal(uid, buildNewCallSignal(call, role));
 
 		if (call.state === 'active') {
 			this.sendSignal(uid, {
@@ -278,24 +241,7 @@ export class GlobalSignalProcessor {
 			this.rejectCallRequest(uid, { ...rejection, reason: 'already-requested' });
 		}
 
-		const transferredBy = getNewCallTransferredBy(call);
-
-		this.sendSignal(uid, {
-			callId: call._id,
-			type: 'new',
-			service: call.service,
-			kind: call.kind,
-			role: 'caller',
-			self: {
-				...call.caller,
-			},
-			contact: {
-				...call.callee,
-			},
-			requestedCallId: signal.callId,
-			...(call.parentCallId && { replacingCallId: call.parentCallId }),
-			...(transferredBy && { transferredBy }),
-		});
+		this.sendSignal(uid, buildNewCallSignal(call, 'caller'));
 
 		return call;
 	}

ee/packages/media-calls/src/internal/agents/CallSignalProcessor.ts

@@ -177,13 +177,69 @@ export class UserActorSignalProcessor {
 	}
 
 	private async processNegotiationNeeded(oldNegotiationId: string): Promise<void> {
+		// Unsigned clients may not request negotiations
+		if (!this.signed) {
+			return;
+		}
+
 		logger.debug({ msg: 'UserActorSignalProcessor.processNegotiationNeeded', oldNegotiationId });
 		const negotiation = await MediaCallNegotiations.findLatestByCallId(this.callId);
-		// If the negotiation that triggered a request for renegotiation is not the latest negotiation, then a new one must already be happening and we can ignore this request.
-		if (negotiation?._id !== oldNegotiationId) {
+
+		// If the call doesn't even have an initial negotiation yet, the clients shouldn't be requesting new ones.
+		if (!negotiation) {
 			return;
 		}
 
+		// If the latest negotiation has an answer, we can accept any request
+		if (negotiation.answer) {
+			return this.startNewNegotiation();
+		}
+
+		const comingFromLatest = oldNegotiationId === negotiation._id;
+		const isRequestImpolite = this.role === 'caller';
+		const isLatestImpolite = negotiation.offerer === 'caller';
+
+		// If the request came from a client who was not yet aware of a newer renegotiation
+		if (!comingFromLatest) {
+			// If the client is polite, we can ignore their request in favor of the existing renegotiation
+			if (!isRequestImpolite) {
+				logger.debug({ msg: 'Ignoring outdated polite renegotiation request' });
+				return;
+			}
+
+			// If the latest negotiation is impolite and the impolite client is not aware of it yet, this must be a duplicate request
+			if (isLatestImpolite) {
+				// If we already received an offer in this situation then something is very wrong (some proxy interfering with signals, perhaps?)
+				if (negotiation.offer) {
+					logger.error({ msg: 'Invalid renegotiation request', requestedBy: this.role, isLatestImpolite });
+					return;
+				}
+
+				// Resend the offer request to the impolite client
+				return this.requestWebRTCOffer({ negotiationId: negotiation._id });
+			}
+
+			// The state of polite negotiations is irrelevant for impolite requests, so we can start a new negotiation here.
+			return this.startNewNegotiation();
+		}
+
+		// The client is up-to-date and requested a renegotiation before the last one was complete
+		// If the request came from the same side as the last negotiation, the client was in no position to request it
+		if (this.role === negotiation.offerer) {
+			logger.error({ msg: 'Invalid state for renegotiation request', requestedBy: this.role, isLatestImpolite });
+			return;
+		}
+
+		// If the request is from the impolite client, it takes priority over the existing polite negotiation
+		if (isRequestImpolite) {
+			return this.startNewNegotiation();
+		}
+
+		// It's a polite negotiation requested while an impolite one was not yet complete
+		logger.error({ msg: 'Invalid state for renegotiation request', requestedBy: this.role, isLatestImpolite });
+	}
+
+	private async startNewNegotiation(): Promise<void> {
 		const negotiationId = await mediaCallDirector.startNewNegotiation(this.call, this.role);
 		if (negotiationId) {
 			await this.requestWebRTCOffer({ negotiationId });

ee/packages/media-calls/src/internal/agents/UserActorAgent.ts

@@ -1,11 +1,11 @@
 import type { IMediaCall, MediaCallSignedContact } from '@rocket.chat/core-typings';
-import { isBusyState, type ClientMediaSignal, type ServerMediaSignal, type ServerMediaSignalNewCall } from '@rocket.chat/media-signaling';
+import { isBusyState, type ClientMediaSignal, type ServerMediaSignal } from '@rocket.chat/media-signaling';
 import { MediaCallNegotiations, MediaCalls } from '@rocket.chat/models';
 
 import { UserActorSignalProcessor } from './CallSignalProcessor';
 import { BaseMediaCallAgent } from '../../base/BaseAgent';
 import { logger } from '../../logger';
-import { getNewCallTransferredBy } from '../../server/getNewCallTransferredBy';
+import { buildNewCallSignal } from '../../server/buildNewCallSignal';
 import { getMediaCallServer } from '../../server/injection';
 
 export class UserActorAgent extends BaseMediaCallAgent {
@@ -77,7 +77,7 @@ export class UserActorAgent extends BaseMediaCallAgent {
 			await this.getOrCreateChannel(call, call.caller.contractId);
 		}
 
-		await this.sendSignal(this.buildNewCallSignal(call));
+		await this.sendSignal(buildNewCallSignal(call, this.role));
 	}
 
 	public async onRemoteDescriptionChanged(callId: string, negotiationId: string): Promise<void> {
@@ -166,21 +166,4 @@ export class UserActorAgent extends BaseMediaCallAgent {
 		logger.debug({ msg: 'UserActorAgent.onDTMF', callId, dtmf, duration });
 		// internal calls have nothing to do with DTMFs
 	}
-
-	protected buildNewCallSignal(call: IMediaCall): ServerMediaSignalNewCall {
-		const transferredBy = getNewCallTransferredBy(call);
-
-		return {
-			callId: call._id,
-			type: 'new',
-			service: call.service,
-			kind: call.kind,
-			role: this.role,
-			self: this.getMyCallActor(call),
-			contact: this.getOtherCallActor(call),
-			...(call.parentCallId && { replacingCallId: call.parentCallId }),
-			...(transferredBy && { transferredBy }),
-			...(call.callerRequestedId && this.role === 'caller' && { requestedCallId: call.callerRequestedId }),
-		};
-	}
 }

ee/packages/media-calls/src/server/buildNewCallSignal.ts

@@ -0,0 +1,42 @@
+import type { IMediaCall } from '@rocket.chat/core-typings';
+import type { CallFlag, CallRole, ServerMediaSignalNewCall } from '@rocket.chat/media-signaling';
+
+import { getNewCallTransferredBy } from './getNewCallTransferredBy';
+
+function getCallFlags(call: IMediaCall, role: CallRole): CallFlag[] {
+	const flags: CallFlag[] = [];
+
+	const isInternal = call.caller.type === 'user' && call.callee.type === 'user';
+	const shouldCreateDataChannel = isInternal && role === 'caller';
+
+	if (isInternal) {
+		flags.push('internal');
+
+		if (shouldCreateDataChannel) {
+			flags.push('create-data-channel');
+		}
+	}
+
+	return flags;
+}
+
+export function buildNewCallSignal(call: IMediaCall, role: CallRole): ServerMediaSignalNewCall {
+	const self = role === 'caller' ? call.caller : call.callee;
+	const contact = role === 'caller' ? call.callee : call.caller;
+	const transferredBy = getNewCallTransferredBy(call);
+	const flags = getCallFlags(call, role);
+
+	return {
+		callId: call._id,
+		type: 'new',
+		service: call.service,
+		kind: call.kind,
+		role,
+		self: { ...self },
+		contact: { ...contact },
+		flags,
+		...(call.parentCallId && { replacingCallId: call.parentCallId }),
+		...(transferredBy && { transferredBy }),
+		...(call.callerRequestedId && role === 'caller' && { requestedCallId: call.callerRequestedId }),
+	};
+}

packages/media-signaling/src/definition/call/IClientMediaCall.ts

@@ -64,10 +64,13 @@ export type CallRejectedReason =
 	| 'invalid-call-params' // something is wrong with the params (eg. no valid route between caller and callee)
 	| 'forbidden'; // one of the actors on the call doesn't have permission for it
 
+export type CallFlag = 'internal' | 'create-data-channel';
+
 export interface IClientMediaCall {
 	callId: string;
 	role: CallRole;
 	service: CallService | null;
+	flags: readonly CallFlag[];
 
 	state: CallState;
 	ignored: boolean;
@@ -78,6 +81,9 @@ export interface IClientMediaCall {
 	held: boolean;
 	/* busy = state >= 'accepted' && state < 'hangup' */
 	busy: boolean;
+	/* if the other side has put the call on hold */
+	remoteHeld: boolean;
+	remoteMute: boolean;
 
 	contact: CallContact;
 	transferredBy: CallContact | null;

packages/media-signaling/src/definition/client.ts

@@ -4,12 +4,8 @@ export type ClientState =
 	| 'accepting' // The client tried to accept the call and is wating for confirmation from the server
 	| 'accepted' // The call was accepted, but the client doesn't have a webrtc offer yet
 	| 'busy-elsewhere' // The call is happening in a different session/client
-	| 'has-offer' // The call was accepted and the client has a webrtc offer
-	| 'has-answer' // The call was accepted and the client has a webrtc offer and answer
 	| 'active' // The webrtc call was established
 	| 'renegotiating' // the webrtc call was established but the client is starting a new negotiation
-	| 'has-new-offer' // The webrtc call was established but the client has an offer for a new negotiation
-	| 'has-new-answer' // The webrtc call was established but the client is finishing a renegotiation
 	| 'hangup'; // The call is over, or happening in some other client
 
 export type ClientContractState =

packages/media-signaling/src/definition/services/IServiceProcessor.ts

@@ -14,11 +14,13 @@ export type ServiceStateValue<ServiceStateMap extends DefaultServiceStateMap, K
 export type ServiceProcessorEvents<ServiceStateMap extends DefaultServiceStateMap> = {
 	internalStateChange: keyof ServiceStateMap;
 	internalError: { critical: boolean; error: string | Error; errorDetails?: string };
-	negotiationNeeded: void;
 };
 
-export interface IServiceProcessor<ServiceStateMap extends DefaultServiceStateMap = DefaultServiceStateMap> {
-	emitter: Emitter<ServiceProcessorEvents<ServiceStateMap>>;
+export interface IServiceProcessor<
+	ServiceStateMap extends DefaultServiceStateMap = DefaultServiceStateMap,
+	ServiceUniqueEvents = Record<never, never>,
+> {
+	emitter: Emitter<ServiceProcessorEvents<ServiceStateMap> & ServiceUniqueEvents>;
 
 	getInternalState<K extends keyof ServiceStateMap>(stateName: K): ServiceStateValue<ServiceStateMap, K>;
 }

packages/media-signaling/src/definition/services/index.ts

@@ -1,3 +1,4 @@
 export * from './webrtc/IWebRTCProcessor';
 export * from './IServiceProcessorFactoryList';
 export * from './MediaStreamFactory';
+export * from './negotiation';

packages/media-signaling/src/definition/services/negotiation.ts

@@ -0,0 +1,30 @@
+import type { IClientMediaCall } from '../call';
+import type { IMediaSignalLogger } from '../logger';
+
+export type NegotiationManagerEvents = {
+	'error': { errorCode: string; negotiationId: string };
+	'local-sdp': { negotiationId: string; sdp: RTCSessionDescriptionInit };
+	'negotiation-needed': { oldNegotiationId: string };
+};
+
+export type NegotiationManagerConfig = {
+	logger?: IMediaSignalLogger | null;
+};
+
+export type NegotiationEvents = {
+	'error': { errorCode: string };
+	'ended': void;
+	'local-sdp': { sdp: RTCSessionDescriptionInit };
+};
+
+export type NegotiationData = {
+	negotiationId: string;
+	sequence: number;
+	isPolite: boolean;
+
+	remoteOffer: RTCSessionDescriptionInit | null;
+};
+
+export interface INegotiationCompatibleMediaCall extends IClientMediaCall {
+	hasInputTrack(): boolean;
+}