Open aryavohra opened 4 years ago
Yo this is is awesome, I am super busy this week so no promises I can get this in quickly. But this looks awesome at a hight level (will look into code later). Two things I noticed:
I am not a fan of the nicknames I would like to see that removed. If that is being used for some sort of UUID hashing then just assign new users a random string and use that. I want to keep user friction as low as possible.
CSS for incoming video feeds.
This is such awesome work guys, I am so happy to see this! I cant wait to get this in! Great job so far! ๐
Thank you so much for the detailed feedback!! We'll hop on and make these changes ASAP and will get back to you once we're ready for review again.
We added in the nicknames so when using messages with >2 peers, users can keep track of the conversation. We could use friendly uuids instead though, e.g Blue Buffalo (adjective + animal name).
I think not showing nicknames at all will fine in the text chat.
Got it, will remove them. Thanks :)
5 person also barely works on my iPhone 6s, this is just a limitation of peer mesh and the high level of compute required to decode 4 incoming videos. I would like to experiment with dropping the resolution and bitrate of videos as the number of callers increases.
Also, dont worry about captions, I am going to remove them in the future. We will save removing them for a different PR however.
5 person also barely works on my iPhone 6s, this is just a limitation of peer mesh and the high level of compute required to decode 4 incoming videos. I would like to experiment with dropping the resolution and bitrate of videos as the number of callers increases.
We can give this a shot too, adding to todos
I just checked it out, great work! :D
The JavaScript seems to be working fine for the most part, including graceful disconnects, etc. However, the mute button isn't working for some reason.
Also, there are some other issues, especially on mobile. For example:
prompt()
part. I'd recommend assigning each user a random color instead. The color could then be shown as a frame around the user's video and also be shown in the chat :)Really cool of you guy to make this PR though! <3
Love the color idea, adding it in to the todos
Make sure that there are no duplicate colored borders, will most likely have to persist a map of whos color is whos.
Make sure that there are no duplicate colored borders, will most likely have to persist a map of whos color is whos.
Actually, the colors don't have to be the same for each client: they just have to be unique for each client. So, just ID each client and distribute the colors in the frontend ยฏ\_(ใ)_/ยฏ
Edit: wow, reading this again I made no sense at all ๐
What I meant is: Given that on client A, client B's color is green. This doesn't mean that B's color is green for client C as well. Every client assigns a (for them) unique color to all of the other clients, but they don't have to sync the color palette with the others. Does that make more sense?
So we decided to do colors that match across all clients. We're using UUIDs to deterministically generate unique colors so we don't have to send any color data back and forth, check it out!
We're fixing picture-in-picture and we're not sure how to decide which remote peer to show in the pop-out window. We're thinking we could ask the clients which peer they want in picture-in-picture.
Ideally, we'd dynamically change the picture-in-picture video based on who's talking, but I that's better suited for a separate pull request.
We're thinking we could ask the clients which peer they want in picture-in-picture.
Don't ask the user. PiP is not that important.
Just auto-advance to the next client after a fixed amount of time, like 5 seconds :)
For Pip I wonder if there is some way to group all the videos together, I doubt it, but for now asking would be ideal, picking last joined client will also work.
For Pip I wonder if there is some way to group all the videos together, I doubt it, but for now asking would be ideal, picking last joined client will also work.
I disagree about the 'asking the user' part. It's bad UX (just like asking for a nickname) and will cause headaches if the user wants to switch to showing a different peer at some point.
Last joined might work, but isn't ideal nor the expected behaviour. I'd only fall back to this if the auto-advance is too hard to implement.
I disagree about the 'asking the user' part. It's bad UX (just like asking for a nickname) and will cause headaches if the user wants to switch to showing a different peer at some point. Last joined might work, but isn't ideal nor the expected behaviour. I'd only fall back to this if the auto-advance is too hard to implement.
Maybe then we can monitor the audio levels somehow and dynamically change the video input on a special PIP video.
Found a pretty lightweight package for detecting who's speaking โ what do you think? https://github.com/otalk/hark
I see, could be interesting, then the next question is can you hotswap video src in PIP.
I would also focus on fixing the video grid layout and the stability, those are top priority right now. I have been testing groupcalling and having mixed results. Hard to pinpoint what is happening exactly.
I see, could be interesting, then the next question is can you hotswap video src in PIP.
I would also focus on fixing the video grid layout and the stability, those are top priority right now. I have been testing groupcalling and having mixed results. Hard to pinpoint what is happening exactly.
Gotcha, we'll prioritise finishing up the CSS. Could you elaborate a little on what issues you're having with the group call itself โ dropped frames etc? We have only been able to get smooth performance on group sizes of 3 & 4, beyond that we've had trouble too.
Gotcha, we'll prioritise finishing up the CSS. Could you elaborate a little on what issues you're having with the group call itself โ dropped frames etc? We have only been able to get smooth performance on group sizes of 3 & 4, beyond that we've had trouble too.
4 person is about the max I have had success with aswell. The main issues I have experienced are:
Added in the downscaling for n>4 nodes in call โ I set it to downscale to a fixed lower resolution of 360p. Not quite sure how we should rescale when the number of peers drops back down.
- When people disconnect for whatever reason it doesnt seem to reconnect / rejoin until a manual refresh happens
I'll add in auto refresh again to solve this, we removed it but forgot to add it back lol.
- When a new caller joins sometimes they will only connect to some of the people in the call but not all of them.
Haven't seen this yet myself, what are the steps to recreate?
Edit: figured out how to recreate issue 2. Seems like the peer discovery is unstable with larger rooms, working on fixing this and will hopefully be able to push a fix alongside all the CSS fixes today.
BTW, we were wondering if you guys had test checklists / unit tests for before pushing to master, we want to catch more errors before we push them. Thanks
For now there are no automatic tests in place. We'll try to manually test as much as possible ยฏ\_(ใ)_/ยฏ
Any ideas how we could implement automatic testing?
Couple of thoughts:
This is great work guys, I will make sure you are recognized in the Readme!
Thanks Ian, we appreciate it! We've added a few changes
peerconnection
smoothly. Achieved by making more intelligent use of the signalling server; for example, we use the signalling server to indicate whether a peer disconnection is intermittent (i.e. will be restored momentarily) or permanent. More generally, you shouldn't see any random failures like there used to be.The big one left is CSS. After that, we should be ready for more rigorous testing, and eventually merging it! Downscaling hurt stability, but we'll take a look at adding it back in.
BTW, we'll try coming up with a checklist and sharing that when it's done too.
Just tested it out with a group of 4 and while I could see everyone in the call, none of the other participants were able to see each person. I think the connecting stability still needs some refinement, perhaps it was because we all joined at the same time. Also downscaling is really needed for calls of more than two.
I thought downscaling to 360p was already enabled? @ianramzy are you talking about lowering it even more?
I thought downscaling to 360p was already enabled? @ianramzy are you talking about lowering it even more?
It was disabled for now I believe.
We're not sure how to replicate the issue. We tested it with five people on separate devices across networks and it worked fine. Could you please give more details?
It might be worth looking at deployment. We used Heroku to deploy @ meet.questo.ai โย feel free to test it out with that and see if the issue persists.
@ianramzy how about we deploy this as a 'beta' feature, with some sort of disclaimer? I know you're not going to like the idea, but given that this is a feature that is hard to test and requires a lot of testing in general, we could add in some kind of feedback option? A simple star rating after the call ends, or a link to a short survey?
By the way, we'd be happy to schedule a call or something so we can hammer out the last few fixes and integration together :)
I think @ianramzy is pretty busy as always, but I'd also be down for a 'proof of concept' group call!
I'd say if @ianramzy doesn't respond soon, you should probably just customize your fork and offer it as an alternative to this project :)
I'd hate to see your work be in vain :/
yeah it is a shame.. that's probably what we'll do.
We've added support for peer-to-peer group video calls for 4 peers, as requested in the linked issue. This is implemented by using multiple RTCPeerConnections per VideoChat client.
The group call actually works, and is done! We are currently just making some small fixes / improvements.
@ianramzy we'd be happy to help load test this and figure out what an appropriate upper bound for group call size should be, although we bet 4 is a good limit.
CSS fixes
Logic fixes
Testing
By: @taixhi @khushjammu @aryavohra