Send publisher offer with join request to accelerate connection#1846
Send publisher offer with join request to accelerate connection#1846cnderrauber wants to merge 3 commits intomainfrom
Conversation
Also create 3 a/v media sections to receive auto subscribed media
|
boks1971
left a comment
boks1971
left a comment
There was a problem hiding this comment.
LGTM! But would be good to get another pair of 👀 . Lukas is out this week. So maybe wait till merging this?
src/api/SignalClient.ts
Outdated
| 'join_request', | ||
| btoa(String.fromCharCode(...wrappedBytes)) | ||
| .replace(/\+/g, '-') | ||
| .replace(/\//g, '_'), |
There was a problem hiding this comment.
Browser complains invalid code in btoa(...), copliot said those char code are invalid in url encoding and fix it with the replace, need @lukasIO to confirm it is ok.
There was a problem hiding this comment.
we shouldn't need these replacers if we keep the TextDecoder path from before.
fromCharCode interprets as UTF-16, but we probably want UTF-8.
There was a problem hiding this comment.
Get error Failed to execute 'btoa' on 'Window': The string to be encoded contains characters outside of the Latin1 range. if use btoa(new TextDecoder('utf-8').decode(wrappedBytes))
There was a problem hiding this comment.
Ah, I see. Sorry for the confusion. the second example here https://developer.mozilla.org/en-US/docs/Web/API/Window/btoa#unicode_strings uses TextEncoder first and then String.fromCodePoint before passing it to btoa
There was a problem hiding this comment.
My comments were wrong, the underlying issue is that btoa creates plain base64 but we need base64url encoding.
For this to work reliably we need to add a third replacer to strip the padding at the end, see https://stackoverflow.com/a/78178053
There was a problem hiding this comment.
the server complains cannot decode base64 ... again if I use the third replace: bytesToBase64(wrappedBytes).replace(/\+/g, '-').replace(/\//g, '_').replace(/=+$/, '')
There was a problem hiding this comment.
ok, not sure why this is, but at this point I'm out of ideas and am inclined to just accept this as is as long as it works.
| this.pendingInitialOffer = { sdp: offer.sdp, type: offer.type }; | ||
| const sdpParsed = parse(offer.sdp ?? ''); | ||
| sdpParsed.media.forEach((media) => { | ||
| ensureIPAddrMatchVersion(media); |
There was a problem hiding this comment.
This is done when handling answer too. Is it needed in both places?
There was a problem hiding this comment.
To make sure server receives same offer as client
There was a problem hiding this comment.
Got it. Thank you. I thought pendingInitialOffer generated here will already have the changes in-place and when it is read in answer path, it should have the changes.
There was a problem hiding this comment.
The setMungedSDP in answer handle has a fallback path that use original sdp again if munged sdp is invalid(return error in setLocalDescription), ensureIPAddrMatchVersion shoud not make sdp invalid in theory but still keep the original sdp here to get a consistent behavior.
|
|
||
| if (!supportOptionalDatachannel(joinResponse.serverInfo?.protocol)) { | ||
| this.createDataChannels(); | ||
| } |
There was a problem hiding this comment.
Has this moved to some other place or not needed any more?
There was a problem hiding this comment.
This is imported when I implement optional datachannel in server side to make sure it will not break function when connect to a old server version. It is not needed now.
size-limit report 📦
|
| } | ||
| const joinRequestBytes = joinRequest.toBinary(); | ||
| const compressedBytes = await (async () => { | ||
| const stream = new CompressionStream('gzip'); |
There was a problem hiding this comment.
We'll need feature gating here to ensure this path is only used for browsers that support compression stream: https://developer.mozilla.org/en-US/docs/Web/API/Compression_Streams_API
3 participants
Also create 3 a/v media sections to receive auto subscribed media