feat: update retain helpers (#21)

* feat: update retain helpers * fix: safely handle missing codec name * fix: pass only AvMediaDescription when retaining codecs --------- Co-authored-by: Bryce Tham <[email protected]>
webex · Jul 30, 2024 · 49b5566 · 49b5566
1 parent ded5c34
commit 49b5566
Show file tree

Hide file tree

Showing 2 changed files with 143 additions and 36 deletions.
diff --git a/src/munge.spec.ts b/src/munge.spec.ts
@@ -1,12 +1,15 @@
 import * as fs from 'fs';
+import { CandidateLine } from 'lines';
 import { AvMediaDescription, CodecInfo, Sdp } from './model';
 import {
+  disableRemb,
+  disableRtcpFbValue,
+  disableTwcc,
   removeCodec,
   retainCandidates,
+  retainCandidatesByTransportType,
   retainCodecs,
-  disableRtcpFbValue,
-  disableRemb,
-  disableTwcc,
+  retainCodecsByCodecName,
 } from './munge';
 import { parse } from './parser';
 
@@ -86,24 +89,41 @@ describe('munging', () => {
       expect(validateOfferCodecs(parsed)).toBe(true);
     });
   });
+
   describe('retainCodecs', () => {
-    it('should retain codecs correctly when passing in an SDP', () => {
+    it('should retain codecs correctly when passing in an AvMediaDescription', () => {
       expect.hasAssertions();
       const offer = fs.readFileSync('./src/sdp-corpus/offer_with_extra_codecs.sdp', 'utf-8');
       const parsed = parse(offer);
 
-      retainCodecs(parsed, ['h264', 'opus']);
+      // eslint-disable-next-line jsdoc/require-jsdoc
+      const predicate = (codecInfo: CodecInfo) =>
+        codecInfo.name === 'h264' || codecInfo.name === 'opus';
+
+      // should return true when some codecs have been filtered out
+      parsed.avMedia.forEach((av) => {
+        expect(retainCodecs(av, predicate)).toBeTruthy();
+      });
       expect(validateOfferCodecs(parsed)).toBe(true);
+      // should return false when no codecs have been filtered out
+      parsed.avMedia.forEach((av) => {
+        expect(retainCodecs(av, predicate)).toBeFalsy();
+      });
     });
-    it('should retain codecs correctly when passing in an AvMediaDescription', () => {
+    it('should retain codecs by name when passing in an AvMediaDescription', () => {
       expect.hasAssertions();
       const offer = fs.readFileSync('./src/sdp-corpus/offer_with_extra_codecs.sdp', 'utf-8');
       const parsed = parse(offer);
 
+      // should return true when some codecs have been filtered out
       parsed.avMedia.forEach((av) => {
-        retainCodecs(av, ['h264', 'opus']);
+        expect(retainCodecsByCodecName(av, ['h264', 'opus'])).toBeTruthy();
       });
       expect(validateOfferCodecs(parsed)).toBe(true);
+      // should return false when no codecs have been filtered out
+      parsed.avMedia.forEach((av) => {
+        expect(retainCodecsByCodecName(av, ['h264', 'opus'])).toBeFalsy();
+      });
     });
   });
 
@@ -113,35 +133,79 @@ describe('munging', () => {
       const offer = fs.readFileSync('./src/sdp-corpus/answer_with_extra_candidates.sdp', 'utf-8');
       const parsed = parse(offer);
 
+      // eslint-disable-next-line jsdoc/require-jsdoc
+      const predicate = (candidate: CandidateLine) =>
+        candidate.transport === 'UDP' || candidate.transport === 'TCP';
+
+      // should return true when some candidates have been filtered out
+      expect(retainCandidates(parsed, predicate)).toBeTruthy();
+      parsed.media.forEach((mline) => {
+        expect(mline.iceInfo.candidates).toHaveLength(4);
+        expect(
+          mline.iceInfo.candidates.every((candidate) =>
+            ['UDP', 'TCP'].includes(candidate.transport)
+          )
+        ).toBeTruthy();
+      });
+      // should return false when no candidates have been filtered out
+      expect(retainCandidates(parsed, predicate)).toBeFalsy();
+    });
+    it('should retain candidates correctly when passing in a MediaDescription', () => {
+      expect.hasAssertions();
+      const offer = fs.readFileSync('./src/sdp-corpus/answer_with_extra_candidates.sdp', 'utf-8');
+      const parsed = parse(offer);
+
+      // eslint-disable-next-line jsdoc/require-jsdoc
+      const predicate = (candidate: CandidateLine) =>
+        candidate.transport === 'UDP' || candidate.transport === 'TCP';
+
+      parsed.media.forEach((media) => {
+        // should return true when some candidates have been filtered out
+        expect(retainCandidates(media, predicate)).toBeTruthy();
+        expect(media.iceInfo.candidates).toHaveLength(4);
+        expect(
+          media.iceInfo.candidates.every((candidate) =>
+            ['UDP', 'TCP'].includes(candidate.transport)
+          )
+        ).toBeTruthy();
+        // should return false when no candidates have been filtered out
+        expect(retainCandidates(media, predicate)).toBeFalsy();
+      });
+    });
+    it('should retain candidates by transport type when passing in an SDP', () => {
+      expect.hasAssertions();
+      const offer = fs.readFileSync('./src/sdp-corpus/answer_with_extra_candidates.sdp', 'utf-8');
+      const parsed = parse(offer);
+
       // should return true when some candidates have been filtered out
-      expect(retainCandidates(parsed, ['udp', 'tcp'])).toBeTruthy();
+      expect(retainCandidatesByTransportType(parsed, ['UDP', 'TCP'])).toBeTruthy();
       parsed.media.forEach((mline) => {
         expect(mline.iceInfo.candidates).toHaveLength(4);
         expect(
           mline.iceInfo.candidates.every((candidate) =>
-            ['udp', 'tcp'].includes(candidate.transport.toLowerCase())
+            ['UDP', 'TCP'].includes(candidate.transport)
           )
         ).toBeTruthy();
       });
       // should return false when no candidates have been filtered out
-      expect(retainCandidates(parsed, ['udp', 'tcp'])).toBeFalsy();
+      expect(retainCandidatesByTransportType(parsed, ['UDP', 'TCP'])).toBeFalsy();
     });
-    it('should retain candidates correctly when passing in an AvMediaDescription', () => {
+    it('should retain candidates by transport type when passing in a MediaDescription', () => {
       expect.hasAssertions();
       const offer = fs.readFileSync('./src/sdp-corpus/answer_with_extra_candidates.sdp', 'utf-8');
       const parsed = parse(offer);
 
       parsed.media.forEach((media) => {
         // should return true when some candidates have been filtered out
-        expect(retainCandidates(media, ['udp', 'tcp'])).toBeTruthy();
+        expect(retainCandidatesByTransportType(media, ['UDP', 'TCP'])).toBeTruthy();
         expect(media.iceInfo.candidates).toHaveLength(4);
         expect(
           media.iceInfo.candidates.every((candidate) =>
-            ['udp', 'tcp'].includes(candidate.transport.toLowerCase())
+            ['UDP', 'TCP'].includes(candidate.transport)
           )
         ).toBeTruthy();
         // should return false when no candidates have been filtered out
-        expect(retainCandidates(media, ['udp', 'tcp'])).toBeFalsy();
+        expect(retainCandidatesByTransportType(media, ['UDP', 'TCP'])).toBeFalsy();
       });
     });
   });

diff --git a/src/munge.ts b/src/munge.ts
@@ -14,6 +14,7 @@
  * limitations under the License.
  */
 
+import { CandidateLine } from './lines';
 import { AvMediaDescription, CodecInfo, MediaDescription, Sdp } from './model';
 
 /**
@@ -68,51 +69,74 @@ export function removeCodec(sdpOrAv: Sdp | AvMediaDescription, codecName: string
 }
 
 /**
- * Retain specific codecs, filtering out unwanted ones from the given SDP or audio/video media
- * description.
+ * Retain specific codecs, filtering out unwanted ones from the given audio/video media description.
+ * The provided predicate should take in a single {@link codecInfo}, and only codecs for which the
+ * predicate returns true will be retained.
  *
- * Note: Done this way because of a feature not implemented in all browsers, currently missing in
- * Firefox. Once that is added we can use `RTPSender.getCapabilities` and filter those to call
- * with `RTCRtpTransceiver.setCodecPreferences` instead of doing this manually.
+ * Note: Done this way because of a feature that was only recently implemented in all browsers,
+ * previously missing in Firefox. You can also use `RTPSender.getCapabilities` and filter those to
+ * call with `RTCRtpTransceiver.setCodecPreferences` instead of doing this manually.
  *
- * @param sdpOrAv - The {@link Sdp} or {@link AvMediaDescription} from which to filter codecs.
- * @param allowedCodecNames - The names of the codecs that should remain in the SDP.
+ * @param av - The {@link AvMediaDescription} from which to filter codecs.
+ * @param predicate - A function used to determine which codecs should be retained.
+ * @returns A boolean that indicates if some codecs have been filtered out.
  */
 export function retainCodecs(
-  sdpOrAv: Sdp | AvMediaDescription,
+  av: AvMediaDescription,
+  predicate: (codecInfo: CodecInfo) => boolean
+): boolean {
+  let filtered = false;
+
+  av.codecs.forEach((codecInfo) => {
+    if (!predicate(codecInfo)) {
+      av.removePt(codecInfo.pt);
+      filtered = true;
+    }
+  });
+
+  return filtered;
+}
+
+/**
+ * Retain specific codecs, filtering out unwanted ones from the given audio/video media description
+ * by codec name.
+ *
+ * @param av - The {@link AvMediaDescription} from which to filter codecs.
+ * @param allowedCodecNames - The names of the codecs that should remain in the media description.
+ * @returns A boolean that indicates if some codecs have been filtered out.
+ */
+export function retainCodecsByCodecName(
+  av: AvMediaDescription,
   allowedCodecNames: Array<string>
-): void {
-  const avMediaDescriptions = sdpOrAv instanceof Sdp ? sdpOrAv.avMedia : [sdpOrAv];
+): boolean {
   const allowedLowerCase = allowedCodecNames.map((s) => s.toLowerCase());
 
-  avMediaDescriptions
-    .map((av) => {
-      return [...av.codecs.values()].map((c) => c.name as string);
-    })
-    .flat()
-    .filter((codecName) => !allowedLowerCase.includes(codecName.toLowerCase()))
-    .forEach((unwantedCodec) => removeCodec(sdpOrAv, unwantedCodec));
+  return retainCodecs(
+    av,
+    (codecInfo) => !!codecInfo.name && allowedLowerCase.includes(codecInfo.name.toLowerCase())
+  );
 }
 
 /**
- * Retain specific candidates, filtering out unwanted ones from the given SDP or media description
- * by transport type.
+ * Retain specific candidates, filtering out unwanted ones from the given SDP or media description.
+ * The provided predicate should take in a single {@link CandidateLine}, and only candidates for
+ * which the predicate returns true will be retained.
  *
  * @param sdpOrMedia - The {@link Sdp} or {@link MediaDescription} from which to filter candidates.
- * @param allowedTransportTypes - The names of the transport types of the candidates that should remain in the SDP.
+ * @param predicate - A function used to determine which candidates should be retained.
  * @returns A boolean that indicates if some candidates have been filtered out.
  */
 export function retainCandidates(
   sdpOrMedia: Sdp | MediaDescription,
-  allowedTransportTypes: Array<string>
+  predicate: (candidate: CandidateLine) => boolean
 ) {
   const mediaDescriptions = sdpOrMedia instanceof Sdp ? sdpOrMedia.media : [sdpOrMedia];
   let filtered = false;
 
   mediaDescriptions.forEach((media) => {
     // eslint-disable-next-line no-param-reassign
     media.iceInfo.candidates = media.iceInfo.candidates.filter((candidate) => {
-      if (allowedTransportTypes.includes(candidate.transport.toLowerCase())) {
+      if (predicate(candidate)) {
         return true;
       }
       filtered = true;
@@ -122,3 +146,22 @@ export function retainCandidates(
 
   return filtered;
 }
+
+/**
+ * Retain specific candidates, filtering out unwanted ones from the given SDP or media description
+ * by transport type.
+ *
+ * @param sdpOrMedia - The {@link Sdp} or {@link MediaDescription} from which to filter candidates.
+ * @param allowedTransportTypes - The names of the transport types of the candidates that should remain in the SDP.
+ * @returns A boolean that indicates if some candidates have been filtered out.
+ */
+export function retainCandidatesByTransportType(
+  sdpOrMedia: Sdp | MediaDescription,
+  allowedTransportTypes: Array<string>
+) {
+  const allowedLowerCase = allowedTransportTypes.map((s) => s.toLowerCase());
+
+  return retainCandidates(sdpOrMedia, (candidate) =>
+    allowedLowerCase.includes(candidate.transport.toLowerCase())
+  );
+}