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

Add video codec and max bitrate settings #10

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

ZZYSonny
Copy link
Contributor

This pull request introduces two new settings to allow users to enhance their video quality.

By default, Chrome utilizes VP8 for WebRTC video encoding and imposes a 3Mbps limit on video bitrate. These limits might be too conservative, especially in the case of 1-1. Users may choose to use a more recent video encoder (if supported) / higher bitrate (if network allows) for much better video quality.

@ZZYSonny
Copy link
Contributor Author

ZZYSonny commented Sep 29, 2023

Hi. Thank you for making this feature greater by allowing changing codec and bitrate on the go.

One thing I would suggest is to order the codec items from high quality (but slower / less support) to lower quality (faster). Even better if it is in the same (quality) ordering as bitrate option.

In my opinion AV1 > VP9 > VP8 ≈ H264 in terms of quality.

// Update codec in real time
if (config.videoSenderCodec !== 'default') {
const senderCapabilities = RTCRtpSender.getCapabilities(videoSender.track.kind);
const selectedCodec = senderCapabilities.codecs.find((codec) =>
Copy link
Contributor Author

@ZZYSonny ZZYSonny Sep 29, 2023

Choose a reason for hiding this comment

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

I am not sure if Array.find is enough. Codec may have other different property (like profiles). In the worst case, you might select a codec whose profile is not supported on the receiver, but the receiver may support other profiles.

The old way of setting codec (setCodecPreferences, although must before createAnswer/createOffer) accepts an array so there might be some negotiation going on.

@ZZYSonny
Copy link
Contributor Author

I am trying to see if I can apply the new codec (without a refresh) by restarting the peerConnection.

@ZZYSonny
Copy link
Contributor Author

ZZYSonny commented Sep 29, 2023

Codec change should work without refreshing the window now. Feel free to review, experiment and adapt the code. (As a reminder you may need to refresh chrome://webrtc-internals to see the codec changes).

Also, it is probably useful to try refreshCodec after sender.replaceTrack() (See https://developer.mozilla.org/en-US/docs/Web/API/RTCRtpSender/replaceTrack#usage_notes)

@ZZYSonny
Copy link
Contributor Author

ZZYSonny commented Oct 3, 2023

Hi. It is been a while since I first submitted this PR and made the enhancement. Please let me know how you feel about this PR. If you find no value in it, I can close the PR for you.

@miroslavpejic85
Copy link
Owner

Hey @ZZYSonny, It's great to hear from you, and I appreciate your patience in waiting for a response. I'll hope to take a look at your PR this weekend and provide you with feedback as soon as possible.

From what I understand, you have tested the code, and it works fine, right? Have you noticed any particular differences when changing the codec and Mbps?

Thank you for your contribution, and we'll discuss its merits and any potential changes if necessary. Feel free to reach out to me anytime, and sorry for the delay, I'm swamped with other commitments. 😊

@ZZYSonny
Copy link
Contributor Author

ZZYSonny commented Oct 3, 2023

Thank you for your update and wish you all the best in your life.

I've deployed mirotalk with this PR. I typically use VP9 (unfortuantely chrome android does not support AV1 yet) with 8Mbps. The video quality improvement is substantial, especially when you move around. With low bitrate the video is just full of pixels.

Thanks again for maintaining this wonderful project and hope to hearing from you soon.

@miroslavpejic85
Copy link
Owner

You're very welcome! I appreciate your kind words and support. It's great to hear that you've deployed Mirotalk with the PR and are enjoying improved video quality with VP9. The advancement of video codecs like VP9 and AV1 is indeed crucial for enhancing video quality, especially in low bandwidth scenarios.

I've made some adjustments to ensure compatibility across various browsers and devices. Please feel free to thoroughly test it, and let me know if you encounter any issues or anomalies during your testing.

Best wishes to you too, and I'm here whenever you need assistance or have more updates to share!

@ZZYSonny
Copy link
Contributor Author

ZZYSonny commented Oct 7, 2023

Hi. It is good to see the compatibility adjustments and the new error catchings. I've refined the error catching and restoring in the last few commits. Now if you select a codec which is not supported on the other sides (which won't really happen because we don't have a AV1 standalone choice.), the error will be caught as well and you will reset to the default codec. Also a few other error catching refining for bitrate. Hope you enjoy the new commits.

@miroslavpejic85
Copy link
Owner

miroslavpejic85 commented Oct 7, 2023

Hey! That sounds like fantastic progress on the compatibility adjustments and error catching in your recent commits. It's great to hear that you've gone the extra mile to ensure smooth operation even in scenarios that are unlikely to happen. The attention to detail with error catching for bitrates is also a valuable addition.

Your commitment to improving the project is truly appreciated, and it's clear you're dedicated to delivering a robust and user-friendly experience. Keep up the excellent work, and I'm sure users will indeed enjoy these new commits. If you have any more updates in the future, feel free to reach me out.

PS: Could you please align this branch with the main branch of the Mirotalk C2C repository and If you think something can still be improved feel free to send commits.

Happy coding and thank you so much for you valuable time and have a great weekend! 😊🚀

@ZZYSonny
Copy link
Contributor Author

Hello. I think this PR is in a good shape to merge. Please consider doing a final review and merging the PR. Thank you so much for your invaluable input and wish that the codec option and the project will help more and more people.

One thing I would suggest though is to make the setting saving per-room. If someone talks to a person with high bandwidth in a meeting room, but wants to talk to another person in another room with lower bandwidth. It would probably save some hassle. Maybe the resolution and fps can be saved as well. But personally I feel these improvements are outside this PR.

@miroslavpejic85
Copy link
Owner

Hello, I hope you understand that I've been quite occupied with other commitments. I do find the concept of managing codecs and Mbps intriguing. However, I've come across situations where infinite loops can be triggered, especially when one of the parties involved doesn't support a specific codec or Mbps. Additionally, I think it would be beneficial to conduct thorough testing across various browsers to create a comprehensive test matrix. I promise to return to this pull request when my schedule permits. In the meantime, please feel free to make improvements, conduct further testing, and address any issues, including the one I mentioned earlier. Thank you for Your time and for the dedication to this project are greatly appreciated!

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.

None yet

2 participants