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.

Sign up for GitHub

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

Jump to bottom

feat: update retain helpers #21

Merged
merged 3 commits into from
Jul 30, 2024
Merged

feat: update retain helpers #21

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3cb6feb
feat: update retain helpers
Jul 29, 2024
68c8527
fix: safely handle missing codec name
Jul 30, 2024
afe9e69
fix: pass only AvMediaDescription when retaining codecs
Jul 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
114 changes: 102 additions & 12 deletions src/munge.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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';

Expand Down Expand Up @@ -86,24 +89,67 @@ describe('munging', () => {
expect(validateOfferCodecs(parsed)).toBe(true);
});
});

describe('retainCodecs', () => {
it('should retain codecs correctly when passing in an SDP', () => {
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) =>
Copy link

Choose a reason for hiding this comment

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

If this predicate is the same for multiple tests, consider putting it in a describe to avoid duplicating code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's only being used for half of the tests though. To me, it would make sense to do so if most or all of the tests use it (and in the future, if we add more cases to this describe block, that means most of the test cases will not use it).

codecInfo.name === 'h264' || codecInfo.name === 'opus';

// should return true when some codecs have been filtered out
expect(retainCodecs(parsed, predicate)).toBeTruthy();
expect(validateOfferCodecs(parsed)).toBe(true);
// should return false when no codecs have been filtered out
expect(retainCodecs(parsed, predicate)).toBeFalsy();
});
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);

// 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) => {
retainCodecs(av, ['h264', 'opus']);
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 by name when passing in an SDP', () => {
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
expect(retainCodecsByCodecName(parsed, ['h264', 'opus'])).toBeTruthy();
expect(validateOfferCodecs(parsed)).toBe(true);
// should return false when no codecs have been filtered out
expect(retainCodecsByCodecName(parsed, ['h264', 'opus'])).toBeFalsy();
});
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) => {
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();
});
});
});

Expand All @@ -113,35 +159,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();
});
});
});
Expand Down
83 changes: 64 additions & 19 deletions src/munge.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
* limitations under the License.
*/

import { CandidateLine } from './lines';
import { AvMediaDescription, CodecInfo, MediaDescription, Sdp } from './model';

/**
Expand Down Expand Up @@ -69,50 +70,75 @@ export function removeCodec(sdpOrAv: Sdp | AvMediaDescription, codecName: string

/**
* Retain specific codecs, filtering out unwanted ones from the given SDP or audio/video media
* description.
* 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 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,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder, do we ever use this with an entire SDP? Retaining codecs across media types doesn't make much sense, not sure why I supported that (or if it was even me, I guess)

I guess the predicate version it makes a bit more sense since then it can detect audio vs video.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a good question. I took a look at the history for this file. It seems like removeCodec was the original helper function which took in an SDP (and it's still in this file, though I don't see it being used anywhere). Then we added the current retainCodecs and retainCandidates functions... and I'm not sure why I had it accept both? Probably because it made sense to take in a media description for those, but removeCodec took in an SDP, so I compromised and just had all of them take both.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah ok, yeah I think removeCodec made sense with an SDP because it would remove all instances of those codecs from all mlines, so it was easy to throw both video and audio codecs in there. With 'retain' though, for codecs, I'm not sure it makes sense (transport types still accepting an entire sdp does, though)

Copy link

Choose a reason for hiding this comment

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

@bbaldino Will it be logical to keep the API consistent between these functions and more flexible? Or is your vision always to retain codecs only from AvMediaDescription when working with SDP?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's true that the APIs would differ, but I think in this situation it's accurate: the transport information is consistent across all mlines, while codecs differ between audio and video.

allowedCodecNames: Array<string>
): void {
predicate: (codecInfo: CodecInfo) => boolean
): boolean {
const avMediaDescriptions = sdpOrAv instanceof Sdp ? sdpOrAv.avMedia : [sdpOrAv];
let filtered = false;

avMediaDescriptions.forEach((av) => {
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 SDP or audio/video media
* description by codec name.
*
* @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.
* @returns A boolean that indicates if some codecs have been filtered out.
*/
export function retainCodecsByCodecName(
sdpOrAv: Sdp | AvMediaDescription,
allowedCodecNames: Array<string>
): 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(sdpOrAv, (codecInfo) =>
allowedLowerCase.includes(codecInfo.name?.toLowerCase() as string)
Copy link

@antsukanova antsukanova Jul 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

How can you ensure that this function will always have codecInfo.name defined to cast type to string and not have some callback value like "" for example?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah yeah, it was like this before but I probably should be checking to see if it exists first before checking if it's in the list of allowed codec names.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@bbaldino, do you remember why CodecInfo.name is optional? Is it just for convenience when building the CodecInfo, so you don't have to define a name until CodecInfo.addLine() is called?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Discussed offline, but I think your memory is right: it's optional because we fill in that information later. Bryce and I discussed some investigation of using an intermediary type while parsing that could then be finalized into another type which probably models required fields vs optional ones, so we can look into that.

);
}

/**
* 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;
Expand All @@ -122,3 +148,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())
);
}
Loading