-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Insert simulcast recv line for m-lines associated with all local sources on Firefox #2391
Conversation
The simulcast receive lines needs to be inserted in the receive desc for all the m-line associated with local tracks, currently its being done only for the first video source.
modules/RTC/TPCUtils.js
Outdated
@@ -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()); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
Currently, it is being done only for the first video source.