Skip to content
This repository has been archived by the owner on Aug 16, 2021. It is now read-only.

fix: voice chat undefined setremotedescription #1887

Closed
wants to merge 7 commits into from

Conversation

pravusjif
Copy link
Member

@pravusjif pravusjif commented Dec 28, 2020

@pravusjif pravusjif added the wip Work in progress label Dec 28, 2020
@github-actions
Copy link

@pravusjif pravusjif added do not merge Don't merge this PR! ready for review and removed wip Work in progress labels Dec 28, 2020
@pravusjif pravusjif marked this pull request as ready for review December 28, 2020 19:14
@pablitar
Copy link
Contributor

This is unrelated to the issue.

See the stack trace:
image

I think the issue is related to the use of an unsupported browser or something similar. In any case, in theory there is code that should reduce the noise generated by the issue. See here:

https://github.com/decentraland/catalyst/pull/244/files

And peer library has in theory been updated, so it shouldn't show up anymore:

3dec2aa

See for instance that the stack trace in rollbar is not the same as the latest code. For some reason, those who are experiencing the issue seem to not be running the latest code.

Copy link
Member

@menduz menduz left a comment

Choose a reason for hiding this comment

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

I'd recommend to close this PR since it adds no functionality

let retryNumber = currentRetryNumber
// Apparently the RTCPeerConnection can be 'undefined' until the user accepts/blocks the microphone usage in the browser (https://github.com/webRTC-io/webrtc.io-client/issues/30#issuecomment-15991572)
// @ts-ignore
if (!dst || !src) {
Copy link
Member

Choose a reason for hiding this comment

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

This condition will always return false.

There is NO WAY in that calling new X returns other than an object. It was a misinterpretation of the comment in the link.

That if should be removed

@@ -317,20 +316,26 @@ export class VoiceCommunicator {

const offer = await src.createOffer()

await src.setLocalDescription(offer)
await src.setLocalDescription(offer).catch((err) => {
return Promise.reject(err)
Copy link
Member

Choose a reason for hiding this comment

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

The following two snippets are exactly the same:

await src.setLocalDescription(offer)
await src.setLocalDescription(offer).catch((err) => {
  return Promise.reject(err)
})

We should remove the .catch to avoid confussions

await dst.setRemoteDescription(offer)
await dst.setRemoteDescription(offer).catch((err) => {
return Promise.reject(err)
})
Copy link
Member

Choose a reason for hiding this comment

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

same here

await src.setRemoteDescription(answer)
await src.setRemoteDescription(answer).catch((err) => {
return Promise.reject(err)
})
Copy link
Member

Choose a reason for hiding this comment

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

and here too

@menduz menduz closed this Jan 4, 2021
@agusaldasoro agusaldasoro deleted the fix/VoiceChatUndefinedSetRemoteDescription branch January 25, 2021 13:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants