ref(TPC) Move all the utility functions to TPCUtils. #2599

jallamsetty1 · 2024-11-18T21:14:44Z

Move all utility functions to TPCUtils that include

munging the SDP.
getting current codecs
getting expected transceiver direction.
getting expected settings for video encodings.

Move all functions that call RTCPeerConnection APIs directly back to TPC.

saghul

Left some comments!

modules/RTC/TPCUtils.js

saghul · 2024-11-19T16:01:25Z

modules/RTC/TPCUtils.js

 */
 export class TPCUtils {
    /**
     * Creates a new instance for a given TraceablePeerConnection
     *
     * @param peerconnection - the tpc instance for which we have utility functions.
     */
-    constructor(peerconnection) {
+    constructor(peerconnection, options) {


Make options default to {}, it will help you later.

Also document it above.

Yep, makes sense! Will modify it.

saghul · 2024-11-19T16:03:36Z

modules/RTC/TPCUtils.js

+        }
+
+        const sdp = this.pc.remoteDescription?.sdp;
+        const defaultCodec = CodecMimeType.VP8;


Constant outside of here?

saghul · 2024-11-19T16:03:50Z

modules/RTC/TPCUtils.js

+        if (this.pc.usesCodecSelectionAPI() && rtpSender) {
+            const { codecs } = rtpSender.getParameters();
+
+            return codecs[0].mimeType.split('/')[1].toLowerCase();


Is there a chance the codec list is empty?

No, it shouldn't be unless there is a bug in the browser. Will add a check for that.

modules/RTC/TPCUtils.js

saghul · 2024-11-19T16:09:42Z

modules/RTC/TPCUtils.js

+            }
+        });
+
+        return new RTCSessionDescription({


saghul · 2024-11-19T16:10:14Z

modules/RTC/TPCUtils.spec.js

@@ -35,7 +35,7 @@ describe('TPCUtils', () => {

        it('sort ssrcs associated with all FID ssrc-groups', () => {
            const pc = new MockPeerConnection();
-            const tpcUtils = new TPCUtils(pc);
+            const tpcUtils = new TPCUtils(pc, { });


If you use a default, we don't need to pass the weird empty object.

You can remove this one now.

Looks like I missed this one, I removed it from all other places.

saghul · 2024-11-19T16:12:45Z

modules/RTC/TraceablePeerConnection.js

+
+        try {
+            transceiver = this.peerconnection.addTransceiver(mediaStreamTrack, transceiverInit);
+        } catch (error) {


Suggestion: make this function async so you don't need to do this, execptions will reject the promsie already.

I just moved the code around and tried not to make any additional changes. This one however is simple enough that I don't mind doing it.

saghul · 2024-11-19T16:14:29Z

modules/RTC/TraceablePeerConnection.js

+    // peerconnection on chrome in unified-plan. It is ok to ignore and not report the error here since the
+    // action that triggers 'addTrack' (like unmute) will also configure the encodings and set bitrates after that.
+    if (!parameters?.encodings?.length) {
+        return Promise.resolve();


Ditto, why not make it async?

saghul · 2024-11-19T16:16:03Z

modules/RTC/TraceablePeerConnection.js

+    let mungedDescription = description;
+
+    this.trace('RTCSessionDescription::preTransform', dumpSDP(description));
+    mungedDescription = this.tpcUtils.mungeOpus(description);


Each of these operations re-parses the SDP. Since we are refactoring, doesn't it make sense to parse it in this functiom, and pass the transform object along, so each of the tpcUtils functions can apply transformations, and then after all of them, re-generate the SDP?

Yep makes sense.
I attempted all transformations in one utils function and that made it look rather unwieldy. Also some of the transormations will be gone soon so I thought its easier to just remove indivdual transformations then.
I see the benefit in passing just the transform object along though, will update it.

…g an instance of RTCSessionDescription. The constructor for RTCSessionDescription has been deprecated - https://developer.mozilla.org/en-US/docs/Web/API/RTCSessionDescription/RTCSessionDescription.

saghul · 2024-11-25T09:11:05Z

modules/RTC/TPCUtils.js

+     * @param desc A session description object (with 'type' and 'sdp' fields)
+     * @return A session description object with its sdp field modified to contain an inject ssrc-group for simulcast.
+     */
+    injectSsrcGroupForUnifiedSimulcast(desc) {


Why does this have "unified" in the name now?

Forgot to rename the method, will do. Thanks for pointing it out.

saghul · 2024-11-25T09:11:28Z

modules/RTC/TPCUtils.js

@@ -574,6 +575,57 @@ export class TPCUtils {
        } ];
    }

+    /**
+     * Injects a 'SIM' ssrc-group line for simulcast into the given session description object to make Jicofo happy.
+     * This is needed only for Firefox since it does not generate it when simulcast is enabled.


We call this regardless of the browser, is it really only required for FF?

Yes, this is only needed for Firefox now since it doesn't produce a SIM group but we also need to verify it on other browsers just in case as this would break the functionality otherwise. This code adds the SIM group only if its missing from the SDP.

saghul · 2024-11-25T09:17:34Z

modules/RTC/TPCUtils.js

     * @returns {Array}
     */
-    getConfiguredVideoCodecs(description) {
-        const currentSdp = description?.sdp ?? this.pc.localDescription?.sdp;
+    getConfiguredVideoCodecs(sdp) {


I suggest you break this function into 2:

getConfiguredVideoCodecs(sdp) { } _getConfiguredVideoCodecsImpl(parsedSdp) { }

With the former calling the latter, after parsing the SDP.

This will allow you to call the 2nd one in the munge code and doing it all in 1 pass!

saghul · 2024-11-25T09:19:09Z

modules/RTC/TPCUtils.spec.js

@@ -35,7 +35,7 @@ describe('TPCUtils', () => {

        it('sort ssrcs associated with all FID ssrc-groups', () => {
            const pc = new MockPeerConnection();
-            const tpcUtils = new TPCUtils(pc);
+            const tpcUtils = new TPCUtils(pc, { });


You can remove this one now.

saghul · 2024-11-25T09:23:56Z

modules/RTC/TraceablePeerConnection.js

+ */
+TraceablePeerConnection.prototype._mungeDescription = function(description) {
+    this.trace('RTCSessionDescription::preTransform', dumpSDP(description));
+    let mungedSdp = transform.parse(description.sdp);


Why are we not using SdpTransformWrap here?

There is no added benefit of using SdpTransformWrap here since util functions operate on the parsedSDP directly. The helper functions that SdpTransformWrap provides are all related to SSRCs and only used by RTXModifier class.

saghul

🚀

saghul · 2024-11-28T08:21:18Z

Pl next time use squash-merge, you can keep the individual commit messages if you want, but this was 1 logical change.

jallamsetty1 marked this pull request as draft November 18, 2024 21:14

jallamsetty1 force-pushed the ref-tpc branch from be70784 to aac1deb Compare November 18, 2024 22:06

ref(TPC) Move all the utility functions to TPCUtils.

7de84c8

jallamsetty1 force-pushed the ref-tpc branch from aac1deb to 7de84c8 Compare November 19, 2024 15:14

jallamsetty1 marked this pull request as ready for review November 19, 2024 15:41

saghul reviewed Nov 19, 2024

View reviewed changes

jallamsetty1 added 4 commits November 21, 2024 11:05

ref(TPC): Convert to async functions.

0bd87d4

ref(TPC) Pass bare objects to methods that take SDP instead of passin…

de4e1a5

…g an instance of RTCSessionDescription. The constructor for RTCSessionDescription has been deprecated - https://developer.mozilla.org/en-US/docs/Web/API/RTCSessionDescription/RTCSessionDescription.

ref(TPCUtils) Make options default to {}.

e667010

ref(TPC) Pass the transform object along for SDP munging.

2cf5416

jallamsetty1 requested a review from saghul November 25, 2024 03:28

saghul reviewed Nov 25, 2024

View reviewed changes

ref(TPCUtils) Fix JSDoc and change impl of a method.

fe8b885

saghul approved these changes Nov 25, 2024

View reviewed changes

jallamsetty1 merged commit 39c4422 into master Nov 25, 2024
2 checks passed

jallamsetty1 deleted the ref-tpc branch November 25, 2024 16:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ref(TPC) Move all the utility functions to TPCUtils. #2599

ref(TPC) Move all the utility functions to TPCUtils. #2599

jallamsetty1 commented Nov 18, 2024 •

edited

Loading

saghul left a comment

saghul Nov 19, 2024

saghul Nov 19, 2024

jallamsetty1 Nov 20, 2024

saghul Nov 19, 2024

saghul Nov 19, 2024

jallamsetty1 Nov 20, 2024

saghul Nov 21, 2024

saghul Nov 19, 2024

saghul Nov 19, 2024

saghul Nov 25, 2024

jallamsetty1 Nov 25, 2024

saghul Nov 19, 2024

jallamsetty1 Nov 20, 2024

saghul Nov 19, 2024

saghul Nov 19, 2024

jallamsetty1 Nov 20, 2024

saghul Nov 25, 2024

jallamsetty1 Nov 25, 2024

saghul Nov 25, 2024

jallamsetty1 Nov 25, 2024

saghul Nov 25, 2024

saghul Nov 25, 2024

saghul Nov 25, 2024

jallamsetty1 Nov 25, 2024

saghul left a comment

saghul commented Nov 28, 2024

ref(TPC) Move all the utility functions to TPCUtils. #2599

ref(TPC) Move all the utility functions to TPCUtils. #2599

Conversation

jallamsetty1 commented Nov 18, 2024 • edited Loading

saghul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saghul left a comment

Choose a reason for hiding this comment

saghul commented Nov 28, 2024

jallamsetty1 commented Nov 18, 2024 •

edited

Loading