From 7042a5a52d11c5700d65b6235f76ae32801e13ae Mon Sep 17 00:00:00 2001 From: Jaya Allamsetty Date: Fri, 24 Jan 2025 16:22:06 -0500 Subject: [PATCH 1/2] fix(codec) Debounce the call that calc codec intersection set. Calculate codec intersection set only once per second even when there is a burst of joins/leaves. Also, check for current codec before chaining a codec change operation when codec selection API is used. --- modules/RTC/TPCUtils.js | 6 +- modules/RTC/TraceablePeerConnection.js | 26 ++-- modules/qualitycontrol/CodecSelection.spec.js | 113 ++++++++++++++---- .../qualitycontrol/QualityController.spec.js | 4 +- modules/qualitycontrol/QualityController.ts | 34 +++++- modules/xmpp/JingleSessionPC.js | 3 +- 6 files changed, 146 insertions(+), 40 deletions(-) diff --git a/modules/RTC/TPCUtils.js b/modules/RTC/TPCUtils.js index efe5a19bb7..98537d677e 100644 --- a/modules/RTC/TPCUtils.js +++ b/modules/RTC/TPCUtils.js @@ -512,10 +512,10 @@ export class TPCUtils { const rtpSender = this.pc.findSenderForTrack(localVideoTrack.getTrack()); if (this.pc.usesCodecSelectionAPI() && rtpSender) { - const { codecs } = rtpSender.getParameters(); + const { encodings } = rtpSender.getParameters(); - if (codecs?.length) { - return codecs[0].mimeType.split('/')[1].toLowerCase(); + if (encodings[0].codec) { + return encodings[0].codec.mimeType.split('/')[1].toLowerCase(); } } diff --git a/modules/RTC/TraceablePeerConnection.js b/modules/RTC/TraceablePeerConnection.js index 6b9eacc420..71f0d582b2 100644 --- a/modules/RTC/TraceablePeerConnection.js +++ b/modules/RTC/TraceablePeerConnection.js @@ -1436,8 +1436,18 @@ TraceablePeerConnection.prototype.setVideoCodecs = function(codecList, screensha this.codecSettings.screenshareCodec = screenshareCodec; } - if (this.usesCodecSelectionAPI()) { - this.configureVideoSenderEncodings(); + if (!this.usesCodecSelectionAPI()) { + return; + } + + for (const track of this.getLocalVideoTracks()) { + const currentCodec = this.tpcUtils.getConfiguredVideoCodec(track); + + if (screenshareCodec && track.getVideoType() === VideoType.DESKTOP && screenshareCodec !== currentCodec) { + this.configureVideoSenderEncodings(track, screenshareCodec); + } else if (currentCodec !== codecList[0]) { + this.configureVideoSenderEncodings(track); + } } }; @@ -1904,16 +1914,16 @@ TraceablePeerConnection.prototype._enableSenderEncodings = async function(sender * that is currently selected. * * @param {JitsiLocalTrack} - The local track for which the sender encodings have to configured. + * @param {CodecMimeType} - The preferred codec for the video track. * @returns {Promise} promise that will be resolved when the operation is successful and rejected otherwise. */ -TraceablePeerConnection.prototype.configureVideoSenderEncodings = function(localVideoTrack = null) { - const preferredCodec = this.codecSettings.codecList[0]; +TraceablePeerConnection.prototype.configureVideoSenderEncodings = function(localVideoTrack, codec) { + const preferredCodec = codec ?? this.codecSettings.codecList[0]; if (localVideoTrack) { - return this.setSenderVideoConstraints( - this._senderMaxHeights.get(localVideoTrack.getSourceName()), - localVideoTrack, - preferredCodec); + const height = this._senderMaxHeights.get(localVideoTrack.getSourceName()) ?? VIDEO_QUALITY_LEVELS[0].height; + + return this.setSenderVideoConstraints(height, localVideoTrack, preferredCodec); } const promises = []; diff --git a/modules/qualitycontrol/CodecSelection.spec.js b/modules/qualitycontrol/CodecSelection.spec.js index f763a5b24e..aeaf18f309 100644 --- a/modules/qualitycontrol/CodecSelection.spec.js +++ b/modules/qualitycontrol/CodecSelection.spec.js @@ -50,43 +50,57 @@ describe('Codec Selection', () => { }; qualityController = new QualityController(conference, options); + jasmine.clock().install(); spyOn(jingleSession, 'setVideoCodecs'); }); - it('and remote endpoints use the new codec selection logic', () => { + afterEach(() => { + jasmine.clock().uninstall(); + }); + + it('and remote endpoints use the new codec selection logic', async () => { // Add a second user joining the call. participant1 = new MockParticipant('remote-1'); conference.addParticipant(participant1, [ 'vp9', 'vp8' ]); + await nextTick(1000); + expect(jingleSession.setVideoCodecs).toHaveBeenCalledTimes(1); + expect(jingleSession.setVideoCodecs).toHaveBeenCalledWith([ 'vp9', 'vp8' ], 'vp9'); // Add a third user joining the call with a subset of codecs. participant2 = new MockParticipant('remote-2'); conference.addParticipant(participant2, [ 'vp8' ]); - expect(jingleSession.setVideoCodecs).toHaveBeenCalledWith([ 'vp8' ], 'vp9'); - - // Make p2 leave the call + // Make p2 leave the call. conference.removeParticipant(participant2); - expect(jingleSession.setVideoCodecs).toHaveBeenCalledTimes(3); + + await nextTick(1000); + + expect(jingleSession.setVideoCodecs).toHaveBeenCalledTimes(2); + expect(jingleSession.setVideoCodecs).toHaveBeenCalledWith([ 'vp9', 'vp8' ], 'vp9'); }); - it('and remote endpoints use the old codec selection logic (RN)', () => { + it('and remote endpoints use the old codec selection logic (RN)', async () => { // Add a second user joining the call. participant1 = new MockParticipant('remote-1'); conference.addParticipant(participant1, null, 'vp8'); + await nextTick(1000); + + expect(jingleSession.setVideoCodecs).toHaveBeenCalledTimes(1); expect(jingleSession.setVideoCodecs).toHaveBeenCalledWith([ 'vp8' ], 'vp9'); // Add a third user (newer) to the call. participant2 = new MockParticipant('remote-2'); conference.addParticipant(participant2, [ 'vp9', 'vp8' ]); - expect(jingleSession.setVideoCodecs).toHaveBeenCalledWith([ 'vp8' ], 'vp9'); - // Make p1 leave the call conference.removeParticipant(participant1); - expect(jingleSession.setVideoCodecs).toHaveBeenCalledTimes(3); + + await nextTick(1000); + expect(jingleSession.setVideoCodecs).toHaveBeenCalledTimes(2); + expect(jingleSession.setVideoCodecs).toHaveBeenCalledWith([ 'vp9', 'vp8' ], 'vp9'); }); }); @@ -102,50 +116,61 @@ describe('Codec Selection', () => { qualityController = new QualityController(conference, options); spyOn(jingleSession, 'setVideoCodecs'); + jasmine.clock().install(); }); - it('and remote endpoints use the new codec selection logic', () => { + afterEach(() => { + jasmine.clock().uninstall(); + }); + + it('and remote endpoints use the new codec selection logic', async () => { // Add a second user joining the call. participant1 = new MockParticipant('remote-1'); conference.addParticipant(participant1, [ 'vp9', 'vp8', 'h264' ]); + await nextTick(1000); expect(jingleSession.setVideoCodecs).toHaveBeenCalledTimes(1); + expect(jingleSession.setVideoCodecs).toHaveBeenCalledWith([ 'vp9', 'vp8' ], undefined); // Add a third user joining the call with a subset of codecs. participant2 = new MockParticipant('remote-2'); conference.addParticipant(participant2, [ 'vp8' ]); - expect(jingleSession.setVideoCodecs).toHaveBeenCalledWith([ 'vp8' ], undefined); - // Make p2 leave the call conference.removeParticipant(participant2); - expect(jingleSession.setVideoCodecs).toHaveBeenCalledTimes(3); + + await nextTick(1000); + + expect(jingleSession.setVideoCodecs).toHaveBeenCalledTimes(2); }); - it('and remote endpoint prefers a codec that is locally disabled', () => { + it('and remote endpoint prefers a codec that is locally disabled', async () => { // Add a second user joining the call the prefers H.264 and VP8. participant1 = new MockParticipant('remote-1'); conference.addParticipant(participant1, [ 'h264', 'vp8' ]); + await nextTick(1200); expect(jingleSession.setVideoCodecs).toHaveBeenCalledWith([ 'vp8' ], undefined); }); - it('and remote endpoints use the old codec selection logic (RN)', () => { + it('and remote endpoints use the old codec selection logic (RN)', async () => { // Add a second user joining the call. participant1 = new MockParticipant('remote-1'); conference.addParticipant(participant1, null, 'vp8'); + await nextTick(1000); + expect(jingleSession.setVideoCodecs).toHaveBeenCalledTimes(1); expect(jingleSession.setVideoCodecs).toHaveBeenCalledWith([ 'vp8' ], undefined); // Add a third user (newer) to the call. participant2 = new MockParticipant('remote-2'); conference.addParticipant(participant2, [ 'vp9', 'vp8', 'h264' ]); - expect(jingleSession.setVideoCodecs).toHaveBeenCalledWith([ 'vp8' ], undefined); - // Make p1 leave the call conference.removeParticipant(participant1); - expect(jingleSession.setVideoCodecs).toHaveBeenCalledTimes(3); + + jasmine.clock().tick(1000); + expect(jingleSession.setVideoCodecs).toHaveBeenCalledTimes(2); }); }); @@ -171,11 +196,14 @@ describe('Codec Selection', () => { participant1 = new MockParticipant('remote-1'); conference.addParticipant(participant1, [ 'av1', 'vp9', 'vp8' ]); + + await nextTick(1000); + expect(jingleSession.setVideoCodecs).toHaveBeenCalledWith([ 'av1', 'vp9', 'vp8' ], undefined); participant2 = new MockParticipant('remote-2'); conference.addParticipant(participant2, [ 'av1', 'vp9', 'vp8' ]); - expect(jingleSession.setVideoCodecs).toHaveBeenCalledWith([ 'av1', 'vp9', 'vp8' ], undefined); + expect(jingleSession.setVideoCodecs).toHaveBeenCalledTimes(1); qualityController.codecController.changeCodecPreferenceOrder(localTrack, 'av1'); @@ -187,6 +215,7 @@ describe('Codec Selection', () => { // Expect the local endpoint to continue sending VP9. expect(jingleSession.setVideoCodecs).toHaveBeenCalledWith([ 'vp9', 'av1', 'vp8' ], undefined); + expect(jingleSession.setVideoCodecs).toHaveBeenCalledTimes(3); }); it('and does not change codec if the current codec is already the lowest complexity codec', async () => { @@ -196,6 +225,8 @@ describe('Codec Selection', () => { participant1 = new MockParticipant('remote-1'); conference.addParticipant(participant1, [ 'av1', 'vp9', 'vp8' ]); + + await nextTick(1000); expect(jingleSession.setVideoCodecs).toHaveBeenCalledWith([ 'vp8', 'vp9', 'av1' ], undefined); participant2 = new MockParticipant('remote-2'); @@ -239,11 +270,14 @@ describe('Codec Selection', () => { participant1 = new MockParticipant('remote-1'); conference.addParticipant(participant1, [ 'av1', 'vp9', 'vp8' ]); + + await nextTick(1000); + expect(jingleSession.setVideoCodecs).toHaveBeenCalledTimes(1); expect(jingleSession.setVideoCodecs).toHaveBeenCalledWith([ 'av1', 'vp9', 'vp8' ], undefined); participant2 = new MockParticipant('remote-2'); conference.addParticipant(participant2, [ 'av1', 'vp9', 'vp8' ]); - expect(jingleSession.setVideoCodecs).toHaveBeenCalledWith([ 'av1', 'vp9', 'vp8' ], undefined); + expect(jingleSession.setVideoCodecs).toHaveBeenCalledTimes(1); const sourceStats = { avgEncodeTime: 12, @@ -269,6 +303,8 @@ describe('Codec Selection', () => { participant3 = new MockParticipant('remote-3'); conference.addParticipant(participant3, [ 'av1', 'vp9', 'vp8' ]); + await nextTick(1000); + // Expect the local endpoint to continue sending VP9. expect(jingleSession.setVideoCodecs).toHaveBeenCalledWith([ 'vp9', 'av1', 'vp8' ], undefined); @@ -328,4 +364,41 @@ describe('Codec Selection', () => { expect(jingleSession.setVideoCodecs).toHaveBeenCalledTimes(0); }); }); + + describe('When multiple joins and leaves happen in a quick burst', () => { + beforeEach(() => { + options = { + jvb: { + preferenceOrder: [ 'AV1', 'VP9', 'VP8' ], + screenshareCodec: 'VP9' + }, + p2p: {} + }; + jasmine.clock().install(); + qualityController = new QualityController(conference, options); + spyOn(jingleSession, 'setVideoCodecs'); + }); + + afterEach(() => { + jasmine.clock().uninstall(); + }); + + it('should call setVideoCodecs only once within the same tick', async () => { + participant1 = new MockParticipant('remote-1'); + conference.addParticipant(participant1, [ 'vp9', 'vp8' ]); + + // Add a third user joining the call with a subset of codecs. + participant2 = new MockParticipant('remote-2'); + conference.addParticipant(participant2, [ 'vp8' ]); + + // Make p1 and p2 leave the call. + conference.removeParticipant(participant2); + conference.removeParticipant(participant1); + + await nextTick(1000); + + expect(jingleSession.setVideoCodecs).toHaveBeenCalledTimes(1); + expect(jingleSession.setVideoCodecs).toHaveBeenCalledWith([ 'av1', 'vp9', 'vp8' ], 'vp9'); + }); + }); }); diff --git a/modules/qualitycontrol/QualityController.spec.js b/modules/qualitycontrol/QualityController.spec.js index 9fc65e7ff7..2fd2d9b8cc 100644 --- a/modules/qualitycontrol/QualityController.spec.js +++ b/modules/qualitycontrol/QualityController.spec.js @@ -33,7 +33,7 @@ describe('QualityController', () => { p2p: {} }; localTrack = new MockLocalTrack('1', 720, 'camera'); - qualityController = new QualityController(conference, options, true); + qualityController = new QualityController(conference, options); sourceStats = { avgEncodeTime: 12, codec: 'VP8', @@ -165,7 +165,7 @@ describe('QualityController', () => { p2p: {} }; localTrack = new MockLocalTrack('1', 720, 'camera'); - qualityController = new QualityController(conference, options, true); + qualityController = new QualityController(conference, options); sourceStats = { avgEncodeTime: 12, codec: 'VP8', diff --git a/modules/qualitycontrol/QualityController.ts b/modules/qualitycontrol/QualityController.ts index b587f25e91..0d145f2cef 100644 --- a/modules/qualitycontrol/QualityController.ts +++ b/modules/qualitycontrol/QualityController.ts @@ -1,3 +1,4 @@ +import { throttle } from 'lodash-es'; import { getLogger } from '@jitsi/logger'; import JitsiConference from "../../JitsiConference"; @@ -145,12 +146,13 @@ export class QualityController { this._conference.on( JitsiConferenceEvents.CONFERENCE_VISITOR_CODECS_CHANGED, (codecList: CodecMimeType[]) => this._codecController.updateVisitorCodecs(codecList)); - this._conference.on( - JitsiConferenceEvents.USER_JOINED, - () => this._codecController.selectPreferredCodec(this._conference.jvbJingleSession)); - this._conference.on( - JitsiConferenceEvents.USER_LEFT, - () => this._codecController.selectPreferredCodec(this._conference.jvbJingleSession)); + + // Debounce the calls to codec selection when there is a burst of joins and leaves. + const debouncedSelectCodec = this._debounce( + () => this._codecController.selectPreferredCodec(this._conference.jvbJingleSession), + 1000); + this._conference.on(JitsiConferenceEvents.USER_JOINED, debouncedSelectCodec.bind(this)); + this._conference.on(JitsiConferenceEvents.USER_LEFT, debouncedSelectCodec.bind(this)); this._conference.rtc.on( RTCEvents.SENDER_VIDEO_CONSTRAINTS_CHANGED, (videoConstraints: IVideoConstraints) => this._sendVideoController.onSenderConstraintsReceived(videoConstraints)); @@ -159,6 +161,26 @@ export class QualityController { (tpc: TraceablePeerConnection, stats: Map) => this._processOutboundRtpStats(tpc, stats)); } + /** + * Creates a debounced function that delays the execution of the provided function until after the specified delay + * has elapsed. Unlike typical debounce implementations, the timer does not reset when the function is called again + * within the delay period. + * + * @param {Function} func - The function to be debounced. + * @param {number} delay - The delay in milliseconds. + * @returns {Function} - The debounced function. + */ + _debounce(func: Function, delay: number) { + return function (...args) { + if (!this._timer) { + this._timer = setTimeout(() => { + func.apply(this, args); + this._timer = null; + }, delay); + } + }; + } + /** * Adjusts the lastN value so that fewer remote video sources are received from the bridge in an attempt to improve * encode resolution of the outbound video streams based on cpuLimited parameter passed. If cpuLimited is false, diff --git a/modules/xmpp/JingleSessionPC.js b/modules/xmpp/JingleSessionPC.js index c9713c2ae6..62faf0943e 100644 --- a/modules/xmpp/JingleSessionPC.js +++ b/modules/xmpp/JingleSessionPC.js @@ -2283,7 +2283,6 @@ export default class JingleSessionPC extends JingleSession { */ setVideoCodecs(codecList, screenshareCodec) { if (this._assertNotEnded()) { - logger.info(`${this} setVideoCodecs: codecList=${codecList}, screenshareCodec=${screenshareCodec}`); this.peerconnection.setVideoCodecs(codecList, screenshareCodec); // Browser throws an error when H.264 is set on the encodings. Therefore, munge the SDP when H.264 needs to @@ -2307,6 +2306,8 @@ export default class JingleSessionPC extends JingleSession { videoType: VideoType.CAMERA }); + logger.info(`${this} setVideoCodecs: codecList=${codecList}, screenshareCodec=${screenshareCodec}`); + // Initiate a renegotiate for the codec setting to take effect. const workFunction = finishedCallback => { this._renegotiate() From b35ec07069280bfdff497dba9bb75197cd2c3d3a Mon Sep 17 00:00:00 2001 From: Jaya Allamsetty Date: Mon, 27 Jan 2025 10:59:23 -0500 Subject: [PATCH 2/2] squash: Address review comments --- modules/qualitycontrol/QualityController.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/modules/qualitycontrol/QualityController.ts b/modules/qualitycontrol/QualityController.ts index 0d145f2cef..d6dc75945e 100644 --- a/modules/qualitycontrol/QualityController.ts +++ b/modules/qualitycontrol/QualityController.ts @@ -1,4 +1,3 @@ -import { throttle } from 'lodash-es'; import { getLogger } from '@jitsi/logger'; import JitsiConference from "../../JitsiConference"; @@ -174,8 +173,8 @@ export class QualityController { return function (...args) { if (!this._timer) { this._timer = setTimeout(() => { - func.apply(this, args); this._timer = null; + func.apply(this, args); }, delay); } };