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

Add synchronization around local track access to JitsiConference #2442

Closed
wants to merge 7 commits into from

Conversation

paweldomas
Copy link
Member

@paweldomas paweldomas commented Jan 31, 2024

Adds an async queue which will execute all operations which
rely on or modify the local tracks state. The operations will
execute in sequential order asynchronously.

Before this change, the following code will yield unexpected results:

const track1 = new JitsiLocalTrack(CAMERA);
const track2 = new JitsiLocalTrack(CAMERA);
const track3 = new JitsiLocalTrack(CAMERA);

conference.addTrack(track1);
conference.replaceTrack(track1, track2);
conference.replaceTrack(track2, track3);

It would have worked if each of the operations would wait for the
previous one to complete:

await conference.addTrack(track1);
await conference.replaceTrack(track1, track2);
await conference.replaceTrack(track2, track3);

However, this adds unnecessary complexity to the consuming applications,
which is especially true if the code that manages the tracks is spread
across different modules (need a central place that tracks promises from
all track operations). Having the queue on the JitsiConference level, rather
than above (jitsi-meet for example), makes the client code simpler.

This problem is not only about making sure that each track operation waits
for the previous one, there's a problem which can not be fixed outside
lib-jitsi-meet. When a new JingleSession is started, it needs to pick up
the local tracks to be used by the session. If it happens that either P2P
or JVB session is processing track operation while another session is
being started, it will use invalid/out of sync local tracks state obtained
from this.rtc.localTracks (see unit test section which covers those
cases). It's possible this could be fixed by adding new code that picks up
the tracks again later, but it's not a universal solution. Having an async
queue to synchronize async operations that modify/access local tracks state
will be useful for new code which needs that in the future.

Adds an async queue which will execute all operations which
rely on or modify the local tracks state. The operations will
execute in sequential order asynchronously.

Before this change, the following code will yield unexpected results:

const track1 = new JitsiLocalTrack(CAMERA);
const track2 = new JitsiLocalTrack(CAMERA);
const track3 = new JitsiLocalTrack(CAMERA);

conference.addTrack(track1);
conference.replaceTrack(track1, track2);
conference.replaceTrack(track2, track3);

It would have worked if each of the operations would wait for the
previous one to complete:

await conference.addTrack(track1);
await conference.replaceTrack(track1, track2);
await conference.replaceTrack(track2, track3);

However, this adds unnecessary complexity to the consuming applications,
which is especially true if the code that manages the tracks is spread
across different modules (need a central place that tracks promises from
all track operations). Having the queue on the JitsiConference level, rather
than above (jitsi-meet for example), makes the client code simpler.

This problem is not only about making sure that each track operation waits
for the previous one, there's a problem which can not be fixed outside
lib-jitsi-meet. When a new JingleSession is started, it needs to pick up
the local tracks to be used by the session. If it happens that either P2P
or JVB session is processing track operation while another session is
being started, it will use invalid/out of sync local tracks state obtained
from this.rtc.localTracks (see unit test section which covers those
cases). It's possible this could be fixed by adding new code that picks up
the tracks again later, but it's not a universal solution. Having an async
queue to synchronize async operations that modify/access local tracks state
will be useful for new code which needs that in the future.
is sending multi stream is now always true

I'm assuming code[1] should not be necessary because of this comment:

https://github.com/jitsi/lib-jitsi-meet/blob/d53d0106793729cc736ef248c8bb18dafc964b6e/modules/xmpp/JingleSessionPC.js#L2161

[1]:
if (track.getVideoType() === VideoType.DESKTOP) {
  this._updateRoomPresence(this.getActiveMediaSession());
}
@paweldomas paweldomas marked this pull request as draft January 31, 2024 20:23
const addTrackPromises = [];

this.p2pJingleSession && addTrackPromises.push(this.p2pJingleSession.addTracks([ track ]));
this.jvbJingleSession && addTrackPromises.push(this.jvbJingleSession.addTracks([ track ]));
Copy link
Member

Choose a reason for hiding this comment

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

As discussed, this is going to be problematic. In both jvb and p2p case, the first track needs to be added to be replaced on the RTCRtpSender associated with the first recvonly transceiver. When a secondary track is added, for jvb, a new transceiver is created and for p2p, a new transceiver is created only when there is no recvonly one. JingleSessionPC::addTracks needs to be modified to take this into account. Right now, it is designed to be used for secondary tracks only and therefore creates a new transceiver for jvb always.

Copy link
Member Author

Choose a reason for hiding this comment

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

By primary and secondary do you mean for each media type, for example:

  • primary track will be the 1st video track added to the connection
  • secondary track will be the 2nd video track added to the connection?

If there are two tracks, one audio and one video then there is one primary video track and one primary audio track?

also adds unit test case which enforces 1 audio track
JingleSessionPC queue is paused until the first offer/answer.

If JitsiConference has a track operation on it's queue,
before session offer/answer, they will never execute,
because the track operation blocks starting the JingleSession's
queue.

It's okay to skip track operations on JingleSessionPC, because
it will pick up the local track on the offer/answer.
@hristoterezov
Copy link
Member

hristoterezov commented Feb 5, 2024

@paweldomas Wouldn't all this be handled by the modification queue in JingleSessionPC even before? What will actually cause the unexpected results?

Is the issue that we do bunch of other things outside of the modification queue that we later rely on and they will be out of sync?

@paweldomas
Copy link
Member Author

paweldomas commented Feb 6, 2024

@hristoterezov the issue is that JitsiConference's track operations(add/replace/remove) as well as the JVB/P2P JingleSession initialization (accept,create offer) rely on this.rtc.localTracks state. You can see that variables get updated only once the operations scheduled on JingleSessionPC resolve. So if you perform either a track operation, or try to initialize new JingleSessionPC, without explicitly waiting for previous track operations to be done, then out of sync state will be taken into consideration.

Note that currently only application that uses lib-jitsi-meet is able to wait for other track operation to resolve, which is the main design flaw. JitsiConference by design can't do it's job correctly. To give an example:

  1. JitsiConference starts and a JVB JingleSessionPC is created. It accept the offer and so on. There are no local tracks.
  2. User calls addTrack(camera1);, this queues addTracks in JingleSessionPC and this operation is async and takes time. Let's say 200ms.
  3. 100ms later, another user joined and started a P2P connection. JitsiConference will not have camera1 in this.rtc.localTracks yet and the P2P session will start without including it in the local tracks of JingleSessionPC.
  4. Another 100ms later, JVB JingleSessionPC is done with addTrack and now we end up in a state where JVB JingleSessionPC has camera1, but P2P JingleSessionPC does not.

The idea in this PR for a fix is to use the modification queue to schedule P2P initialization to be executed after the first addTrack. So that it will have camera1 in this.rtc.localTracks when creating/answering the offer. It makes JitsiConference aware of all currently scheduled track operations.

@paweldomas
Copy link
Member Author

I think this is too big of a change to land, let's try to go with this more ad-hoc fix: #2469

@paweldomas paweldomas closed this Mar 12, 2024
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