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

fix: Change ffmpeg to post process at priority 20 #21587

Merged
merged 8 commits into from
Jun 2, 2022

Conversation

rrauenza
Copy link
Contributor

@rrauenza rrauenza commented May 20, 2022

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

  • [n/a ] Have tests been added/updated?
  • Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
  • [n/a ] Has a PR for user-facing changes been opened in cypress-documentation?
  • [n/a ] Have API changes been updated in the type definitions?
  • [ n/a] Have new configuration options been added to the cypress.schema.json?

@rrauenza rrauenza requested a review from a team as a code owner May 20, 2022 17:25
@rrauenza rrauenza requested review from jennifer-shehane and removed request for a team May 20, 2022 17:25
@cypress-bot
Copy link
Contributor

cypress-bot bot commented May 20, 2022

Thanks for taking the time to open a PR!

@CLAassistant
Copy link

CLAassistant commented May 20, 2022

CLA assistant check
All committers have signed the CLA.

@rrauenza
Copy link
Contributor Author

rrauenza commented May 20, 2022

Issue: #21585

@jennifer-shehane jennifer-shehane requested review from BlueWinds and removed request for jennifer-shehane May 20, 2022 17:32
@mjhenkes mjhenkes linked an issue May 23, 2022 that may be closed by this pull request
@BlueWinds
Copy link
Contributor

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.

@BlueWinds BlueWinds changed the title Change ffmpeg to post process at priority 20 fix: Change ffmpeg to post process at priority 20 May 23, 2022
@BlueWinds
Copy link
Contributor

BlueWinds commented May 23, 2022

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. priority is a fluent ffmpeg option, so we don't need to test that it works, and our existing set up sets around video recording would catch any regression it would cause. 1 line PR seems fine in this case. 🤷‍♀️

@BlueWinds
Copy link
Contributor

BlueWinds commented May 24, 2022

@rrauenza - Looks mostly good, just need to clean up the dangling comma so it'll pass linting (specifically, it needs a trailing comma).

@mjhenkes mjhenkes self-requested a review May 25, 2022 20:51
@rrauenza
Copy link
Contributor Author

rrauenza commented May 25, 2022

@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.

@rrauenza rrauenza force-pushed the patch0 branch 2 times, most recently from 7cfec87 to 14e5abc Compare May 25, 2022 22:09
@BlueWinds
Copy link
Contributor

@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.

@BlueWinds BlueWinds merged commit a0a64cc into cypress-io:develop Jun 2, 2022
tgriesser added a commit that referenced this pull request Jun 2, 2022
…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)
tgriesser added a commit that referenced this pull request Jun 2, 2022
…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)
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Jun 2, 2022

Released in 10.0.2.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v10.0.2, please open a new issue.

@cypress-bot cypress-bot bot locked as resolved and limited conversation to collaborators Jun 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ffmpeg doesn't always run at low priority
4 participants