-
-
Notifications
You must be signed in to change notification settings - Fork 32
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 framerate cli option #36
Conversation
First of all thank you very much for your contribution, that is very appreciated. The problem with a fully configurable frame rate are IMO 2. This is why in the reference issue I suggested a flag that leads to a preconfigured framerate opposed to a fully flexible configuration. So what I suggest in essence is to add a flag that translates to something like 10fps and then analyzing if the capture delays are still acceptable small. |
First off, my bad for just dropping a random PR on you that isn't even close to right! I did some analysis of the time it takes to take a screenshot on my platform. The inner part of the capture_thread loop (everything besides the timeout) takes about 126ms on average. My platform is Manjaro linux, so this could be different on other platforms. Here's the more detailed data:
So even if the delay was calculated dynamically, with something like this: let time_before_screenshot = Instant::now()
// Take and process screenshot
let time_after_screenshot = Instant::now()
let lag = time_after_screenshot - time_before_screenshot;
if lag < framerate {
// Only sleep the amount needed to catch the next frame. Only works if framerate is higher than the time it takes to capture.
std::thread::sleep(framerate - lag);
} one could only run at 6fps without any skipping, or almost at 8fps without skipping. Does this seem like it's worth it to you? I don't know if a 2-3fps win is worth it. If not I'll just close this PR. Thank you for taking the time to review it! |
Thanks for the investigation and infos. So we got here 2 options.
Of course option 1 would be more effort but might be an interesting challenge. (as part of #29 I'm starting to use a thread pool for throttle like below via rayon): let pool = rayon::ThreadPoolBuilder::new()
.num_threads(10)
.build()
.unwrap(); |
Sorry for the radio silence. I've been trying to think of how to approach attempt strategy 1 but I keep getting stuck when it comes to the |
No worries, then just go ahead as you've said. |
19420c7
to
a1c2b8c
Compare
- add a framerate cli argument `-f | --framerate` - limit the possible framerates to 4 and 8 for now - introduce new wrapper types for better readability - adjust the docs
- add a framerate cli argument `-f | --framerate` - limit the possible framerates to 4 and 8 for now - introduce new wrapper types for better readability - adjust the docs
- add a framerate cli argument `-f | --framerate` - limit the possible framerates to 4 and 8 for now - introduce new wrapper types for better readability - adjust the docs
- add a framerate cli argument `-f | --framerate` - limit the possible framerates to 4 and 8 for now - introduce new wrapper types for better readability - adjust the docs - retyping of u128 to Timecode not everywhere fully clean - the framedrop mechanism is not thread safe solid
- retyping of u128 to Timecode not everywhere fully clean - impl thread safe frame dropping strategy with comparing image hashes - keeping a linked list of `FrameEssences`
this PR continues with #112 |
- add a framerate cli argument `-f | --framerate` - limit the possible framerates to 4 and 8 for now - introduce new wrapper types for better readability - adjust the docs
- add a framerate cli argument `-f | --framerate` - limit the possible framerates to 4 and 8 for now - introduce new wrapper types for better readability - adjust the docs - retyping of u128 to Timecode not everywhere fully clean - the framedrop mechanism is not thread safe solid
- retyping of u128 to Timecode not everywhere fully clean - impl thread safe frame dropping strategy with comparing image hashes - keeping a linked list of `FrameEssences`
Simple addition to fix issue #28