-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
feat(screencast): use ffmpeg to produce webm in chromium #3668
Conversation
094b2cb
to
3bb2b1a
Compare
@@ -38,6 +38,7 @@ | |||
}, | |||
"license": "Apache-2.0", | |||
"dependencies": { | |||
"@ffmpeg-installer/ffmpeg": "^1.0.20", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the Docker image as well as in GitHub Actions we install ffmpeg by the operating system. If we add this NPM package it would result in two ffmpeg versions on the system. I personally would force the user to install ffmpeg on his system which is available in his PATH instead of using this package since it seems to be not very well maintained and "could" lead to unwanted behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good point. Long-term if we proceed with ffmpeg we'll likely get the binary from https://ffmpeg.org/download.html and package it along with the chromium binaries. For linux we can also just use the system one and add it to the list of required deps to avoid duplication. Using @ffmpeg-installer/ffmpeg is the easiest way to validate the approach as it has binaries for all 3 platforms (2 yrs old but still working).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a short-term solution for ffmpeg to unblock Yury.
We'll bundle it with Chromium ASAP: #3680
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fine for me then, sounds good to me 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works
e0a5d82
to
b50d8b1
Compare
a86f459
to
44a0558
Compare
44a0558
to
d29333a
Compare
#1158