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

Handle reversed streams from signaling #189

Conversation

pnts-se-whereby
Copy link
Contributor

@pnts-se-whereby pnts-se-whereby commented Jan 15, 2024

Tested like:

  1. Checkout video-conference pontusfagerstrom/cob-305-error-establishing-peer-connection-in-p2p-on-active branch and run it in local-dev
  2. Run storybook against p2p-room with camera and screenshare running
    image
  3. Join with another client and it should fail. No camera in either direction and no screenshare for the joining peer. You should also notice two ready_to_receive_offer been sent from the joining peer.
    image
  4. Checkout this browser-sdk branch and it should work. Also, only one ready_to_receive_offer should be sent on join

@pnts-se-whereby pnts-se-whereby requested a review from thyal January 15, 2024 10:04
Copy link
Member

@thyal thyal left a comment

Choose a reason for hiding this comment

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

Seems to work well, I haven't seen any issues when testing locally. There's some e2e tests failing, and some are related to screenshare. Could you look in to those? Let me know if you need a second pair of eyes.

@pnts-se-whereby
Copy link
Contributor Author

Seems to work well, I haven't seen any issues when testing locally. There's some e2e tests failing, and some are related to screenshare. Could you look in to those? Let me know if you need a second pair of eyes.

Thank you, I'll take a look.

@thyal
Copy link
Member

thyal commented Jan 22, 2024

@pnts-se-whereby when thinking about it, the tests are flaky sometimes. It's worth trying a re-run first

@pnts-se-whereby
Copy link
Contributor Author

@pnts-se-whereby when thinking about it, the tests are flaky sometimes. It's worth trying a re-run first

The tests were actually flaky, but my fix was erroneous.
I've pushed a new one and updated the test instructions.
Please have a look 🙏

…rstrom/cob-305-error-establishing-peer-connection-in-p2p-on-active
Copy link
Member

@thyal thyal left a comment

Choose a reason for hiding this comment

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

🍰 Nice. Can verify that the bug happens on main and that this fixes it

@pnts-se-whereby pnts-se-whereby merged commit 364dbf3 into main Jan 29, 2024
2 checks passed
@pnts-se-whereby pnts-se-whereby deleted the pontusfagerstrom/cob-305-error-establishing-peer-connection-in-p2p-on-active branch January 29, 2024 08:41
jamesdools-whereby pushed a commit that referenced this pull request Feb 6, 2024
Handle reversed stream order in room_joined response when joining a P2P room with active screen share
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.

2 participants