-
Notifications
You must be signed in to change notification settings - Fork 403
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
Better customisation of call params #459
base: master
Are you sure you want to change the base?
Better customisation of call params #459
Conversation
# Conflicts: # android/src/main/java/com/twiliorn/library/CustomTwilioVideoView.java # android/src/main/java/com/twiliorn/library/CustomTwilioVideoViewManager.java # src/TwilioVideo.android.js # src/TwilioVideo.ios.js
I haven't had a chance to test this yet, has anyone else? @umarniz do you feel that this is in a good enough place to merge or would you prefer more testing to take place before proceeding? Also n.b. there appear to be some merge conflicts that need to be resolved now. |
Also huge thanks for putting this together. It's an awesome effort and addresses one of the most persistent feature requests we've had! |
Any plan on moving this forward? Happy to provide support if it would help |
@mrtkrcm the code in this PR is fully functional and well tested but in a different branch that we use internally in our product. I had tested this PR briefly on iOS and Android and vaguely remember seeing some an issue once or twice with the video not starting on the 2nd call in one of the platforms. I am not certain about this and hence was hesitant to make sure we don't break master. It would help greatly if you can test this PR on iOS & Android and ideally test it by doing multiple calls to each other. If you do encounter the video issue, its most likely explanation behind that is we have some modification in our code for start call & end call which make it optional to turn on/off video. I might have broken something while resolving merge conflicts. |
Any plans to merge this PR? It's a really good work for improve the quality of video calls |
Looks like this PR has quite a few merge conflicts. Is anyone willing to take up the mantle of resolving the conflicts so we can get this merged? |
@slycoder I can, i'll really appreciate to contribute here |
7c3cd06
to
bce7e78
Compare
This PR adds support for Bandwidth Profile API, Encoding Parameters and Track Priorities.
The PR allows you to configure all options required for Develop High Quality Video Applications with Twilio: https://www.twilio.com/docs/video/tutorials/developing-high-quality-video-applications
Major changes:
Connect
between Android and iOSThis PR introduces a breaking change from the previous version i.e: it removes support for passing
enableRemoteAudio
on connect for Android.This functionality was only available on Android and I see there are better ways to implement this feature and the current implementation is already inconsistent as its not supported on iOS.
Outside of this, the overall bandwidth profile API and the new features that I have implemented here are quite well tested into our own product but the implementation in here is not fully tested.
I did a quick test to see if everything was running in this branch due to resolving merge conflicts and I can see everything compiles and runs okay.
It is recommended to test this deeper if possible as I see our own branch and this upstream has some merge conflicts (specially as we have our own implementation of disabling video on connect and this repo has its own implementation of the same logic).