Add Public APIs for Media Renegotiation

[jpsantosbh]

Description

Add Public APIs for Media Renegotiation, allowing a developer to change the media options and execute a renegotiation.

Type of change

• [ ] Internal refactoring
• [ ] Bug fix (bugfix - non-breaking)
• [X] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)

Code snippets

await call.renegotiateMedia({video: false, audio: true, negotiateVideo: true,  negotiateAudio: true,});
// or
await call.enableVideo({sendOnly: true});
// or
await call.disableVideo({recvOnly: true});

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

[changeset-bot[bot]]

🦋 Changeset detected

Latest commit: 613ce32db130346c86b6d8aecf28f79340158a51

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

| Name | Type | | ------------------ | ----- | | @signalwire/webrtc | Minor | | @signalwire/js | Patch |

Not sure what this means? Click here to learn what changesets are.

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

[iAmmar7]

Have you tested this with the Video SDK? Asking because the code that has been changed is a part of the both Video and CF SDK.

Can you please integrate the API in the playgrounds so that we can reproduce and see the implementation in action?

[jpsantosbh]

I added the feature to the playground, also the Video SDK e2e are passing.

[iAmmar7]

I added the feature to the playground, also the Video SDK e2e are passing.

Thanks for the playground update.

The Video SDK e2e would pass because it's a new feature. We might want to add a new e2e test for it as well, or we can decide to move functions to the CallFabricRoomSession class to ensure they are only exposed to the CF SDK for now.

[iAmmar7]

A few observations I noticed while testing the feature in the playground;

While joining a room with two members I noticed that ;

1. If one member requests for enableVideo with the sendOnly flag set to true, it does not negotiate the video, it does not even open the user camera. I am not sure if this flag adds any value.
2. If one member enables the video and then later disables the video, I get this error and call disconnects

   {
   "code": "ICE_GATHERING_FAILED",
   "message": "Ice gathering timeout"
   }

Lastly, which is more important, in my opinion, is the usage of the start function from the RTCPeer. Now that I can see its full usage, I am forced to think if this is even required. I thought there was a new server API that we needed to hit but it seems we only need to send the verto.modify which is already being integrated within the BaseConnection methods.

The method named updateConstraints is responsible for upgrading/downgrading the media stream, and updating those streams with a more robust recursive function which includes falling back to the previous constraints if the new constraints fail. It also handles the event listeners such as camera.updated, microphone.updated, etc, and calls the server API verto.modify indirectly.

You can clearly see the difference in your code if you simply comment down one line and add one new line;

async renegotiateMedia(params: UpdateMediaOptions): Promise<void> {
    this.updateMediaOptions(params)
    // await this.peer?.start({ isRenegotiate: true })
    await this.updateConstraints(params) // new line
}

[jpsantosbh]

The Video SDK e2e would pass because it's a new feature. We might want to add a new e2e test for it, or we can move functions to the CallFabricRoomSession class to ensure they are only exposed to the CF SDK for now.

Yes, I meant it's not causing a regression in the Video SDK.

The product requested the feature to be available on Both SDK.

[jpsantosbh]

A few observations I noticed while testing the feature in the playground;

While joining a room with two members I noticed that ;

1. If one member requests for enableVideo with the sendOnly flag set to true, it does not negotiate the video, it does not even open the user camera. I am not sure if this flag adds any value.
2. If one member enables the video and then later disables the video, I get this error and call disconnects

{
    "code": "ICE_GATHERING_FAILED",
    "message": "Ice gathering timeout"
}

Lastly, which is more important, in my opinion, is the usage of the start function from the RTCPeer. Now that I can see its full usage, I am forced to think if this is even required. I thought there was a new server API that we needed to hit but it seems we only need to send the verto.modify which is already being integrated within the BaseConnection methods.

The method named updateConstraints is responsible for upgrading/downgrading the media stream, and updating those streams with a more robust recursive function which includes falling back to the previous constraints if the new constraints fail. It also handles the event listeners such as camera.updated, microphone.updated, etc, and calls the server API verto.modify indirectly.

You can clearly see the difference in your code if you simply comment down one line and add one new line;

async renegotiateMedia(params: UpdateMediaOptions): Promise<void> {
    this.updateMediaOptions(params)
    // await this.peer?.start({ isRenegotiate: true })
    await this.updateConstraints(params) // new line
}

I couldn't reproduce the issue you reported about sendOnly flag: For reference, This is also covered by the 2e2 tests here: https://github.com/signalwire/signalwire-js/pull/1132/files#diff-e1d7afe85b7f7137519ee6170abc4c8cbbd5d3af7e2852666fd8b6f36f3ac103R110

I've noticed the ICE_GATHERING_FAILED issue, too, and I'm investigating, but it doesn't seem to be a client/SDK issue.

Regarding the suggestion of this.updateConstraints(params), it doesn't produce the same results in many cases. But the update events are a good point. I don't want to make changes to updateConstraints, but doing the events makes sense.

[jpsantosbh]

I'm trying some changes to use the updateConstraints

[jpsantosbh]

@iAmmar7 Tests are green with the changes required to use the updateConstraints instead of start (although, in some cases, I have to call start internally). They look safe, IMO. Let me know if you see any potential issues.

Also, disableVideo seems to be working now.