Skip to content
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

Closed
wants to merge 2 commits into from
Closed

Add framerate cli option #36

wants to merge 2 commits into from

Conversation

octotep
Copy link

@octotep octotep commented Jan 2, 2021

Simple addition to fix issue #28

@sassman
Copy link
Owner

sassman commented Jan 2, 2021

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.
First with the current threading model it won't be possible to hit any high (>10 probably) fps because the capture and writing to cache comes at a cost that will delay the next capture.
Second the guarantee of being fast might not more uphold for >10fps and probably not brings to much value.

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.

@sassman sassman added the Type: Enhancement New feature or request label Jan 2, 2021
@octotep
Copy link
Author

octotep commented Jan 2, 2021

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:

Median: 126.9086225
Mean: 126.72211361818182
Stdev: 12.075912167786203
Min: 111.949008
Max: 155.277019

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!

@sassman
Copy link
Owner

sassman commented Jan 2, 2021

Thanks for the investigation and infos.
I think at some point it would be worth to introduce a thread pool that is the size of the frame rate e.g. 10. With this thread pool we could achieve a stable capture rate since every frame within a second is captured by a thread of the pool. Of course this implies that one capture will never take longer than 1 second. So that in the best case only 1 - 3 threads will be busy at a time, and in the worst case 6-8 threads maybe remain busy.

So we got here 2 options.

  1. you could explore the capture via a thread pool to be used in the loop
  2. you close up this PR by only adding a flag that translates to the 8fps that you have mentioned, and we are good.

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();

@octotep
Copy link
Author

octotep commented Jan 6, 2021

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 last_frame mechanic. Since I can't think of a good way to handle that with multithreading, I'm just going to implement simpler flag to set the fps to 8.

@sassman
Copy link
Owner

sassman commented Jan 6, 2021

No worries, then just go ahead as you've said.
I think sooner or later I will refactor the capture thread anyway maybe this issue then is getting away.

@sassman sassman force-pushed the main branch 2 times, most recently from 19420c7 to a1c2b8c Compare January 6, 2021 14:42
sassman added a commit that referenced this pull request Feb 15, 2022
- 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
sassman added a commit that referenced this pull request Feb 18, 2022
- 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
sassman added a commit that referenced this pull request Mar 28, 2022
- 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
sassman added a commit that referenced this pull request Mar 28, 2022
- 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
sassman added a commit that referenced this pull request Mar 28, 2022
- 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`
@sassman
Copy link
Owner

sassman commented Apr 18, 2022

this PR continues with #112

@sassman sassman closed this Apr 18, 2022
sassman added a commit that referenced this pull request Apr 18, 2022
- 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
sassman added a commit that referenced this pull request Apr 18, 2022
- 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
sassman added a commit that referenced this pull request Apr 18, 2022
- 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`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants