-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
fix: Change ffmpeg to post process at priority 20 #21587
Conversation
Thanks for taking the time to open a PR!
|
Issue: #21585 |
Yeah, that's why we push it off until after the tests complete - and then when there's nothing else competing for CPU, allow it to process as quickly as it can. "One thread per CPU, process as quickly as possible" is the intended behavior here. Generally you don't want to be running multiple instances of Cypress on the same machine at the same time. The preferred pattern is to use docker containers for parallelization - and then use Docker's facilities to manage resources. |
On further consideration (see comments in parent issue), I think I'm fine with the change - if ffmpeg is the only thing running, then it won't matter if priority is set lower, it won't slow down video processing. I updated the PR with latest develop in order to kick off the automated tests - once they're green I'll verify that they haven't been slowed down and we can merge in. I don't think we need any additional tests here. |
@rrauenza - Looks mostly good, just need to clean up the dangling comma so it'll pass linting (specifically, it needs a trailing comma). |
@BlueWinds Actually... that lint error was really good because I didn't put the priority: 20 inside a {}. I don't understand the lint error... there was a trailing comma.... but the code was wrong! Fix pushed just now. |
7cfec87
to
14e5abc
Compare
@rrauenza - Looks good to me. We're holding off merging anything else in before the 10.0.0 release tomorrow, but will try and get this in for 10.0.1. |
…ypress-io/cypress into tgriesser/fix/22038-esm-import-windows * 'tgriesser/fix/22038-esm-import-windows' of github.com:cypress-io/cypress: fix: Change ffmpeg to post process at priority 20 (#21587)
…hub.com:cypress-io/cypress into tgriesser/fix/change-typescript-detection-rules * 'tgriesser/fix/change-typescript-detection-rules' of github.com:cypress-io/cypress: fix: do not watch specs on run mode (#22060) fix: #22038 support esm import for windows (#22042) fix: Change ffmpeg to post process at priority 20 (#21587)
Released in This comment thread has been locked. If you are still experiencing this issue after upgrading to |
User facing changelog
Run the post processing ffmpeg at lower priority, similar to when capturing
Additional details
When running concurrent cicd, the ffmpeg consumes a LOT of CPU. My experience showed 600%.
How has the user experience changed?
No
PR Tasks
cypress-documentation
?type definitions
?cypress.schema.json
?