-
Notifications
You must be signed in to change notification settings - Fork 134
try setting x-google-start-bitrate for vp9 #820
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
base: main
Are you sure you want to change the base?
try setting x-google-start-bitrate for vp9 #820
Conversation
|
Maybe we should add a simple unit test for |
| pub type OnOfferCreated = Box<dyn FnMut(SessionDescription) + Send + Sync>; | ||
|
|
||
| #[cfg(any(target_os = "windows", target_os = "macos", target_os = "linux"))] | ||
| const DEFAULT_VP9_START_BITRATE_KBPS: u32 = 2500; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should use the currently negotiated bitrate? instead of a fixed amount? the JS logic uses 80% of ultimate bitrate. I think it's ok to go to 100% as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will address the comment
| Ok(answer) | ||
| } | ||
|
|
||
| fn munge_x_google_start_bitrate(sdp: &str, start_bitrate_kbps: u32) -> String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we use a proper sdp parser/rewriter? instead of string changes? https://crates.io/crates/sdp
in case we'd need to do more munging for other things
| let mut offer = self.peer_connection.create_offer(options).await?; | ||
| let start_bitrate_kbps = DEFAULT_VP9_START_BITRATE_KBPS; | ||
| let sdp = offer.to_string(); | ||
| // TODO, we should extend the codec support to AV1 ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
worth testing if AV1 has this issue or not.. the JS logic is applying to AV1 as well
And we should do it for Desktop only