Skip to content
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

Fix loop when an SDP offer is received without video #536

Merged
merged 4 commits into from
Sep 24, 2024

Conversation

giavac
Copy link
Contributor

@giavac giavac commented Sep 17, 2024

When an SDP offer is received without video, the current code still performs a getUserMedia() request with video set to true.

Then the local streams are added to the RTCPeerConnection, before the remote description has been added to it.

This triggers an additional onnegotiationneeded that calls startNegotiating() again, which in turns calls _createAnswer() again, which will trigger new onnegotiationneeded events in a loop that continues for the duration of a session.

I'm not sure about the side effect of this loop, but I'm concerned it may appear as missing ICE candidates in the answer SDP, as if the loop is impacting the correct addition of ICE candidates to the answer SDP that's being built.

To reproduce this and see the loop:

  1. Add a log line inside onnegotiationneeded
  2. Send an audio-only offer to the running client

With the changes in this PR instead the constraint for getUserMedia will depend on the combination of available devices and what was received in the offer. An audio-only offer will only cause a getUserMedia with video false.

jpsantosbh
jpsantosbh previously approved these changes Sep 17, 2024
Copy link
Contributor

@jpsantosbh jpsantosbh left a comment

Choose a reason for hiding this comment

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

I like this solution better than what I did on the V3.
I'm going to port it over to v3 as well.

Copy link
Contributor

@iAmmar7 iAmmar7 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! 🚀 please rebase your branch with the main.

I might be wrong but just thinking out loud; this seems like a design change considering we will no longer negotiate the video/audio in case the offer SDP does not include the video/audio. This might impact the end user, right? and we probably want to release this as a new minor version of the SDK.

packages/common/src/webrtc/Peer.ts Outdated Show resolved Hide resolved
packages/common/src/webrtc/Peer.ts Outdated Show resolved Hide resolved
@giavac
Copy link
Contributor Author

giavac commented Sep 17, 2024

this seems like a design change

Even without these changes the answer SDP from the SDK would not have a video section, if the offer didn't.
What changes is that the local video stream is not created at all.
This would be only visible to the client if getUserMedia triggered a permission request for video, but since video won't be used it is more correct not to try to get those permissions.

Apart from the possibility of regressions that can always happen with changes, it seems to me more like a refactoring or fix, rather than a behaviour change. Let me know what you think.

@iAmmar7
Copy link
Contributor

iAmmar7 commented Sep 17, 2024

Even without these changes the answer SDP from the SDK would not have a video section, if the offer didn't.

Gotcha, then I think it's fine. Thanks for the details. 👍

Copy link
Contributor

@iAmmar7 iAmmar7 left a comment

Choose a reason for hiding this comment

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

🥇

@giavac giavac merged commit 09cfb69 into main Sep 24, 2024
3 checks passed
@giavac giavac deleted the gv/fix_onn_loop_clean branch September 24, 2024 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants