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

feat: add disableTwcc helper function #19

merged 2 commits into from
Dec 20, 2023

Conversation

bxu2
Copy link
Contributor

@bxu2 bxu2 commented Dec 18, 2023

This pr adds a new function disableTwcc to and also modifies the existing function disableRtcpFbValue in munge.ts.
With this change we'll be able to disable audio twcc, which impacts probing and bandwidth estimation according to our test.

src/munge.ts Outdated
@@ -40,6 +41,15 @@ export function disableRemb(sdp: Sdp) {
disableRtcpFbValue(sdp, 'goog-remb');
}

/**
* 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.

src/munge.ts Outdated
@@ -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.

* @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.

): 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.

Copy link
Collaborator

@bbaldino bbaldino left a comment

Choose a reason for hiding this comment

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

lgtm other than the stuff bryce called out

Copy link
Collaborator

@brycetham brycetham left a comment

Choose a reason for hiding this comment

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

Looks good! Left a minor style comment but it's not a big deal.

return [...av.codecs.values()].some((ci: CodecInfo) => {
return ci.feedback.includes(rtcpFbValue);
});
});
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.

@bxu2 bxu2 merged commit c9ec114 into main Dec 20, 2023
1 check passed
@bxu2 bxu2 deleted the bxu2/disableTwcc branch December 20, 2023 01:16
bbaldino pushed a commit that referenced this pull request Dec 20, 2023
# [1.6.0](v1.5.0...v1.6.0) (2023-12-20)

### Features

* add disableTwcc helper function ([#19](#19)) ([c9ec114](c9ec114))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants