Skip to content

Conversation

@danxuliu
Copy link
Member

@danxuliu danxuliu commented Nov 27, 2020

webrtc.sendToAll only sends the signaling message to participants for which there is a Peer object. Due to this now it also emits and event that is handled by the WebRTC wrapper to send the message too to the rest of participants in the call that do not have a Peer object.

Sending signaling messages to participants without peers is currently needed for "raise hand" messages, and it will be needed for mute/unmute messages once the data channels are removed.

This pull request also fixes receiving raised hand events from participants without a Peer object, which could be useful in the webcast mode.

Besides the fix also some unneeded code was removed. Extracted to its own pull request.

Pending:

  • Fix signaling message sent also to own peer when the HPB is used (previously existing bug, not introduced here, but I have just noticed it)? The signaling server seems to just drop the message, so it is not a big trouble, it would be just for tidyness - Turns out that the code is uglier with that change, so let's keep it as is.
  • Set a proper roomType in signaling messages to participants without peers
  • Ensure that mobile apps will not choke if a mute/unmute message is received from a participant without audio and video
  • Rebase on master and check that everything works as expected with SIP
  • Test thoroughly

How to test (scenario 1)

  • Setup the HPB
  • As user A, start a call with audio and/or video
  • As user B, in a private window open the conversation
  • Open Talk settings and disable both audio and video
  • Join the call
  • As user A, raise the hand

Result with this pull request

User B sees that user A raised the hand

Result without this pull request

User B does not see that user A raised the hand

How to test (scenario 2)

  • Setup the HPB
  • As user A, start a call with audio and/or video
  • As user B, in a private window open the conversation
  • Open Talk settings and disable both audio and video
  • Join the call
  • Raise the hand

Result with this pull request

User A sees that user B raised the hand

Result without this pull request

User A does not see that user B raised the hand

How to test (scenario 3)

  • Do not setup the HPB
  • As user A, open Talk settings and disable both audio and video
  • Start a call with audio and/or video
  • As user B, in a private window open the conversation
  • Open Talk settings and disable both audio and video
  • Join the call
  • Raise the hand

Result with this pull request

User A sees that user B raised the hand

Result without this pull request

User A does not see that user B raised the hand

@nickvergessen
Copy link
Member

Master is 22 now

@danxuliu danxuliu force-pushed the fix-not-sending-signaling-messages-to-participants-without-a-peer-object branch from fb7409b to 1e23cc4 Compare May 28, 2021 17:24
@danxuliu danxuliu requested a review from PVince81 May 28, 2021 17:31
danxuliu added 2 commits June 7, 2021 10:22
"webrtc.sendToAll" only sends the signaling message to participants for
which there is a Peer object. Due to this now it also emits an event
that is handled by the WebRTC wrapper to send the message too to the
rest of participants in the call that do not have a Peer object.

Signed-off-by: Daniel Calviño Sánchez <[email protected]>
Participants can raise their hand even if there is no Peer object for
them (that is, if they are not sending audio nor video). Therefore the
received "raiseHand" signaling message needs to be handled in a general
way outside of the Peer object.

Signed-off-by: Daniel Calviño Sánchez <[email protected]>
@nickvergessen
Copy link
Member

  • How to test (scenario 1)
  • How to test (scenario 2)
  • How to test (scenario 3)

@nickvergessen
Copy link
Member

Rebase was clean, I will push it so we see that tests pass

@nickvergessen nickvergessen force-pushed the fix-not-sending-signaling-messages-to-participants-without-a-peer-object branch from 1e23cc4 to 5d51bd2 Compare June 7, 2021 09:15
@nickvergessen
Copy link
Member

  • Ensure that mobile apps will not choke if a mute/unmute message is received from a participant without audio and video

At least android worked, iOS doesn't work with master atm.

  • Rebase on master …

done

  • … check that everything works as expected with SIP

SIP doesn't work with master atm.

@nickvergessen
Copy link
Member

I would say let's merge and then recheck with iOS and SIP once they are there.

@danxuliu
Copy link
Member Author

danxuliu commented Jun 7, 2021

I would say let's merge and then recheck with iOS and SIP once they are there.

👍 Thanks for the testing!

@nickvergessen nickvergessen merged commit 851152a into master Jun 7, 2021
@nickvergessen nickvergessen deleted the fix-not-sending-signaling-messages-to-participants-without-a-peer-object branch June 7, 2021 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants