Skip to content

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