search

IEvangelist / IEvangelist.VideoChat

Imagine two Twilio SDKs, ASP.NET Core/C#, Angular/TypeScript, SignalR, etc... Yeah, amazing!
http://bit.ly/video-chat-tutorial
MIT License
67 stars 59 forks source link

Work around WebKit bug 179363. #17

Closed manjeshbhargav closed 4 years ago

manjeshbhargav commented 4 years ago

Hi @IEvangelist ,

I'm an engineer working on the Twilio Video JavaScript SDK. A customer of ours is building a Video Chat App based on your ASP.NET Angular App. They ran into an issue where iOS Safari Participants were not heard by others in a Room.

The reason for this is due to this WebKit bug which mutes previously acquired tracks when getUserMedia is called again. Since you have 2 instances of the CameraComponent, and both of them are acquiring tracks, what ends up happening is that the muted audio track gets passed in ConnectOptions while joining a Room.

Since I'm not familiar with Angular, I decided to keep this PR simple by just removing one of the CameraComponents (the one within the HomeComponent). With this change, I was able to confirm that iOS Safari Participants can be heard by others in a Room.

I'm sure you have a better way of working around the webkit bug, but I wanted to open this PR to bring this issue to your attention. Please feel free to incorporate the changes in this PR however you see fit.

Thanks,

Manjesh Malavalli

IEvangelist commented 4 years ago

Hi @manjeshbhargav - thank you for posting this issue. I'm curious, why not have a more conditional approach? If this is only affecting one browser, and that browser has had a well-known bug in it - since 2017, that kind of seems like something we should cater to.

Wouldn't it make more sense to not capture the audio track when doing the preview? I think that is probably a cleaner approach and I'm hopeful that you might be able to update this PR to leave the camera preview intact, but when doing the preview flow -- do not initialize the tracks with audio.

https://github.com/IEvangelist/IEvangelist.VideoChat/blob/master/ClientApp/src/app/camera/camera.component.ts#L71-L82

Does that make sense?

manjeshbhargav commented 4 years ago

@IEvangelist ,

Sure, that would work. Let me update the PR.

IEvangelist commented 4 years ago

@IEvangelist ,

Sure, that would work. Let me update the PR.

Awesome, thanks again @manjeshbhargav. Feel free to convert this pull request to a "draft" until you're ready for a review.

manjeshbhargav commented 4 years ago

@IEvangelist ,

I've made the changes we discussed earlier. Please take another look when you have some time.

munesh13490 commented 4 years ago

This code not gonna work. You replaced LocalTrack with LocalVideoTrack but the Connect method requires LocalTrack object. Or if Twilio updated Connect method also.