fix(TPC) Disable media instead of changing dir for p2p->jvb switch. (#2226)

* fix(TPC) Disable media instead of changing dir for p2p->jvb switch.
Resume or suspend the media on the jvb peerconnection by changing the RTCRtpEncodingParamters.active state instead of changing the direction on the transceiver. This avoids the needs to start a O/A renegotiation cycle for these operations. The media direction will be changed only for p2p lastn=0 case since video needs to be disabled on both the sender and the peer for p2p lastn=0 case.

* Address review comments

* Disable media after adding source while media is suspended on the jvb connection. Default 'active' state for stream encodings after the source is added is 'true'.

* Wait for all the promises to be settled before returning

JitsiConference.js

     }
     this.receiveVideoController.setLastN(n);
 
-    // If the P2P session is not fully established yet, we wait until it gets
-    // established.
+    // If the P2P session is not fully established yet, we wait until it gets established.
     if (this.p2pJingleSession) {
         const isVideoActive = n !== 0;
 
         this.p2pJingleSession
-            .setMediaTransferActive(true, isVideoActive)
+            .setP2pVideoTransferActive(isVideoActive)
             .catch(error => {
-                logger.error(
-                    `Failed to adjust video transfer status (${isVideoActive})`,
-                    error);
+                logger.error(`Failed to adjust video transfer status (${isVideoActive})`, error);
             });
     }
 };
  */
 JitsiConference.prototype._resumeMediaTransferForJvbConnection = function() {
     logger.info('Resuming media transfer over the JVB connection...');
-    this.jvbJingleSession.setMediaTransferActive(true, true).then(
-        () => {
+    this.jvbJingleSession.setMediaTransferActive(true)
+        .then(() => {
             logger.info('Resumed media transfer over the JVB connection!');
-        },
-        error => {
-            logger.error(
-                'Failed to resume media transfer over the JVB connection:',
-                error);
+        })
+        .catch(error => {
+            logger.error('Failed to resume media transfer over the JVB connection:', error);
         });
 };
 
         // when the lastN value was being adjusted.
         const isVideoActive = this.getLastN() !== 0;
 
-        this.p2pJingleSession
-            .setMediaTransferActive(true, isVideoActive)
+        this.p2pJingleSession.setP2pVideoTransferActive(isVideoActive)
             .catch(error => {
-                logger.error(
-                    'Failed to sync up P2P video transfer status'
-                        + `(${isVideoActive})`, error);
+                logger.error(`Failed to sync up P2P video transfer status (${isVideoActive}), ${error}`);
             });
     } else {
         logger.info('Peer to peer connection closed!');
  */
 JitsiConference.prototype._suspendMediaTransferForJvbConnection = function() {
     logger.info('Suspending media transfer over the JVB connection...');
-    this.jvbJingleSession.setMediaTransferActive(false, false).then(
-        () => {
+    this.jvbJingleSession.setMediaTransferActive(false)
+        .then(() => {
             logger.info('Suspended media transfer over the JVB connection !');
-        },
-        error => {
-            logger.error(
-                'Failed to suspend media transfer over the JVB connection:',
-                error);
+        })
+        .catch(error => {
+            logger.error('Failed to suspend media transfer over the JVB connection:', error);
         });
 };
 

modules/RTC/TPCUtils.js

 
         return localTrack.isVideoTrack()
             ? [ {
-                active: true,
+                active: this.pc.videoTransferActive,
                 maxBitrate: this.videoBitrates.high
             } ]
-            : [ { active: true } ];
+            : [ { active: this.pc.audioTransferActive } ];
     }
 
     /**
 
         return [
             {
-                active: true,
+                active: this.pc.videoTransferActive,
                 maxBitrate: browser.isFirefox() ? maxVideoBitrate : this.encodingBitrates.low,
                 rid: SIM_LAYER_1_RID,
                 scaleResolutionDownBy: browser.isFirefox() ? HD_SCALE_FACTOR : LD_SCALE_FACTOR
             },
             {
-                active: true,
+                active: this.pc.videoTransferActive,
                 maxBitrate: this.encodingBitrates.standard,
                 rid: SIM_LAYER_2_RID,
                 scaleResolutionDownBy: SD_SCALE_FACTOR
             },
             {
-                active: true,
+                active: this.pc.videoTransferActive,
                 maxBitrate: browser.isFirefox() ? this.encodingBitrates.low : maxVideoBitrate,
                 rid: SIM_LAYER_3_RID,
                 scaleResolutionDownBy: browser.isFirefox() ? LD_SCALE_FACTOR : HD_SCALE_FACTOR
         return encodingsBitrates;
     }
 
+    /**
+     * Returns the max resolution that the client is configured to encode for a given local video track. The actual
+     * send resolution might be downscaled based on cpu and bandwidth constraints.
+     *
+     * @param {JitsiLocalTrack} localVideoTrack - The local video track.
+     * @returns {number} The max encoded resolution for the given video track.
+     */
+    getConfiguredEncodeResolution(localVideoTrack) {
+        const localTrack = localVideoTrack.getTrack();
+        const { height } = localTrack.getSettings();
+        const videoSender = this.pc.findSenderForTrack(localVideoTrack.getTrack());
+        let maxHeight = 0;
+
+        if (!videoSender) {
+            return maxHeight;
+        }
+        const parameters = videoSender.getParameters();
+
+        if (!parameters?.encodings?.length) {
+            return maxHeight;
+        }
+        for (const encoding of parameters.encodings) {
+            if (encoding.active) {
+                maxHeight = Math.max(maxHeight, height / encoding.scaleResolutionDownBy);
+            }
+        }
+
+        return maxHeight;
+    }
+
     /**
      * Replaces the existing track on a RTCRtpSender with the given track.
      *
             .then(() => Promise.resolve(transceiver));
     }
 
-    /**
-    * Enables/disables audio transmission on the peer connection. When
-    * disabled the audio transceiver direction will be set to 'inactive'
-    * which means that no data will be sent nor accepted, but
-    * the connection should be kept alive.
-    * @param {boolean} active - true to enable audio media transmission or
-    * false to disable.
-    * @returns {void}
-    */
-    setAudioTransferActive(active) {
-        this.setMediaTransferActive(MediaType.AUDIO, active);
-    }
-
     /**
      * Set the simulcast stream encoding properties on the RTCRtpSender.
      * @param {JitsiLocalTrack} track - the current track in use for which
     }
 
     /**
-     * Enables/disables media transmission on the peerconnection by changing the direction
-     * on the transceiver for the specified media type.
-     * @param {String} mediaType - 'audio' or 'video'
-     * @param {boolean} active - true to enable media transmission or false
-     * to disable.
-     * @returns {void}
+     * Resumes or suspends media on the peerconnection by setting the active state on RTCRtpEncodingParameters
+     * associated with all the senders that have a track attached to it.
+     *
+     * @param {boolean} enable - whether media needs to be enabled or suspended.
+     * @returns {Promise} - A promise that is resolved when the change is succesful on all the senders, rejected
+     * otherwise.
      */
-    setMediaTransferActive(mediaType, active) {
-        const transceivers = this.pc.peerconnection.getTransceivers()
-            .filter(t => t.receiver && t.receiver.track && t.receiver.track.kind === mediaType);
+    setMediaTransferActive(enable) {
+        logger.info(`${this.pc} ${enable ? 'Resuming' : 'Suspending'} media transfer.`);
 
-        logger.info(`${this.pc} ${active ? 'Enabling' : 'Suspending'} ${mediaType} media transfer.`);
-        transceivers.forEach(transceiver => {
-            if (active) {
-                const localTrackMids = Array.from(this.pc._localTrackTransceiverMids);
+        const senders = this.pc.peerconnection.getSenders().filter(s => Boolean(s.track));
+        const promises = [];
 
-                if (localTrackMids.find(mids => mids[1] === transceiver.mid)) {
-                    transceiver.direction = MediaDirection.SENDRECV;
-                } else {
-                    transceiver.direction = MediaDirection.RECVONLY;
+        for (const sender of senders) {
+            const parameters = sender.getParameters();
+
+            if (parameters?.encodings?.length) {
+                for (const encoding of parameters.encodings) {
+                    encoding.active = enable;
                 }
-            } else {
-                transceiver.direction = MediaDirection.INACTIVE;
             }
-        });
+            promises.push(sender.setParameters(parameters));
+        }
+
+        return Promise.allSettled(promises)
+            .then(settledResult => {
+                const errors = settledResult
+                    .filter(result => result.status === 'rejected')
+                    .map(result => result.reason);
+
+                if (errors.length) {
+                    return Promise.reject(new Error('Failed to change encodings on the RTCRtpSenders'
+                        + `${errors.join(' ')}`));
+                }
+
+                return Promise.resolve();
+            });
     }
 
     /**
-    * Enables/disables video media transmission on the peer connection. When
-    * disabled the SDP video media direction in the local SDP will be adjusted to
-    * 'inactive' which means that no data will be sent nor accepted, but
-    * the connection should be kept alive.
-    * @param {boolean} active - true to enable video media transmission or
-    * false to disable.
-    * @returns {void}
-    */
+     * Enables/disables video media transmission on the peer connection. When disabled the SDP video media direction in
+     * the local SDP will be adjusted to 'inactive' which means that no data will be sent nor accepted, but the
+     * connection should be kept alive. This is used for setting lastn=0 on p2p connection.
+     *
+     * @param {boolean} active - true to enable media transmission or false to disable.
+     * @returns {void}
+     */
     setVideoTransferActive(active) {
-        this.setMediaTransferActive(MediaType.VIDEO, active);
+        const transceivers = this.pc.peerconnection.getTransceivers()
+            .filter(t => t.receiver && t.receiver.track && t.receiver.track.kind === MediaType.VIDEO);
+
+        logger.info(`${this.pc} ${active ? 'Enabling' : 'Suspending'} video media transfer.`);
+        transceivers.forEach(transceiver => {
+            const localTrackMids = Array.from(this.pc._localTrackTransceiverMids);
+            const direction = active
+                ? localTrackMids.find(mids => mids[1] === transceiver.mid)
+                    ? MediaDirection.SENDRECV : MediaDirection.RECVONLY
+                : MediaDirection.INACTIVE;
+
+            logger.debug(`Setting direction to ${direction} on mid=${transceiver.mid}`);
+            transceiver.direction = direction;
+        });
     }
 
     /**

modules/RTC/TraceablePeerConnection.js

 
     if (this._usesUnifiedPlan) {
         logger.debug(`${this} TPC.replaceTrack using unified plan`);
-        const mediaType = newTrack?.getType() ?? oldTrack?.getType();
 
         return this.tpcUtils.replaceTrack(oldTrack, newTrack)
             .then(transceiver => {
                         this.localSSRCs.set(newTrack.rtcId, oldTrackSSRC);
                     }
                 }
-                const mediaActive = mediaType === MediaType.AUDIO
-                    ? this.audioTransferActive
-                    : this.videoTransferActive;
-
-                // Set the transceiver direction only if media is not suspended on the connection. This happens when
-                // the client is using the p2p connection. Transceiver direction is updated when media is resumed on
-                // this connection again.
-                if (transceiver && mediaActive) {
+
+                if (transceiver) {
                     // In the scenario where we remove the oldTrack (oldTrack is not null and newTrack is null) on FF
                     // if we change the direction to RECVONLY, create answer will generate SDP with only 1 receive
                     // only ssrc instead of keeping all 6 ssrcs that we currently have. Stopping the screen sharing
                     // RECVONLY if FF still sends the media even though the enabled flag is set to false.
                     transceiver.direction
                         = newTrack || browser.isFirefox() ? MediaDirection.SENDRECV : MediaDirection.RECVONLY;
-                } else if (transceiver) {
-                    transceiver.direction = MediaDirection.INACTIVE;
                 }
 
                 // Avoid configuring the encodings on Chromium/Safari until simulcast is configured
  * @returns {Promise} promise that will be resolved when the operation is successful and rejected otherwise.
  */
 TraceablePeerConnection.prototype.configureSenderVideoEncodings = function(localVideoTrack = null) {
+    // If media is suspended on the peerconnection, make sure that media stays disabled. The default 'active' state for
+    // the encodings after the source is added to the peerconnection is 'true', so it needs to be explicitly disabled
+    // after the source is added.
+    if (!(this.videoTransferActive && this.audioTransferActive)) {
+        return this.tpcUtils.setMediaTransferActive(false);
+    }
+
     if (localVideoTrack) {
         return this.setSenderVideoConstraints(
             this._senderMaxHeights.get(localVideoTrack.getSourceName()),
     });
 };
 
-/**
- * Enables/disables audio media transmission on this peer connection. When
- * disabled the SDP audio media direction in the local SDP will be adjusted to
- * 'inactive' which means that no data will be sent nor accepted, but
- * the connection should be kept alive.
- * @param {boolean} active <tt>true</tt> to enable audio media transmission or
- * <tt>false</tt> to disable. If the value is not a boolean the call will have
- * no effect.
- * @return {boolean} <tt>true</tt> if the value has changed and sRD/sLD cycle
- * needs to be executed in order for the changes to take effect or
- * <tt>false</tt> if the given value was the same as the previous one.
- * @public
- */
-TraceablePeerConnection.prototype.setAudioTransferActive = function(active) {
-    logger.debug(`${this} audio transfer active: ${active}`);
-    const changed = this.audioTransferActive !== active;
-
-    this.audioTransferActive = active;
-
-    if (this._usesUnifiedPlan) {
-        this.tpcUtils.setAudioTransferActive(active);
-
-        // false means no renegotiation up the chain which is not needed in the Unified mode
-        return false;
-    }
-
-    return changed;
-};
-
 TraceablePeerConnection.prototype.setRemoteDescription = function(description) {
     let remoteDescription = description;
 
     if (frameHeight < 0) {
         throw new Error(`Invalid frameHeight: ${frameHeight}`);
     }
-
     const sourceName = localVideoTrack.getSourceName();
 
-    if (this._senderMaxHeights.get(sourceName) === frameHeight) {
+    this._senderMaxHeights.set(sourceName, frameHeight);
+
+    // Ignore sender constraints for the following cases -
+    // 1. If the media on the peerconnection is suspended (jvb conn when p2p is currently active).
+    // 2. If the client is already sending video of the requested resolution.
+    if (!this.videoTransferActive || this.tpcUtils.getConfiguredEncodeResolution(localVideoTrack) === frameHeight) {
         return Promise.resolve();
     }
-    this._senderMaxHeights.set(sourceName, frameHeight);
 
     if (!localVideoTrack || localVideoTrack.isMuted()) {
         return Promise.resolve();
                     .find(layer => layer.scaleResolutionDownBy === scaleFactor)?.maxBitrate ?? bitrate;
             }
             parameters.encodings[0].maxBitrate = bitrate;
+        } else {
+            parameters.encodings[0].maxBitrate = undefined;
         }
     } else {
         parameters.encodings[0].active = false;

modules/xmpp/JingleSessionPC.js

         });
     }
 
+    /**
+     * Resumes or suspends media transfer over the underlying peer connection.
+     *
+     * @param {boolean} active - <tt>true</tt> to enable media transfer or <tt>false</tt> to suspend media transmission
+     * @returns {Promise}
+     */
+    setMediaTransferActive(active) {
+        return this.peerconnection.tpcUtils.setMediaTransferActive(active)
+            .then(() => {
+                this.peerconnection.audioTransferActive = active;
+                this.peerconnection.videoTransferActive = active;
+            });
+    }
+
     /**
      * Replaces <tt>oldTrack</tt> with <tt>newTrack</tt> and performs a single
      * offer/answer cycle after both operations are done. Either
     }
 
     /**
-     * Resumes or suspends media transfer over the underlying peer connection.
-     * @param {boolean} audioActive <tt>true</tt> to enable audio media
-     * transfer or <tt>false</tt> to suspend audio media transmission.
-     * @param {boolean} videoActive <tt>true</tt> to enable video media
-     * transfer or <tt>false</tt> to suspend video media transmission.
-     * @return {Promise} a <tt>Promise</tt> which will resolve once
-     * the operation is done. It will be rejected with an error description as
-     * a string in case anything goes wrong.
+     * Resumes or suspends video media transfer over the p2p peer connection.
+     *
+     * @param {boolean} videoActive <tt>true</tt> to enable video media transfer or <tt>false</tt> to suspend video
+     * media transmission.
+     * @return {Promise} a <tt>Promise</tt> which will resolve once the operation is done. It will be rejected with
+     * an error description as a string in case anything goes wrong.
      */
-    setMediaTransferActive(audioActive, videoActive) {
+    setP2pVideoTransferActive(videoActive) {
         if (!this.peerconnection) {
-            return Promise.reject(
-                'Can not modify transfer active state,'
+            return Promise.reject('Can not modify video transfer active state,'
                     + ' before "initialize" is called');
         }
 
-        const logAudioStr = audioActive ? 'audio active' : 'audio inactive';
         const logVideoStr = videoActive ? 'video active' : 'video inactive';
 
-        logger.info(`${this} Queued make ${logVideoStr}, ${logAudioStr} task`);
+        logger.info(`${this} Queued make ${logVideoStr} task`);
 
         const workFunction = finishedCallback => {
             const isSessionActive = this.state === JingleSessionState.ACTIVE;
 
-            // Because the value is modified on the queue it's impossible to
-            // check it's final value reliably prior to submitting the task.
-            // The rule here is that the last submitted state counts.
-            // Check the values here to avoid unnecessary renegotiation cycle.
-            const audioActiveChanged
-                = this.peerconnection.setAudioTransferActive(audioActive);
-
             if (this._localVideoActive !== videoActive) {
                 this._localVideoActive = videoActive;
-
-                // Do only for P2P - Jicofo will reply with 'bad-request'
-                // We don't want to send 'content-modify', before the initial
-                // O/A (state === JingleSessionState.ACTIVE), because that will
-                // mess up video media direction in the remote SDP.
-                // 'content-modify' when processed only affects the media
-                // direction in the local SDP. We're doing that, because setting
-                // 'inactive' on video media in remote SDP will mess up our SDP
-                // translation chain (simulcast, RTX, video mute etc.).
                 if (this.isP2P && isSessionActive) {
                     this.sendContentModify();
                 }
             }
 
-            const pcVideoActiveChanged
-                = this.peerconnection.setVideoTransferActive(
-                    this._localVideoActive && this._remoteVideoActive);
+            this.peerconnection.setVideoTransferActive(this._localVideoActive && this._remoteVideoActive);
 
-            // Will do the sRD/sLD cycle to update SDPs and adjust the media
-            // direction
-            if (isSessionActive
-                    && (audioActiveChanged || pcVideoActiveChanged)) {
-                this._renegotiate()
-                    .then(
-                        finishedCallback,
-                        finishedCallback /* will be called with an error */);
-            } else {
-                finishedCallback();
-            }
+            // Always initiate a renegotiation cycle for p2p connection when the media direction is changed.
+            this._renegotiate()
+                .then(() => finishedCallback())
+                .catch(error => finishedCallback(error));
         };
 
         return new Promise((resolve, reject) => {
                 workFunction,
                 error => {
                     if (error) {
-                        logger.error(`${this} Make ${logVideoStr}, ${logAudioStr} task failed!`);
+                        logger.error(`${this} Make ${logVideoStr} task failed!`);
                         reject(error);
                     } else {
-                        logger.debug(`${this} Make ${logVideoStr}, ${logAudioStr} task done!`);
+                        logger.debug(`${this} Make ${logVideoStr} task done!`);
                         resolve();
                     }
                 });
     }
 
     /**
-     * Processes new value of remote video "senders" Jingle attribute and tries
-     * to apply it for {@link _remoteVideoActive}.
-     * @param {string} remoteVideoSenders the value of "senders" attribute of
-     * Jingle video content element advertised by remote peer.
-     * @return {boolean} <tt>true</tt> if the change affected state of
-     * the underlying peerconnection and renegotiation is required for
-     * the changes to take effect.
+     * Processes new value of remote video "senders" Jingle attribute and tries to apply it for
+     * {@link _remoteVideoActive}.
+     * @param {string} remoteVideoSenders the value of "senders" attribute of Jingle video content element advertised
+     * by remote peer.
+     * @return {boolean} <tt>true</tt> if the change affected state of the underlying peerconnection and renegotiation
+     * is required for the changes to take effect.
      * @private
      */
     _modifyRemoteVideoActive(remoteVideoSenders) {
         if (isRemoteVideoActive !== this._remoteVideoActive) {
             logger.debug(`${this} new remote video active: ${isRemoteVideoActive}`);
             this._remoteVideoActive = isRemoteVideoActive;
+
+            return this.peerconnection.setVideoTransferActive(this._localVideoActive && this._remoteVideoActive);
         }
 
-        return this.peerconnection.setVideoTransferActive(this._localVideoActive && this._remoteVideoActive);
+        return false;
     }
 
     /**

types/hand-crafted/modules/RTC/TPCUtils.d.ts

   getLocalStreamHeightConstraints: ( localTrack: JitsiLocalTrack ) => number[];
   removeTrackMute: ( localTrack: JitsiLocalTrack ) => Promise<void>;
   replaceTrack: ( oldTrack: JitsiLocalTrack, newTrack: JitsiLocalTrack ) => Promise<void>;
-  setAudioTransferActive: ( active: boolean ) => void;
   setEncodings: ( track: JitsiLocalTrack ) => Promise<void>;
-  setMediaTransferActive: ( mediaType: MediaType, active: boolean ) => void;
+  setMediaTransferActive: ( active: boolean ) => void;
   setVideoTransferActive: ( active: boolean ) => void;
   updateEncodingsResolution: ( parameters: RTCRtpEncodingParameters ) => void;
 }

types/hand-crafted/modules/RTC/TraceablePeerConnection.d.ts

   removeTrackMute: ( localTrack: JitsiLocalTrack ) => Promise<boolean>;
   createDataChannel: ( label: unknown, opts: unknown ) => unknown; // TODO:
   setLocalDescription: ( description: unknown ) => Promise<unknown>;
-  setAudioTransferActive: ( active: boolean ) => boolean;
   setSenderVideoDegradationPreference: () => Promise<void>;
   setMaxBitRate: () => Promise<void>; // TODO: definite bug in the JSDocs
   setRemoteDescription: ( description: unknown ) => unknown; // TODO:

types/hand-crafted/modules/xmpp/JingleSessionPC.d.ts

   replaceTrack: ( oldTrack: JitsiLocalTrack | null, newTrack: JitsiLocalTrack | null ) => Promise<unknown>; // TODO:
   addTrackAsUnmute: ( track: JitsiLocalTrack ) => Promise<unknown>; // TODO:
   removeTrackAsMute: ( track: JitsiLocalTrack ) => Promise<unknown>; // TODO:
-  setMediaTransferActive: ( audioActive: boolean, videoActive: boolean ) => Promise<unknown>; // TODO:
+  setMediaTransferActive: ( videoActive: boolean ) => Promise<unknown>; // TODO:
   modifyContents: ( jingleContents: JQuery ) => void;
   notifyMySSRCUpdate: ( oldSDP: unknown, newSDP: unknown ) => void; // TODO:
   newJingleErrorHandler: ( request: unknown, failureCb: ( error: Error ) => void ) => ( this: JingleSessionPC ) => unknown; // TODO: