search

signalwire / signalwire-js

MIT License
18 stars 15 forks source link

don't request incompatible userMedia #1118

Closed jpsantosbh closed 1 month ago

jpsantosbh commented 2 months ago

Description

This an optimized implementation to avoid the onnegaotiationneeded infite loop.

When the remote SDP don't have a media channel, but the client add a media track to the RTCPeerConnection the onnegaotiationneeded is fired but it's will result in the same local and remote SDPs creating a infinite loop.

To avoid that the SDK was skiping to add any incompatible media track. BUt it was open the media device anyway.

This implementation takes into account the remote SDP channels when creating the request media constraints. Avoiding the SDK to request/open any incompabible media.

Type of change

Code snippets

In case of new feature or breaking changes, please include code snippets.

changeset-bot[bot] commented 2 months ago

⚠️ No Changeset found

Latest commit: 595ec4dc93a5e386f08356e65a824bdf3a349da4

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

iAmmar7 commented 2 months ago

Can you check the CI, please? @jpsantosbh

iAmmar7 commented 2 months ago

Could you please highlight how we can test this feature manually? Preferably with both CF and Video SDK, considering the code that has been changed is a part of both of the SDKs. @jpsantosbh

I tried the following steps for the CF SDK:

  1. Celler: Call the subscriber with the Video muted as well as the negotiateVideo flag set to false. It only asks for microphone permission.
  2. Callee: Incoming call accepted with the Video muted. It asks for both camera and microphone permissions.

On the callee side, I can see the remoteSDP with only m=audio line, but it still requests the user's camera device permission.

jpsantosbh commented 2 months ago

Could you please highlight how we can test this feature manually? Preferably with both CF and Video SDK, considering the code that has been changed is a part of both of the SDKs. @jpsantosbh

I tried the following steps for the CF SDK:

  1. Celler: Call the subscriber with the Video muted as well as the negotiateVideo flag set to false. It only asks for microphone permission.
  2. Callee: Incoming call accepted with the Video muted. It asks for both camera and microphone permissions.

On the callee side, I can see the remoteSDP with only m=audio line, but it still requests the user's camera device permission.

Your test steps are correct; only the expectations are not. This isn't about permission but the resulting streams in the call. So what needs to be verified are:

iAmmar7 commented 2 months ago
  • Is the browser or OS indicating that a camera device is on?

Should it be on or off when the caller dials a call with video and negotiateVideo set to false? @jpsantosbh

jpsantosbh commented 1 month ago

The camera led should be off, if negotiateVideo is false.

iAmmar7 commented 1 month ago

The camera led should be off, if negotiateVideo is false.

That's what I have been trying to say that it is not off.

You can try reproducing it by simply dialing a call to a subscriber and choosing the video to be false. Are you not facing this issue? For me, the callee side always asks for Camera permission.