feat(JingleSession) Convert Jingle->SDP directly w/o interop layer. #2590
Conversation
| removeSsrcInfo[idx] += `${ssrcLines.join('\r\n')}\r\n`; | ||
| removeSsrcInfo[idx] += lines; | ||
| } | ||
| for (const [ sourceName, sourceInfo ] of this._remoteSsrcMap.entries()) { |
There was a problem hiding this comment.
You don't need the .entries() to iterate a Map.
| * @property {Array<TPCGroupInfo>} groups - The SSRC groups associated with the track. | ||
| * @property {string} msid - The track's MSID. | ||
| */ | ||
| this._remoteSsrcMap = null; |
There was a problem hiding this comment.
Looking at how we need to handle the code later on, doesn't it make sense to always have this initialized to a an empty Map?
Having its size be 0 would be equivalente to the current null value, but I bet it sill simplify some code and make things easier to work with.
There was a problem hiding this comment.
I have a follow up PR that will get rid of localSSRCs/remoteSSRCs and start using _localSSCMap and _remoteSSRCMap for all other helper functions, I have them initialized to an empty map there. I should have done that here itself :) Modified it.
| @@ -1145,7 +1154,7 @@ TraceablePeerConnection.prototype._processAndExtractSourceInfo = function(localS | |||
| const media = session.media.filter(mline => mline.direction === MediaDirection.SENDONLY | |||
| || mline.direction === MediaDirection.SENDRECV); | |||
|
|
|||
| if (!Array.isArray(media)) { | |||
| if (!media?.length) { | |||
There was a problem hiding this comment.
Is the optional chaining necessary? media will always be an array since filter always returns one.
There was a problem hiding this comment.
Yes its not, force of habit for some reason :)
| */ | ||
| TraceablePeerConnection.prototype.updateRemoteSources = function(sourceMap, isAdd) { | ||
| if (this._remoteSsrcMap) { | ||
| for (const [ sourceName, ssrcInfo ] of sourceMap.entries()) { |
| this._remoteSsrcMap.delete(sourceName); | ||
| } | ||
| } | ||
| } else if (isAdd) { |
There was a problem hiding this comment.
This branch wouldn't be necessary if the Map is always initialized.
modules/xmpp/JingleSessionPC.js
Outdated
|
|
||
| // Update the SSRC map on the peerconnection. | ||
| if (sourceInfo) { | ||
| this.peerconnection._remoteSsrcMap.delete(oldSourceName); |
There was a problem hiding this comment.
Having to dig in the private methods here looks nasty shall this have some API?
There was a problem hiding this comment.
Oops, I added TPC.updateRemoteSources for this exact purpose later but forgot to update it here. Thanks for pointing it out.
Co-authored-by: Saúl Ibarra Corretgé <[email protected]>
Address review comments, fix test faiilures for ssrc-rewriting case and add unit tests.
| * @type {Map<string, TPCSourceInfo>} | ||
| * @property {string} msid - The track's MSID. | ||
| * @property {Array<string>} ssrcs - The SSRCs associated with the track. | ||
| * @property {Array<TPCGroupInfo>} groups - The SSRC groups associated with the track. | ||
| */ | ||
| this._localSsrcMap = null; |
There was a problem hiding this comment.
Any reason not to initialize this to a new map too?
There was a problem hiding this comment.
I have some more work planned around this, i.e., getting rid of localSSRCs and remoteSSRCs in favor of these. Lets restrict this PR to only the SDP conversion, WDYT?
There was a problem hiding this comment.
It just looks odd that one is a map and the other one is null. If it's not a huge change it would be nice to at least have them aligned. No worries if it would break things, we can do it later.
| * Returns the remote sourceInfo for a given source name. | ||
| * | ||
| * @param {string} sourceName - The source name. | ||
| * @returns {TPCSourceInfo} |
There was a problem hiding this comment.
It can also return null, but I think undefined is also good, see below.
| if (type === MediaType.APPLICATION) { | ||
| const newMline = cloneDeep(mLine); | ||
|
|
||
| newMline.mid = newMedia.length.toString(); |
There was a problem hiding this comment.
Is this what we really want? The lengfth as a string? Not its index?
There was a problem hiding this comment.
Yes, that is the requirement. The mid for the new m-line needs to be a string.
There was a problem hiding this comment.
I mean for the MID to the the length. I guess it's the index for the last added element, right?
modules/sdp/SDP.js
Outdated
| newMline.direction = idx ? 'sendonly' : 'sendrecv'; | ||
|
|
||
| // Add the sources and the related FID source group to the new m-line. | ||
| const group = mLine.ssrcGroups?.find(g => g.ssrcs.includes(ssrc.id.toString())); |
There was a problem hiding this comment.
You are converting the ssrc id into a string many times here, calculate it once: const idStr = ssrc.id.toString(); and then compare with it.
Co-authored-by: Saúl Ibarra Corretgé <[email protected]>
No description provided.