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

Insert simulcast recv line for m-lines associated with all local sources on Firefox #2391

Merged
merged 2 commits into from
Nov 13, 2023
Merged

Insert simulcast recv line for m-lines associated with all local sources on Firefox #2391

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
004caf0
fix(TPCUtils) Insert simulcast recv line for all senders on FF.
jallamsetty1 Nov 13, 2023
ff9c34c
ref(TPCUtils) Alpha sort the methods.
jallamsetty1 Nov 13, 2023
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
58 changes: 26 additions & 32 deletions modules/RTC/TPCUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,45 +342,21 @@ export class TPCUtils {
}

/**
* Takes in a *unified plan* offer and inserts the appropriate
* parameters for adding simulcast receive support.
* Takes in a *unified plan* offer and inserts the appropriate parameters for adding simulcast receive support.
* @param {Object} desc - A session description object
* @param {String} desc.type - the type (offer/answer)
* @param {String} desc.sdp - the sdp content
*
* @return {Object} A session description (same format as above) object
* with its sdp field modified to advertise simulcast receive support
* @return {Object} A session description (same format as above) object with its sdp field modified to advertise
* simulcast receive support.
*/
insertUnifiedPlanSimulcastReceive(desc) {
// a=simulcast line is not needed on browsers where we SDP munging is used for enabling on simulcast.
// Remove this check when the client switches to RID/MID based simulcast on all browsers.
if (browser.usesSdpMungingForSimulcast()) {
return desc;
}
const sdp = transform.parse(desc.sdp);
const idx = sdp.media.findIndex(mline => mline.type === MediaType.VIDEO);

if (sdp.media[idx].rids && (sdp.media[idx].simulcast_03 || sdp.media[idx].simulcast)) {
// Make sure we don't have the simulcast recv line on video descriptions other than
// the first video description.
sdp.media.forEach((mline, i) => {
if (mline.type === MediaType.VIDEO && i !== idx) {
sdp.media[i].rids = undefined;
sdp.media[i].simulcast = undefined;

// eslint-disable-next-line camelcase
sdp.media[i].simulcast_03 = undefined;
}
});

return new RTCSessionDescription({
type: desc.type,
sdp: transform.write(sdp)
});
}

// In order of highest to lowest spatial quality
sdp.media[idx].rids = [
const rids = [
{
id: SIM_LAYER_1_RID,
direction: 'recv'
Expand All @@ -401,11 +377,29 @@ export class TPCUtils {
const simulcastLine = browser.isFirefox() && browser.isVersionGreaterThan(71)
? `recv ${SIM_LAYER_RIDS.join(';')}`
: `recv rid=${SIM_LAYER_RIDS.join(';')}`;
const sdp = transform.parse(desc.sdp);
const mLines = sdp.media.filter(m => m.type === MediaType.VIDEO);
const senderMids = Array.from(this.pc._localTrackTransceiverMids.values());
Copy link
Member

Choose a reason for hiding this comment

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

Do we need Array.from? The return value from Map.values should be iterator which also has find method. Wouldn't that work?

Copy link
Member Author

Choose a reason for hiding this comment

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

Technically its not needed, added here for clarity. I don't see any harm in using it though :)

Copy link
Member

Choose a reason for hiding this comment

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

Generally for performance? We are adding a redundant operation.

Also I don't understand the clarity argument. Actually for me it was the opposite I started wondering what would the .values() call return so that we need to convert it to array. Would you explain what the array conversation clarifies in this case?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually that doesn't seem to be true. values() returns a Map iterator which doesn't support find.

Copy link
Member

Choose a reason for hiding this comment

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

I was looking into https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Iterator/find . But just gave it a try and it seems to not work for some reason...


// eslint-disable-next-line camelcase
sdp.media[idx].simulcast_03 = {
value: simulcastLine
};
mLines.forEach((mLine, idx) => {
// Make sure the simulcast recv line is only set on video descriptions that are associated with senders.
if (senderMids.find(sender => mLine.mid.toString() === sender.toString()) || idx === 0) {
if (!mLine.simulcast_03 || !mLine.simulcast) {
mLine.rids = rids;

// eslint-disable-next-line camelcase
mLine.simulcast_03 = {
value: simulcastLine
};
}
} else {
mLine.rids = undefined;
mLine.simulcast = undefined;

// eslint-disable-next-line camelcase
mLine.simulcast_03 = undefined;
}
});

return new RTCSessionDescription({
type: desc.type,
Expand Down