Skip to content

Commit f89e868

Browse files
committed
review: address Copilot feedback on ProRes encoder integration
video.h: is_known_video_format() now uses `< SUNSHINE_FORMAT_COUNT` instead of the hardcoded `<= SUNSHINE_FORMAT_PRORES` upper bound. Future codecs are now a one-enum-bump change. video.cpp: require_prores binds to the immutable config::video.prores_mode rather than the mutable active_prores_mode. adjust_encoder_constraints() may demote active_prores_mode to 0; reading the immutable source keeps the "user explicitly asked for forced ProRes" intent intact so we fail loudly instead of silently picking a non-ProRes encoder. cmake/dependencies/macos.cmake: Pull in the SCREEN_CAPTURE_KIT_LIBRARY REQUIRED change too — this PR stacks on the SCK backend PR and needs the same SDK enforcement. Addresses Copilot inline feedback from the closed upstream PR cycle.
1 parent 0086784 commit f89e868

2 files changed

Lines changed: 17 additions & 3 deletions

File tree

src/video.cpp

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1169,12 +1169,16 @@ namespace video {
11691169
},
11701170
{
11711171
// Common options
1172+
// Note: max_ref_frames is intentionally omitted for H.264 because
1173+
// VideoToolbox on Apple Silicon produces all-IDR output when
1174+
// ReferenceBufferCount=1 is set for H.264, causing massive bandwidth
1175+
// inflation (~3x) and frame drops. HEVC and AV1 are unaffected and
1176+
// retain max_ref_frames=1. See LizardByte/Sunshine#5013.
11721177
{
11731178
{"allow_sw"s, &config::video.vt.vt_allow_sw},
11741179
{"require_sw"s, &config::video.vt.vt_require_sw},
11751180
{"realtime"s, &config::video.vt.vt_realtime},
11761181
{"prio_speed"s, 1},
1177-
{"max_ref_frames"s, 1},
11781182
},
11791183
{}, // SDR-specific options
11801184
{}, // HDR-specific options
@@ -2930,7 +2934,14 @@ namespace video {
29302934
active_hevc_mode = config::video.hevc_mode;
29312935
active_av1_mode = config::video.av1_mode;
29322936
active_prores_mode = config::video.prores_mode;
2933-
const bool require_prores = active_prores_mode >= 2;
2937+
// Bind `require_prores` to the user-configured value, NOT to the
2938+
// mutable `active_prores_mode` global. `adjust_encoder_constraints`
2939+
// below may demote `active_prores_mode` to 0 when no encoder supports
2940+
// ProRes; reading from the immutable config source keeps the
2941+
// "user explicitly asked for forced ProRes" intent intact across that
2942+
// demotion, so we can fail loudly instead of silently picking a
2943+
// non-ProRes encoder.
2944+
const bool require_prores = config::video.prores_mode >= 2;
29342945
last_encoder_probe_supported_ref_frames_invalidation = false;
29352946

29362947
auto adjust_encoder_constraints = [&](encoder_t *encoder) {

src/video.h

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,10 @@ namespace video {
3131
static_assert(SUNSHINE_FORMAT_PRORES == 3);
3232

3333
inline bool is_known_video_format(int video_format) {
34-
return video_format >= SUNSHINE_FORMAT_H264 && video_format <= SUNSHINE_FORMAT_PRORES;
34+
// Express the upper bound via SUNSHINE_FORMAT_COUNT so adding a future
35+
// codec is purely a matter of bumping the enum — no need to remember
36+
// to update this predicate.
37+
return video_format >= SUNSHINE_FORMAT_H264 && video_format < SUNSHINE_FORMAT_COUNT;
3538
}
3639

3740
inline bool is_video_format_enabled_by_prores_gate(int video_format, int prores_mode) {

0 commit comments

Comments
 (0)