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)
• 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]

🦋 Changeset detected

Latest commit: 613ce32

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]

Is this required? The expectMCUVisible will wait by itself since it's using the Playwright's waitForSelector API, we shouldn't need the waitForTimeout here.

[jpsantosbh]

It's weird... but it's

[iAmmar7]

You meant expectMCUVisible?

Suggested change

	expectMCUVisibleForAudience(page)
	expectMCUVisible(page)

[jpsantosbh]

No, in this test, we do not expect the local overlay to be visible, but we expect the remote video to be visible.

[iAmmar7]

But why? The local overlay should be visible unless the user passes the applyLocalOverlay flag set to false, no?

[jpsantosbh]

No, the local overlay is only created when you have a video track in the localStream. In this test case, we are recvOnly. There is no video track and no local overlay.

[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.

[iAmmar7]

But why? The local overlay should be visible unless the user passes the applyLocalOverlay flag set to false, no?

[iAmmar7]

I have also shared a video regarding my findings for the sendOnly flag with you.

[iAmmar7]

In which scenario does this require renegotiation? When a user no longer wishes to receive the incoming video/audio but keeps on sending his video/audio?

Regardless of the reason, do you think calling the peer.start is okay here? The peer.start method is meant to be used when starting the peer connection, it sets up the RTC peer connection, adds the transceiver, and starts the negotiation. I don't think we need all these things, we only want to update the current RTC peer with the new device direction, right? Perhaps we can go with a new function that only triggers re-negotiation with updated params without affecting or creating any other peer.

[jpsantosbh]

Yes, This is a case where the existing RTCPeerConnection fires the needed negotiation event.
When we are "removing" one or both media channels we need create a new RTCPeerConnection.

[iAmmar7]

When we are "removing" one or both media channels we need create a new RTCPeerConnection.

Umm.. .why? We can just update the current RTCPeer, can't we? As we are doing already in the case of attaching audio/video via the updateConstraints method.

For eg; if we join the call without video and do not negotiate it, and then later call the updateConstraints method with the video params, it will update the currently running RTC peer and send the verto.modify as well for renegotiation. Will that be helpful?

[jpsantosbh]

In this case RTCPeerConnection doesn't fire negotiationneeded event.

You can test it by commenting out this part. Also, if you replace start() with startNegotiation(), you will see that the result is not what we want.

[iAmmar7]

The RTC peer automatically emits the negotiationneeded event, every time the track is being added via the addTrack method or removed via the removeTrack method.
https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection/removeTrack

The observation you might have seen is because, the updateStream is calling the addTrack when you add the new track but it is not calling the removeTrack when you remove it. We might need to see where should we call this removeTrack method, we can't just call the peer.start method. It is not meant for renegotiation.

[iAmmar7]

There can be many reasons;

• Separation of concerns,
• Possibility of double renegotiation (remember that the updateStream triggers the renegotiation if require),
• Possibility of unnecessary renegotiation (if video changes but the negotiateVideo flag is still the same), it is possible that both the updateStream and the updateConstraints due to this start method begin the renegotiation,
• Unnecessarily restarting the peer connection may cause media interruptions plus resource usage.
• Also, we can analyze each step of the start method and can clearly observe that many things that the start method is doing are not required for the renegotiation.

I shared with you the 4 requirements of this PR, if you could please share with me which one of those requirements is failing, I would be able to debug and might be able to find a different solution.

[jpsantosbh]

This is the test that will fail: https://github.com/signalwire/signalwire-js/pull/1132/files#diff-e1d7afe85b7f7137519ee6170abc4c8cbbd5d3af7e2852666fd8b6f36f3ac103R113

Another option requires more changes on the startSegotiation... Since the code already considers the double negotiation IMO, calling start() is cleaner. But I'll make the change in startNegotiation

[iAmmar7]

The four requirements we discussed were:

1. The user should be able to send and receive his video -> sendrecv
2. The user should be able to only send his video -> sendonly
3. The user should be able to stop his outgoing video -> recvonly
4. The user should be able to stop both incoming and outgoing video -> remove the video track from the peer.

Can you please specify which one of the above requirements falls into this failing test?

[jpsantosbh]

I tested changes to startNegotiation

But even with the correct SDPs, the test fails(we don't receive video)
The only way to do this is to create a new RTPPeerConnection.

The test scenario is that the user connects with audio-only but wants to start receiving video, too.
enableVideo({video: false}) or enableVideo({video: false, sendOnly:false})

https://github.com/signalwire/signalwire-js/pull/1132/files#diff-e1d7afe85b7f7137519ee6170abc4c8cbbd5d3af7e2852666fd8b6f36f3ac103R113

[iAmmar7]

This seems like a new requirement that does not fall under any of the above requirements mentioned. I think I tried to convey this before as well; before starting to implement the solution, we should first clearly write the whole problem. Also, the enableVideo with false video param does not make any sense.

In the meantime, I will try to see how can we solve this recvOnly problem.