-
-
Notifications
You must be signed in to change notification settings - Fork 448
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 option to encode using baseline/constrained baseline. #1932
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -184,6 +184,13 @@ VBR: Variable BitRate mode. Not commended because it may throw off the adaptive | |
#[schema(flag = "steamvr-restart")] | ||
pub filler_data: bool, | ||
|
||
#[schema(strings( | ||
display_name = "h264: Profile", | ||
help = "Whenever possible, attempts to use this profile. May increase compatibility with varying mobile devices. Only has an effect for h264. Doesn't affect NVENC on Windows." | ||
))] | ||
#[schema(flag = "steamvr-restart")] | ||
pub h264_profile: H264Profile, | ||
|
||
#[schema(strings(help = r#"CAVLC algorithm is recommended. | ||
CABAC produces better compression but it's significantly slower and may lead to runaway latency"#))] | ||
#[schema(flag = "steamvr-restart")] | ||
|
@@ -420,6 +427,18 @@ pub enum CodecType { | |
Hevc = 1, | ||
} | ||
|
||
#[repr(u8)] | ||
#[derive(SettingsSchema, Serialize, Deserialize, Debug, Copy, Clone)] | ||
#[schema(gui = "button_group")] | ||
pub enum H264Profile { | ||
#[schema(strings(display_name = "High"))] | ||
High = 0, | ||
#[schema(strings(display_name = "Main"))] | ||
Main = 1, | ||
#[schema(strings(display_name = "Baseline"))] | ||
Baseline = 2, | ||
} | ||
|
||
#[derive(SettingsSchema, Serialize, Deserialize, Clone)] | ||
#[schema(collapsible)] | ||
pub struct VideoConfig { | ||
|
@@ -1165,6 +1184,9 @@ pub fn session_settings_default() -> SettingsDefault { | |
variant: RateControlModeDefaultVariant::Cbr, | ||
}, | ||
filler_data: false, | ||
h264_profile: H264ProfileDefault { | ||
variant: H264ProfileDefaultVariant::High, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe the default level should be Main. Why was High chosen? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. High was chosen for continuity with the existing default settings in some of the encoders. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nowrep Any reason for this? Main should have less latency. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure how reliable this is but this says that high main and base have no difference in latency There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While higher profiles do increase complexity it seems it may not correlate to latency There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you want to fix software decoder compatibility then just use ffmpeg in the client instead of MediaCodec. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While this is correct, in practice, AHardwareBuffers are involved, and it'd likely involve some refactoring, and there are APIs that expose these AHardwareBuffers. I don't have the necessary knowledge. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe the encoding profile is still good to have. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As far as I'm aware decoder can't provide supported profiles outside of successful or failed initialization. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That's too bad
No we don't. we can do the same thing we do for FPS, we just choose the closes one if unsupported by the client. |
||
}, | ||
entropy_coding: EntropyCodingDefault { | ||
variant: EntropyCodingDefaultVariant::Cavlc, | ||
}, | ||
|
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 it's more natural to have Baseline as 0th index, then Main then High. Also, you don't need to specify the display_name, as in this case the dashboard is able to figure it out by itself.
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.
High was set as zeroth index because I was uncertain of the behaviour of certain defaults. In most safe languages, values which are not strictly defined but are initialized end up zero. Rust is not necessarily the same, but there is also C++ code involved with no default value provided when reading from the JSON. I do not know the behaviour of this code on error, so I decided the zero value should be the default value, so the behaviour is as consistent as reasonably possible.
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'm not too opinionated, that's ok then.