Skip to content

Commit 99f06f4

Browse files
djadjkaclaude
andcommitted
refactor(video): derive framerate rationals via shared helper
Address review feedback: extract video::framerate_to_rational() as the single place implementing the "exact rational when framerateX100 is set, integer framerate otherwise" pattern, and rebuild capture_frame_interval() on top of it. Migrate the call sites that re-implemented the pattern: avcodec and NVENC encoder setup become branchless, the PipeWire and wlroots capture pacers no longer re-derive the rational for logging, and the Windows strict frame rate reuses the helper inside its sentinel branch (the {0,0} sentinel must stay to keep the refresh-rate matching heuristic). Add unit tests for the new helper and the capture frame interval, including the integer fallback. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 79aa037 commit 99f06f4

7 files changed

Lines changed: 75 additions & 32 deletions

File tree

src/nvenc/nvenc_base.cpp

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -222,13 +222,9 @@ namespace nvenc {
222222
init_params.darWidth = encoder_params.width;
223223
init_params.encodeHeight = encoder_params.height;
224224
init_params.darHeight = encoder_params.height;
225-
init_params.frameRateNum = client_config.framerate;
226-
init_params.frameRateDen = 1;
227-
if (client_config.framerateX100 > 0) {
228-
AVRational fps = video::framerateX100_to_rational(client_config.framerateX100);
229-
init_params.frameRateNum = fps.num;
230-
init_params.frameRateDen = fps.den;
231-
}
225+
const AVRational fps = video::framerate_to_rational(client_config);
226+
init_params.frameRateNum = fps.num;
227+
init_params.frameRateDen = fps.den;
232228

233229
if (client_config.videoFormat > 0 && get_encoder_cap(NV_ENC_CAPS_NUM_ENCODER_ENGINES) > 1) {
234230
// SFE supports HEVC/AV1 if you have more than 1 nvenc block

src/platform/linux/pipewire.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -676,11 +676,11 @@ namespace pipewire {
676676
// calculate frame interval we should capture at
677677
framerate = config.framerate;
678678
delay = ::video::capture_frame_interval(config);
679-
if (config.framerateX100 > 0) {
680-
AVRational fps_strict = ::video::framerateX100_to_rational(config.framerateX100);
681-
BOOST_LOG(info) << "[pipewire] Requested frame rate [" << fps_strict.num << "/" << fps_strict.den << ", approx. " << av_q2d(fps_strict) << " fps]";
679+
const AVRational fps = ::video::framerate_to_rational(config);
680+
if (fps.den != 1) {
681+
BOOST_LOG(info) << "[pipewire] Requested frame rate [" << fps.num << "/" << fps.den << ", approx. " << av_q2d(fps) << " fps]";
682682
} else {
683-
BOOST_LOG(info) << "[pipewire] Requested frame rate [" << framerate << "fps]";
683+
BOOST_LOG(info) << "[pipewire] Requested frame rate [" << fps.num << "fps]";
684684
}
685685
mem_type = hwdevice_type;
686686

src/platform/linux/wlgrab.cpp

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -30,15 +30,12 @@ namespace wl {
3030
public:
3131
int init(platf::mem_type_e hwdevice_type, const std::string &display_name, const ::video::config_t &config) {
3232
// calculate frame interval we should capture at
33-
if (config.framerateX100 > 0) {
34-
AVRational fps_strict = ::video::framerateX100_to_rational(config.framerateX100);
35-
delay = std::chrono::nanoseconds(
36-
(static_cast<int64_t>(fps_strict.den) * 1'000'000'000LL) / fps_strict.num
37-
);
38-
BOOST_LOG(info) << "[wlgrab] Requested frame rate [" << fps_strict.num << "/" << fps_strict.den << ", approx. " << av_q2d(fps_strict) << " fps]";
33+
delay = ::video::capture_frame_interval(config);
34+
const AVRational fps = ::video::framerate_to_rational(config);
35+
if (fps.den != 1) {
36+
BOOST_LOG(info) << "[wlgrab] Requested frame rate [" << fps.num << "/" << fps.den << ", approx. " << av_q2d(fps) << " fps]";
3937
} else {
40-
delay = ::video::capture_frame_interval(config);
41-
BOOST_LOG(info) << "[wlgrab] Requested frame rate [" << config.framerate << "fps]";
38+
BOOST_LOG(info) << "[wlgrab] Requested frame rate [" << fps.num << "fps]";
4239
}
4340

4441
mem_type = hwdevice_type;

src/platform/windows/display_base.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -720,7 +720,7 @@ namespace platf::dxgi {
720720
client_frame_rate = config.framerate;
721721
client_frame_rate_strict = {0, 0};
722722
if (config.framerateX100 > 0) {
723-
AVRational fps = ::video::framerateX100_to_rational(config.framerateX100);
723+
const AVRational fps = ::video::framerate_to_rational(config);
724724
client_frame_rate_strict = DXGI_RATIONAL {static_cast<UINT>(fps.num), static_cast<UINT>(fps.den)};
725725
}
726726

src/video.cpp

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1670,13 +1670,9 @@ namespace video {
16701670
ctx.reset(avcodec_alloc_context3(codec));
16711671
ctx->width = config.width;
16721672
ctx->height = config.height;
1673-
ctx->time_base = AVRational {1, config.framerate};
1674-
ctx->framerate = AVRational {config.framerate, 1};
1675-
if (config.framerateX100 > 0) {
1676-
AVRational fps = video::framerateX100_to_rational(config.framerateX100);
1677-
ctx->framerate = fps;
1678-
ctx->time_base = AVRational {fps.den, fps.num};
1679-
}
1673+
const AVRational fps = video::framerate_to_rational(config);
1674+
ctx->framerate = fps;
1675+
ctx->time_base = AVRational {fps.den, fps.num};
16801676

16811677
switch (config.videoFormat) {
16821678
case 0:

src/video.h

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -388,15 +388,24 @@ namespace video {
388388
}
389389
}
390390

391+
/**
392+
* @brief Requested framerate as an exact rational.
393+
* Uses the exact fractional rate when the client provided an X100 value,
394+
* otherwise the integer framerate over 1.
395+
*/
396+
inline AVRational framerate_to_rational(const config_t &config) {
397+
if (config.framerateX100 > 0) {
398+
return framerateX100_to_rational(config.framerateX100);
399+
}
400+
return AVRational {config.framerate, 1};
401+
}
402+
391403
/**
392404
* @brief Capture frame interval for the requested framerate.
393405
* Uses the exact fractional rate when the client provided an X100 value.
394406
*/
395407
inline std::chrono::nanoseconds capture_frame_interval(const config_t &config) {
396-
if (config.framerateX100 > 0) {
397-
AVRational fps = framerateX100_to_rational(config.framerateX100);
398-
return std::chrono::nanoseconds {(static_cast<int64_t>(fps.den) * 1'000'000'000LL) / fps.num};
399-
}
400-
return std::chrono::nanoseconds {std::chrono::seconds {1}} / config.framerate;
408+
const AVRational fps = framerate_to_rational(config);
409+
return std::chrono::nanoseconds {(static_cast<int64_t>(fps.den) * 1'000'000'000LL) / fps.num};
401410
}
402411
} // namespace video

tests/unit/test_video.cpp

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,3 +76,48 @@ INSTANTIATE_TEST_SUITE_P(
7676
std::make_tuple(9498, AVRational {4749, 50}) // from my LG 27GN950
7777
)
7878
);
79+
80+
struct FramerateToRationalTest: testing::TestWithParam<std::tuple<int, int, AVRational>> {};
81+
82+
TEST_P(FramerateToRationalTest, Run) {
83+
const auto &[framerate, framerateX100, expected] = GetParam();
84+
video::config_t config {};
85+
config.framerate = framerate;
86+
config.framerateX100 = framerateX100;
87+
auto res = video::framerate_to_rational(config);
88+
ASSERT_EQ(0, av_cmp_q(res, expected)) << "expected "
89+
<< expected.num << "/" << expected.den
90+
<< ", got "
91+
<< res.num << "/" << res.den;
92+
}
93+
94+
INSTANTIATE_TEST_SUITE_P(
95+
FramerateToRationalTests,
96+
FramerateToRationalTest,
97+
testing::Values(
98+
std::make_tuple(60, 0, AVRational {60, 1}), // no X100 value, fall back to integer framerate
99+
std::make_tuple(60, 5994, AVRational {60000, 1001}),
100+
std::make_tuple(120, 11988, AVRational {120000, 1001}),
101+
std::make_tuple(24, 2398, AVRational {24000, 1001})
102+
)
103+
);
104+
105+
struct CaptureFrameIntervalTest: testing::TestWithParam<std::tuple<int, int, std::chrono::nanoseconds>> {};
106+
107+
TEST_P(CaptureFrameIntervalTest, Run) {
108+
const auto &[framerate, framerateX100, expected] = GetParam();
109+
video::config_t config {};
110+
config.framerate = framerate;
111+
config.framerateX100 = framerateX100;
112+
ASSERT_EQ(expected, video::capture_frame_interval(config));
113+
}
114+
115+
INSTANTIATE_TEST_SUITE_P(
116+
CaptureFrameIntervalTests,
117+
CaptureFrameIntervalTest,
118+
testing::Values(
119+
std::make_tuple(60, 0, std::chrono::nanoseconds {16666666}),
120+
std::make_tuple(60, 5994, std::chrono::nanoseconds {16683333}), // 1e9 * 1001 / 60000
121+
std::make_tuple(120, 11988, std::chrono::nanoseconds {8341666}) // 1e9 * 1001 / 120000
122+
)
123+
);

0 commit comments

Comments
 (0)