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

feat: replace native ffmpeg with bundled wasm version #305

Merged
merged 19 commits into from
Sep 4, 2022
Merged

Conversation

miraclx
Copy link
Owner

@miraclx miraclx commented Aug 8, 2022

Closes #294.

This patches #297 a bit to avoid depending on modified versions of @ffmpeg/{core,ffmpeg}.

All credit to @hard-shutdown for investigating this.

@github-actions
Copy link

github-actions bot commented Aug 8, 2022


🐋 🤖

A docker image for this PR has been built!

docker pull freyrcli/freyrjs-git:pr-305
Base Branch (master)
This Patch


What's this?

This docker image is a self-contained sandbox that includes all the patches made in this PR. Allowing others to easily use your patches without waiting for it to get merged and released officially.

For more context, see https://github.com/miraclx/freyr-js#docker-development.

@bluefoxy009
Copy link

bluefoxy009 commented Aug 8, 2022

Hello, may I ask what node version this uses, as well as how you are able to run the embedding queue without running into errors? Thanks!

EDIT: That was what the new ffmpeg instances were used for, see the logs from the tests.

@miraclx
Copy link
Owner Author

miraclx commented Aug 8, 2022

Yeah, so I found out that ffmpeg-wasm works only for node < v18 because node v18 introduces the fetch() function – https://nodejs.org/en/blog/announcements/v18-release-announce/#fetch-experimental.

The hack that worked was to remove that function from the global scope. So when ffmpeg-core checks for it, it just defaults to reading it as a file:

freyr-js/cli.js

Line 974 in c500eaf

delete globalThis.fetch;

embedding queue without running into errors?

Didn't run into any embedding errors. The only issue I see now is not being able to run one command at a time.

I saw your approach to fix that was to load ffmpeg afresh every time a track needs to be transcoded, my concern with that approach is memory usage.

Because for a 100-track playlist, it would load the 8 MB WASM blob, allocate resources to assign context etc. 100 times.

@bluefoxy009
Copy link

That was an extremely smart trick regarding the fetch api :) Then again, I believe that most computers running this will have enough RAM to run the 8 MB contexts. Maybe another trick would be to wipe MEMFS to help clear memory usage because running ffmpeg.exit() seems to make freyr exit with code 1

@miraclx
Copy link
Owner Author

miraclx commented Aug 8, 2022

Resolved. Made it so that there's only N number of ffmpeg instances at any given point in time. Defined by the concurrency.encoder key in the conf.json.

By default, this is 6 concurrent processes. Spawned on demand and reused for more tracks, if need be. Can be less than the defined value, but never more than.

@bluefoxy009
Copy link

Excellent idea, thanks for the ideas!

@Crilum

This comment was marked as off-topic.

@miraclx miraclx mentioned this pull request Sep 3, 2022
@miraclx
Copy link
Owner Author

miraclx commented Sep 3, 2022

Finally, found 6 hours to investigate the issue with this. 😪
Fixed in #335.

@miraclx miraclx merged commit 890a1dd into master Sep 4, 2022
@miraclx miraclx deleted the ffmpeg-wasm branch September 4, 2022 02:12
miraclx added a commit that referenced this pull request Sep 4, 2022
@miraclx miraclx mentioned this pull request Sep 18, 2022
@miraclx miraclx mentioned this pull request Dec 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using WASM to reduce system dependencies
3 participants