-
Notifications
You must be signed in to change notification settings - Fork 136
feat: parallel rendering, make ffmpeg exporter default #74
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
Conversation
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 great work!!!
const hiddenFolderId = uuidv4(); | ||
|
||
/** | ||
* TODO: Change this in the future after more testing. |
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.
I think we can delete this todo haha
); | ||
|
||
renderPromises.push(renderVideoOnPage(browser, server, url)); | ||
} |
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.
I think we're losing time here. Couldn't a browser start rendering right after it's ready? Right now the first one needs to wait for all the others before it can start.
|
||
for (const folder of cleanupFolders) { | ||
try { | ||
await fs.promises.rm(folder, {recursive: true, force: true}); |
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.
We could parallelize this with an array and Promise.all(). Feel free to ignore this though, I don't think we would notice any difference.
packages/template/src/project.meta
Outdated
@@ -13,15 +13,15 @@ | |||
"audioOffset": 0 | |||
}, | |||
"preview": { | |||
"fps": 60, | |||
"fps": 30, |
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.
Is this change intended?
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.
yeah, this makes the preview in the editor smoother
No description provided.