ref(SDPDiffer) Convert to ES6 class. #2577
Conversation
jallamsetty1
force-pushed
the
ref-sdp
branch
4 times, most recently
from
d4e058e
to
f01bc53
Compare
Make it work directly with unified plan SDP that has multiple m-lines and add more unit tests.
Without having to run it through the SDPInterop.toPlanB cycle.
| * | ||
| * @type {Map<string, TPCSSRCInfo>} | ||
| */ | ||
| this._ssrcMap = null; |
There was a problem hiding this comment.
Maybe rename this to localSsrcMap ?
| || typeof desc.sdp !== 'string') { | ||
| logger.warn('An empty description was passed as an argument'); | ||
|
|
||
| if (!localSDP || typeof localSDP !== 'string') { |
There was a problem hiding this comment.
Is this an error? Should a warning be logged?
There was a problem hiding this comment.
This technically wouldn't happen since the browser will always produce an SDP for createOffer/createAnswer. The call itself will fail and user kicked out when that happens so I thought it's ok not to display the error here.
There was a problem hiding this comment.
In that case, let's throw an exception, as if it was an assertion. If the invariant is ever broken, better do it as loud as possible.
| groupsMap.set(primarySSRC, []); | ||
| } | ||
| groupsMap.get(primarySSRC).push(group); | ||
| if (Array.isArray(ssrcGroups)) { |
There was a problem hiding this comment.
We check ssrcs with if (ssrcs?.length) { but here we do it differently. I feel we should check consistently since they are both arrays.
There was a problem hiding this comment.
An artifact of c+p of the old code, fixed it.
modules/sdp/SDP.js
Outdated
|
|
||
| ssrcGroupLines.forEach(line => { | ||
| const idx = line.indexOf(' '); | ||
| const semantics = line.substr(0, idx).substr(13); |
There was a problem hiding this comment.
Where do these 13 and 14 numbers come from?
There was a problem hiding this comment.
length of 'a=ssrc-group:'
There was a problem hiding this comment.
I wish there was a better way...
There was a problem hiding this comment.
I added a helper function in SDPUtil for this.
modules/sdp/SDPDiffer.js
Outdated
| // recurse into the nested arrays | ||
| if (!array1[i].equals(array2[i])) { | ||
| return false; | ||
| if (othersSsrcs.length && !isEqual([ ...mySsrcs ].sort(), [ ...othersSsrcs ].sort())) { |
There was a problem hiding this comment.
Do Object.keys(mySource.ssrcs).sort() so you don't need to create a copy.
Remove LOCAL_TRACK_SSRC_UPDATED event as the application ignores the event and no additional action needs to be taken when that event is fired.
| || typeof desc.sdp !== 'string') { | ||
| logger.warn('An empty description was passed as an argument'); | ||
|
|
||
| if (!localSDP || typeof localSDP !== 'string') { |
There was a problem hiding this comment.
In that case, let's throw an exception, as if it was an assertion. If the invariant is ever broken, better do it as loud as possible.
modules/sdp/LocalSdpMunger.js
Outdated
|
|
||
| if (ssrcMap.size) { | ||
| for (const [ id, trackSsrcs ] of ssrcMap.entries()) { | ||
| if (isEqual([ ...sources ].sort(), [ ...trackSsrcs.ssrcs ].sort())) { |
There was a problem hiding this comment.
Aha. In that case it might be better to have a const sortedSources = sources.slice().sort(); to avoid a copy on each loop iteration, which would be a bunch of them in case of many participants, no?
modules/sdp/SDP.js
Outdated
|
|
||
| ssrcGroupLines.forEach(line => { | ||
| const idx = line.indexOf(' '); | ||
| const semantics = line.substr(0, idx).substr(13); |
There was a problem hiding this comment.
I wish there was a better way...
modules/sdp/LocalSdpMunger.js
Outdated
|
|
||
| if (ssrcMap.size) { | ||
| for (const [ id, trackSsrcs ] of ssrcMap.entries()) { | ||
| const sortedSources = sources.slice().sort(); |
There was a problem hiding this comment.
Put this outside of the loop since it doesn't change within.
Make it work directly with unified-plan SDP and add more unit tests.
Please look at the individual commits.