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.

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

feat: add disableTwcc helper function #19

Merged
merged 2 commits into from
Dec 20, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
1 change: 1 addition & 0 deletions cspell.json
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@
"trackingid",
"transcoding",
"transpiled",
"twcc",
"typedoc",
"ufrag",
"ulpfec",
Expand Down
74 changes: 73 additions & 1 deletion src/munge.spec.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
import * as fs from 'fs';
import { AvMediaDescription, CodecInfo, Sdp } from './model';
import { removeCodec, retainCandidates, retainCodecs } from './munge';
import {
removeCodec,
retainCandidates,
retainCodecs,
disableRtcpFbValue,
disableTwcc,
} from './munge';
import { parse } from './parser';

/**
Expand Down Expand Up @@ -31,6 +37,29 @@ const validateOfferCodecs = (offer: Sdp): boolean => {
return true;
};

/**
* Check that the sdp offer contains rtcpFbValue or not.
*
* @param offer - The {@link Sdp} or {@link AvMediaDescription} to validate.
* @param rtcpFbValue - The rtcp-fb value to check.
* @returns True if the offer contains rtcp-fb value.
*/
const checkOfferContainsRtcpFeedbacks = (
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: plural of "feedback" is still "feedback". This function should be called checkOfferContainsRtcpFeedback.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

udpated.

offer: Sdp | AvMediaDescription,
rtcpFbValue: string
): boolean => {
let bContains = false;
const mediaDescriptions = offer instanceof Sdp ? offer.avMedia : [offer];
mediaDescriptions.forEach((av: AvMediaDescription) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It doesn't matter here since it's a spec file so you don't need to change it, but a minor optimization would be to use every instead of forEach, since every would break out of the loop as soon as it finds a value that fails the condition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated to use some() here.

av.codecs.forEach((ci: CodecInfo) => {
if (ci.feedback.includes(rtcpFbValue)) {
bContains = true;
}
});
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you omit the curly braces, you don't need to use return here. So this can be:

  return mediaDescriptions.some((av: AvMediaDescription) =>
    [...av.codecs.values()].some((ci: CodecInfo) => ci.feedback.includes(rtcpFbValue))
  );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to do this as well but I'll then get a eslint warning saying -
Array.prototype.some() expects a return value from arrow function.eslintarray-callback-return
So as discussed, I'll just keep the return here.

return bContains;
};

describe('munging', () => {
describe('removeCodec', () => {
it('should remove codecs correctly when passing in an SDP', () => {
Expand Down Expand Up @@ -119,4 +148,47 @@ describe('munging', () => {
});
});
});

describe('disableRtcpFbValue', () => {
it('should remove rtcp feedback correctly when passing in an SDP', () => {
expect.hasAssertions();
const offer = fs.readFileSync('./src/sdp-corpus/offer_with_rtcp_feedback.sdp', 'utf-8');
const parsed = parse(offer);

disableRtcpFbValue(parsed, 'transport-cc');
expect(checkOfferContainsRtcpFeedbacks(parsed, 'transport-cc')).toBe(false);
expect(checkOfferContainsRtcpFeedbacks(parsed, 'goog-remb')).toBe(true);
});
it('should remove rtcp feedback correctly when passing in an AvMediaDescription', () => {
expect.hasAssertions();
const offer = fs.readFileSync('./src/sdp-corpus/offer_with_rtcp_feedback.sdp', 'utf-8');
const parsed = parse(offer);

parsed.avMedia
.filter((av) => av.type === 'audio')
.forEach((av) => {
disableRtcpFbValue(av, 'transport-cc');
expect(checkOfferContainsRtcpFeedbacks(av, 'transport-cc')).toBe(false);
});
expect(checkOfferContainsRtcpFeedbacks(parsed, 'transport-cc')).toBe(true);
expect(checkOfferContainsRtcpFeedbacks(parsed, 'goog-remb')).toBe(true);
});
});

describe('disableTwcc', () => {
it('should remove rtcp feedback correctly when passing in an AvMediaDescription', () => {
expect.hasAssertions();
const offer = fs.readFileSync('./src/sdp-corpus/offer_with_rtcp_feedback.sdp', 'utf-8');
const parsed = parse(offer);

parsed.avMedia
.filter((av) => av.type === 'audio')
.forEach((av) => {
disableTwcc(av);
expect(checkOfferContainsRtcpFeedbacks(av, 'transport-cc')).toBe(false);
});
expect(checkOfferContainsRtcpFeedbacks(parsed, 'transport-cc')).toBe(true);
expect(checkOfferContainsRtcpFeedbacks(parsed, 'goog-remb')).toBe(true);
});
});
});
18 changes: 14 additions & 4 deletions src/munge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,14 @@
import { AvMediaDescription, CodecInfo, MediaDescription, Sdp } from './model';

/**
* Disable an rtcp-fb value from all media blocks in the given SDP.
* Disable an rtcp-fb value from the media blocks in the given SDP or audio/video media description.
*
* @param sdp - The SDP from which to filter an rtcp-fb value.
* @param sdpOrAv - The {@link Sdp} or {@link AvMediaDescription} from which to filter an rtcp-fb value.
* @param rtcpFbValue - The rtcp-fb value to filter.
*/
export function disableRtcpFbValue(sdp: Sdp, rtcpFbValue: string) {
sdp.avMedia.forEach((media: AvMediaDescription) => {
export function disableRtcpFbValue(sdpOrAv: Sdp | AvMediaDescription, rtcpFbValue: string) {
const mediaDescriptions = sdpOrAv instanceof Sdp ? sdpOrAv.avMedia : [sdpOrAv];
mediaDescriptions.forEach((media: AvMediaDescription) => {
media.codecs.forEach((codec: CodecInfo) => {
// eslint-disable-next-line no-param-reassign
codec.feedback = codec.feedback.filter((fb) => fb !== rtcpFbValue);
Expand All @@ -40,6 +41,15 @@ export function disableRemb(sdp: Sdp) {
disableRtcpFbValue(sdp, 'goog-remb');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we update this to take an sdpOrAv while we're at it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

udpated. also add UT for disableRemb.

}

/**
* Disable REMB from all media blocks in the given SDP.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo here (should say TWCC instead of REMB).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

udpated.

*
* @param sdpOrAv - The {@link Sdp} or {@link AvMediaDescription} from which to filter TWCC.
*/
export function disableTwcc(sdpOrAv: Sdp | AvMediaDescription) {
disableRtcpFbValue(sdpOrAv, 'transport-cc');
}

/**
* Remove the codec with the given name (as well as any secondary codecs associated with
* it) from the media blocks in the given SDP or audio/video media description.
Expand Down
Loading