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

Cancelling screen-sharing doesn't propagate to the receiver #98

Open
SimonBrandner opened this issue Jan 11, 2023 · 12 comments
Open

Cancelling screen-sharing doesn't propagate to the receiver #98

SimonBrandner opened this issue Jan 11, 2023 · 12 comments
Assignees
Labels
T-Defect Something isn't working

Comments

@SimonBrandner
Copy link
Contributor

  1. A starts screen-sharing
  2. B sees screen-sharing
  3. A stops screen-sharing
  4. A performs a successful re-negotiation with the SFU
  5. B sees no changes - no messages from the SFU
@SimonBrandner SimonBrandner added the T-Defect Something isn't working label Jan 11, 2023
@SimonBrandner
Copy link
Contributor Author

Somehow I don't see any remote track closed in the logs

@SimonBrandner
Copy link
Contributor Author

But I do see No RTP on subscription for...

@daniel-abramov daniel-abramov self-assigned this Jan 11, 2023
@SimonBrandner SimonBrandner self-assigned this Jan 27, 2023
@SimonBrandner
Copy link
Contributor Author

It looks like the negotiating is going completely awry here... We send an offer with the transceiver being recvonly and the SFU also responds with recvonly (i.e. sendonly from our POV) and it somehow gets accepted and so the SFU still thinks it will be able to receive new packets and so we get no remote track closed

@daniel-abramov
Copy link
Contributor

Interesting! Can/should we do something regarding that on the SFU side? Currently, we don't munge the SDP in any way, we pass them directly to Pion.

@SimonBrandner
Copy link
Contributor Author

Possibly we can play with the transceivers though experience tells me the negotiation code probably isn't completely wrong and it's us screwing up somehow

@SimonBrandner
Copy link
Contributor Author

FWIW, the cause seems to be pion/webrtc#2226

@daniel-abramov
Copy link
Contributor

Interesting.

OT: It seems like the same issue is present in the webrtc-rs, the author of the GitHub issue is the one who also contributes/maintains webrtc-rs at the moment.

@dbkr dbkr assigned dbkr and unassigned daniel-abramov and SimonBrandner Jan 31, 2023
@dbkr
Copy link
Member

dbkr commented Feb 2, 2023

Yeah, I think this is basically correct — as per that bug, pion basically just doesn't support tracks (or at least receivers) being paused and then reactivated. There is a further bug whereby the receiver is only stopped if the transceiver is set to inactive and not when it's set to recvonly (ie. sendonly from the SFU's PoV). In other words:

  • The reason the screenshare doesn't go away is that EC currently makes screenshare transceivers sendrecv and never use the receiver, so when it deactivates them they go to send/recvonly instead of inactive, therefore pion doesn't stop the track, therefore the SFU never removes the screenshare from the metadata.
  • However, if EC uses a sendonly transceiver, pion does stop the track when the screenshare ends, but it can't be reactivated because pion just doesn't know how to do that. It would need to either treat the track as appearing again (another onTrack event) or introduce the idea of the track being muted & unmuted like in the WebRTC API.

For now, I suggest we work around this by not re-using transceivers with the SFU.

An interesting question is what livekit does here.

@daniel-abramov
Copy link
Contributor

For now, I suggest we work around this by not re-using transceivers with the SFU.

So that's basically making EC use sendonly transceiver as you described in the second point, right? (i.e. each time we start the screen sharing again, that would result in a new onTrack etc, which is not a problem per se despite being a bit inefficient).

@dbkr
Copy link
Member

dbkr commented Feb 2, 2023

Not really - using sendonly vs sendrecv makes pion close the track, so fixes the screenshare getting stuck. Re-using transceivers is then necessary because pion can't restart the track once it's closed it. But yes, the effect is a new onTrakc every time screen sharing starts.

@dbkr
Copy link
Member

dbkr commented Feb 7, 2023

Worked around client side in matrix-org/matrix-js-sdk#3120. @EnricoSchw was looking at the causes of this in pion.

@dbkr dbkr removed their assignment Feb 7, 2023
@EnricoSchw
Copy link
Contributor

The main Issue that the track IDs can change or assigned to another transceiver we will solve with an custom media identifier. The resolution of this ticket depends on these adjustments element-hq/element-call#943

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Defect Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants