Fix sharing own screen when other peer is sharing the screen #1571
Conversation
danxuliu
added
2. developing
bug
help wanted
regression
feature: WebRTC 🚡
|
Strange, sharing the own screen when somebody else is already sharing works in older versions. We have Nextcloud 13 + Talk 3.2.x running on one of the servers here and I recently had a meeting with multiple people sharing their screen. |
I think I do :-) There are two problems here: first, in webrtc.js, the same change that I made when the internal signaling server is in use needs to be done too when the standalone signaling server is in use (please refer to the commit message for the full explanation). I will take care of that. However, that is not enough to fix the issue. The second problem is in the standalone signaling server itself; the mentioned change only works if the signaling server sends the However... I am not a fan of changing the standalone signaling server just to accommodate SimpleWebRTC, specially now that we are going to remove it. I will see if it can be fixed without touching the standalone signaling server. |
The explanation below only applies when no MCU is used; the issue happened both with and without MCU, but the code paths were not modified when a MCU is used, so the bug is still present in that case. When the local screen is shared a new screen peer is created for each of the remote peers. However, when a screen offer is received a new screen peer is created too; as screen sharing is unidirectional there are separate Peer objects for the screen sent to and the screen received from a remote peer. As both Peer objects are for the same remote peer they have the same session ID; the only difference between them is in the "sharemyscreen" property, which is "true" for the screen sent to the remote peer. Due to all this, when sharing the local screen it is not enough to check if there is already a screen Peer object with the remote peer id to prevent creating another one, as that screen Peer object can represent a screen received from the remote peer; it is necessary to ensure that the Peer object represents the screen sent to the remote peer using the "sharemyscreen" property. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
When the MCU is used and the local screen is shared the screen is not directly sent to the remote peers, but to the MCU, which then relays it to the desired remote peers. Thus, when the local screen should be shared to a remote peer the MCU has to be notified to send the screen offer to the remote peer. Then, when the remote peer receives the screen offer, it creates a new Peer object for the received screen. The MCU was notified to send the screen offer only if there was no screen Peer object for the remote peer. Due to this, when a remote peer shared the screen with the local peer and the local peer then shared the screen with that remote peer no screen offer was sent, because there was already a screen Peer object for that remote peer. To fix this, now the screen offer is sent whenever the local screen is shared with a remote peer. Note that this should not cause duplicate offers, as the offer will be sent only when the screen sharing starts or when a new user joins the call. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
As screen sharing is unidirectional there are separate Peer objects for the screen sent to and the screen received from a remote Peer. When the MCU is used there is a single Peer object for the screen sent to (as it is sent to the MCU), but when there is no MCU there is one Peer object for each remote peer that the screen is sent to. In that later case, the Peer objects for the screen sent to and the screen received from a remote peer have both the same session ID; the only difference between them is in the "sharemyscreen" property, which is "true" for the screen sent to the remote peer. Due to this, when looking for stale peers the Peer object for the local screen needs to be ignored; otherwise when the screen offer from the remote peer is received the Peer object for the local screen would be seen as stale and removed due to having the same ID and type. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
SimpleWebRTC expects that screen offers from remote peers include the "broadcaster" property set to the ID of the peer; this is used to set the "sharemyscreen" property, which is used to differentiate Peer objects for sent and received screens. Screen offers received from the MCU do not include the "broadcaster" property, so SimpleWebRTC mark the Peer objects created from those offers as local screens. This causes, for example, that the remote screen peers are ended when the local screen is stopped. Due to this, now the "broadcaster" property is added to the screen offers received from the MCU before they are processed by SimpleWebRTC. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
danxuliu
force-pushed
the
fix-sharing-own-screen-when-other-peer-is-sharing-the-screen
branch
from
0b7cb32 to
83f4c9f
Compare
|
OK, I fixed the issue with and without MCU, and without touching the standalone signaling server ;-) In the end exactly the same fix used when there is no MCU was not really needed with the MCU; the offer can be sent without checking the existing peers, as the local screen is sent only to the MCU, and thus there is not going to be a Peer object for the local screen sent to the remote peer. Regarding the This is now ready for review both with and without MCU; besides the original testing steps please test too unsharing the screens. |
|
/backport to stable15 |
nickvergessen
deleted the
fix-sharing-own-screen-when-other-peer-is-sharing-the-screen
branch
|
The backport to stable15 failed. Please do this backport manually. |
This pull request fixes a regression introduced when the support for the MCU was added (Talk 4.0).
I have fixed the issue when there is no MCU, but it seems that it is also present when a MCU is used (at least, based on a test I made in our company instance it is present in latest release, so I guess it is present in master too).
@fancycode @Ivansss Could you fix this when a MCU is being used too? I am not confident enough to mess with the MCU related code without having a local MCU to test against it ;-) Thanks a lot :-)
One thing to keep in mind: when a MCU is used the
Peerobject that represents the screen received from the remote peer hassharemyscreenset totrue, just like thePeerobject that represents the local screen sent to the remote peer. Therefore, it is not possible to differentiate between screen peers (which is how I fixed it when no MCU is used).This seems to happen because the screen offer received when the MCU is used does not contain the
broadcasterproperty, which is used by SimpleWebRTC to set thesharemyscreenproperty;I guess that it would be a matter of adding it to theNo, that does not work; see comments below.sendOfferdata, but I do not really know.How to test:
Result with this pull request:
Both users can see both screens (by clicking on the screen icon in their video/avatar).
Result without this pull request:
User A can not see the screen of user B; user B can see both screens.