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

media: add Safari17 device handler #499

Conversation

kevinwhereby
Copy link
Contributor

@kevinwhereby kevinwhereby commented Dec 2, 2024


Description

Summary:

The Safari12 handler in mediasoup-client doesn't pass our list of encodings to
peerConnection.addTransceiver, so our rate limits weren't being
applied

The new version removes some problematic legacy simulcast stuff and
ensures the encodings have an rid as addTransceiver requires

The updated parts of the Safari12 handler all pretty much come from the Chrome111 handler

I also removed the deviceHandlerFactory init option from VegaRtcManager to keep all the custom handler code in one place

Related Issue:
https://linear.app/whereby/issue/COB-1331/mediasoup-safari-handler-not-using-simulcast-bandwidth-config

Testing

  1. add the canary to your local PWA
  2. join an SFU room with the ?stats url param in Safari and Chrome
  3. ensure video works
  4. in the displayed stats in Safari, make sure the bitrates for the 3 streams are roughly the same as what is shown in chrome:

Screenshots/GIFs (if applicable)

Checklist

  • My code follows the project's coding standards.
  • Prefixed the PR title and commit messages with the service or package name
  • I have written unit tests (if applicable).
  • I have updated the documentation (if applicable).
  • By submitting this pull request, I confirm that my contribution is made
    under the terms of the MIT license.

Additional Information

Here's a diff of the changes to the Safari12 handler:

diff --git a/src/handlers/Safari12.ts b/src/handlers/Safari12.ts
index 5cda7f9..4ea5793 100644
--- a/src/handlers/Safari12.ts
+++ b/src/handlers/Safari12.ts
@@ -1,11 +1,11 @@
 import * as sdpTransform from 'sdp-transform';
-import { Logger } from '../Logger';
-import * as utils from '../utils';
-import * as ortc from '../ortc';
-import * as sdpCommonUtils from './sdp/commonUtils';
-import * as sdpUnifiedPlanUtils from './sdp/unifiedPlanUtils';
-import * as ortcUtils from './ortc/utils';
-import { InvalidStateError } from '../errors';
+import { Logger } from 'mediasoup-client/lib/Logger';
+import * as utils from 'mediasoup-client/lib/utils';
+import * as ortc from 'mediasoup-client/lib/ortc';
+import * as sdpCommonUtils from 'mediasoup-client/lib/handlers/sdp/commonUtils';
+import * as sdpUnifiedPlanUtils from 'mediasoup-client/lib/handlers/sdp/unifiedPlanUtils';
+import * as ortcUtils from 'mediasoup-client/lib/handlers/ortc/utils';
+import { InvalidStateError } from 'mediasoup-client/lib/errors';
 import {
 	HandlerFactory,
 	HandlerInterface,
@@ -18,19 +18,25 @@ import {
 	HandlerSendDataChannelResult,
 	HandlerReceiveDataChannelOptions,
 	HandlerReceiveDataChannelResult,
-} from './HandlerInterface';
-import { RemoteSdp } from './sdp/RemoteSdp';
-import { parse as parseScalabilityMode } from '../scalabilityModes';
-import { IceParameters, DtlsRole } from '../Transport';
-import { RtpCapabilities, RtpParameters } from '../RtpParameters';
-import { SctpCapabilities, SctpStreamParameters } from '../SctpParameters';
+} from 'mediasoup-client/lib/handlers/HandlerInterface';
+import { RemoteSdp } from 'mediasoup-client/lib/handlers/sdp/RemoteSdp';
+import { parse as parseScalabilityMode } from 'mediasoup-client/lib/scalabilityModes';
+import { IceParameters, DtlsRole } from 'mediasoup-client/lib/Transport';
+import {
+	RtpCapabilities,
+	RtpParameters,
+} from 'mediasoup-client/lib/RtpParameters';
+import {
+	SctpCapabilities,
+	SctpStreamParameters,
+} from 'mediasoup-client/lib/SctpParameters';
 
-const logger = new Logger('Safari12');
+const logger = new Logger('Safari17');
 
-const NAME = 'Safari12';
+const NAME = 'Safari17';
 const SCTP_NUM_STREAMS = { OS: 1024, MIS: 1024 };
 
-export class Safari12 extends HandlerInterface {
+export class Safari17 extends HandlerInterface {
 	// Closed flag.
 	private _closed = false;
 	// Handler direction.
@@ -63,7 +69,7 @@ export class Safari12 extends HandlerInterface {
 	 * Creates a factory function.
 	 */
 	static createFactory(): HandlerFactory {
-		return (): Safari12 => new Safari12();
+		return (): Safari17 => new Safari17();
 	}
 
 	constructor() {
@@ -337,6 +343,30 @@ export class Safari12 extends HandlerInterface {
 			codec
 		);
 
+		if (encodings && encodings.length > 1) {
+			// Set rid and verify scalabilityMode in each encoding.
+			// NOTE: Even if WebRTC allows different scalabilityMode (different number
+			// of temporal layers) per simulcast stream, we need that those are the
+			// same in all them, so let's pick up the highest value.
+			// NOTE: If scalabilityMode is not given, Chrome will use L1T3.
+			let maxTemporalLayers = 1;
+
+			for (const encoding of encodings) {
+				const temporalLayers = encoding.scalabilityMode
+					? parseScalabilityMode(encoding.scalabilityMode).temporalLayers
+					: 3;
+
+				if (temporalLayers > maxTemporalLayers) {
+					maxTemporalLayers = temporalLayers;
+				}
+			}
+
+			encodings.forEach((encoding, idx: number) => {
+				encoding.rid = `r${idx}`;
+				encoding.scalabilityMode = `L1T${maxTemporalLayers}`;
+			});
+		}
+
 		const sendingRemoteRtpParameters = utils.clone<RtpParameters>(
 			this._sendingRemoteRtpParametersByKind![track.kind]
 		);
@@ -348,18 +378,33 @@ export class Safari12 extends HandlerInterface {
 		);
 
 		const mediaSectionIdx = this._remoteSdp!.getNextMediaSectionIdx();
-		const transceiver = this._pc.addTransceiver(track, {
-			direction: 'sendonly',
-			streams: [this._sendStream],
-		});
+		let transceiver: RTCRtpTransceiver;
+		try {
+			transceiver = this._pc.addTransceiver(track, {
+				direction: 'sendonly',
+				streams: [this._sendStream],
+				sendEncodings: encodings,
+			});
+		} catch (e) {
+			if ((e as Error).message === 'Type error') {
+				console.warn(
+					'PeerConnection::addTransceiver with encodings failed, retrying without encodings'
+				);
+				transceiver = this._pc.addTransceiver(track, {
+					direction: 'sendonly',
+					streams: [this._sendStream],
+				});
+			} else {
+				throw e;
+			}
+		}
 
 		if (onRtpSender) {
 			onRtpSender(transceiver.sender);
 		}
 
-		let offer = await this._pc.createOffer();
+		const offer = await this._pc.createOffer();
 		let localSdpObject = sdpTransform.parse(offer.sdp);
-		let offerMediaObject;
 
 		if (!this._transportReady) {
 			await this.setupTransport({
@@ -370,50 +415,44 @@ export class Safari12 extends HandlerInterface {
 
 		const layers = parseScalabilityMode((encodings ?? [{}])[0].scalabilityMode);
 
-		if (encodings && encodings.length > 1) {
-			logger.debug('send() | enabling legacy simulcast');
-
-			localSdpObject = sdpTransform.parse(offer.sdp);
-			offerMediaObject = localSdpObject.media[mediaSectionIdx.idx];
-
-			sdpUnifiedPlanUtils.addLegacySimulcast({
-				offerMediaObject,
-				numStreams: encodings.length,
-			});
-
-			offer = { type: 'offer', sdp: sdpTransform.write(localSdpObject) };
-		}
-
 		logger.debug('send() | calling pc.setLocalDescription() [offer:%o]', offer);
 
 		await this._pc.setLocalDescription(offer);
 
 		// We can now get the transceiver.mid.
-		const localId = transceiver.mid;
+		const localId = transceiver.mid!;
 
 		// Set MID.
 		sendingRtpParameters.mid = localId;
 
 		localSdpObject = sdpTransform.parse(this._pc.localDescription.sdp);
-		offerMediaObject = localSdpObject.media[mediaSectionIdx.idx];
+		const offerMediaObject = localSdpObject.media[mediaSectionIdx.idx];
 
 		// Set RTCP CNAME.
 		sendingRtpParameters.rtcp!.cname = sdpCommonUtils.getCname({
 			offerMediaObject,
 		});
 
-		// Set RTP encodings.
-		sendingRtpParameters.encodings = sdpUnifiedPlanUtils.getRtpEncodings({
-			offerMediaObject,
-		});
+		// Set RTP encodings by parsing the SDP offer if no encodings are given.
+		if (!encodings) {
+			sendingRtpParameters.encodings = sdpUnifiedPlanUtils.getRtpEncodings({
+				offerMediaObject,
+			});
+		}
+		// Set RTP encodings by parsing the SDP offer and complete them with given
+		// one if just a single encoding has been given.
+		else if (encodings.length === 1) {
+			const newEncodings = sdpUnifiedPlanUtils.getRtpEncodings({
+				offerMediaObject,
+			});
 
-		// Complete encodings with given values.
-		if (encodings) {
-			for (let idx = 0; idx < sendingRtpParameters.encodings.length; ++idx) {
-				if (encodings[idx]) {
-					Object.assign(sendingRtpParameters.encodings[idx], encodings[idx]);
-				}
-			}
+			Object.assign(newEncodings[0], encodings[0]);
+
+			sendingRtpParameters.encodings = newEncodings;
+		}
+		// Otherwise if more than 1 encoding are given use them verbatim.
+		else {
+			sendingRtpParameters.encodings = encodings;
 		}
 
 		// If VP8 or H264 and there is effective simulcast, add scalabilityMode to

Copy link

changeset-bot bot commented Dec 2, 2024

🦋 Changeset detected

Latest commit: 8124373

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@whereby.com/media Patch
@whereby.com/core Patch
@whereby.com/browser-sdk Patch
@whereby.com/react-native-sdk Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@kevinwhereby kevinwhereby force-pushed the kevinhanna/cob-1331-mediasoup-safari-handler-not-using-simulcast-bandwidth branch from 684f822 to dec2c56 Compare December 2, 2024 12:52
@kevinwhereby
Copy link
Contributor Author

/canary

@kevinwhereby kevinwhereby force-pushed the kevinhanna/cob-1331-mediasoup-safari-handler-not-using-simulcast-bandwidth branch 5 times, most recently from 1fb2ab0 to 02bf368 Compare December 2, 2024 13:09
@kevinwhereby
Copy link
Contributor Author

/canary

@kevinwhereby kevinwhereby force-pushed the kevinhanna/cob-1331-mediasoup-safari-handler-not-using-simulcast-bandwidth branch 17 times, most recently from 8093032 to b78b672 Compare December 6, 2024 12:14
} catch (e) {
if ((e as Error).message === "Type error") {
console.warn("addTransceiver with encodings failed, retrying without encodings");
transceiver = this._pc.addTransceiver(track, {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe if we get here we should do something like encodings = undefined
As later we use the presence of that array to do some other stuff.

But this is only here to fix a problem with the Playwright Webkit browser. using sendEncodings in addTransceiver works fine in 'real' safari 17.4 and 18.1.1, so maybe we don't need to worry about it?

@kevinwhereby kevinwhereby force-pushed the kevinhanna/cob-1331-mediasoup-safari-handler-not-using-simulcast-bandwidth branch from b78b672 to 3bebbc1 Compare December 6, 2024 12:44
@kevinwhereby kevinwhereby requested a review from a team December 6, 2024 12:56
@kevinwhereby
Copy link
Contributor Author

/canary

Copy link
Contributor

🚀 The canary releases have been published to npm.

You can test the releases by installing the newly published versions:

yarn add @whereby.com/browser-sdk@0.0.0-canary-20241210112249
yarn add @whereby.com/core@0.0.0-canary-20241210112249
yarn add @whereby.com/media@0.0.0-canary-20241210112249
yarn add @whereby.com/react-native-sdk@0.0.0-canary-20241210112249

@kevinwhereby kevinwhereby force-pushed the kevinhanna/cob-1331-mediasoup-safari-handler-not-using-simulcast-bandwidth branch 2 times, most recently from fd7bf27 to f32340e Compare December 10, 2024 12:59
@kevinwhereby kevinwhereby force-pushed the kevinhanna/cob-1331-mediasoup-safari-handler-not-using-simulcast-bandwidth branch from f32340e to 765a588 Compare December 11, 2024 14:51
The handler in mediasoup-client doesn't pass our list of encodings to
`peerConnection.addTransceiver`, so our rate limits weren't being
applied

The new version removes some problematic legacy simulcast stuff and
ensures the encodings have an `rid` as `addTransceiver` requires
@kevinwhereby kevinwhereby force-pushed the kevinhanna/cob-1331-mediasoup-safari-handler-not-using-simulcast-bandwidth branch 5 times, most recently from 838fac8 to a6a2567 Compare December 11, 2024 16:00
This was added to support using the sdk in a node client, but now we
have two custom factories it makes sense to use them in the same way
@kevinwhereby kevinwhereby force-pushed the kevinhanna/cob-1331-mediasoup-safari-handler-not-using-simulcast-bandwidth branch from a6a2567 to 8124373 Compare December 11, 2024 16:02
Copy link
Contributor

@pnts-se-whereby pnts-se-whereby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🍰

@kevinwhereby kevinwhereby merged commit 67c8ea2 into main Jan 6, 2025
3 checks passed
@kevinwhereby kevinwhereby deleted the kevinhanna/cob-1331-mediasoup-safari-handler-not-using-simulcast-bandwidth branch January 6, 2025 10:55
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.

2 participants