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

optimize temp frame creation and transcoding #23

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

transitive-bullshit
Copy link
Owner

@transitive-bullshit transitive-bullshit commented Jun 30, 2018

This PR implements the approach discussed in #8, namely it makes a tradeoff between compute time versus temp storage requirements that should make the program more robust for larger input videos.

Previously, we first extracted all frames from each input video, then rendered the transitions where appropriate, then did a final transcoding pass that was very efficient since it only had one input source as a single stream of frames.

Now, we only extract frames from input videos that are actually used in the transitions, which for larger input videos can be a significant reduction in temp frames extracted. This has the disadvantage, however, that the final transcoding phase is more complex and involves O(n) inputs instead of O(1) inputs.

If we decide to merge this approach, it will necessitate a major version bump to v2.0.0.

I would like to gather some performance numbers for speed and memory usage before committing to this approach. In my very preliminary tests, the speed has remained approximately the same with a significant reduction in temporary disk and memory usage, though I'm guessing that we should expect speed improvements from longer / larger input videos.

@BrandonCookeDev, @daniel-habib, I'm guessing these changes will interest you. Thoughts & feedback are more than welcome.

@transitive-bullshit
Copy link
Owner Author

transitive-bullshit commented Jun 30, 2018

@mergebandit this would seriously reduce some of the issues we've seen with prod Automagical renders running out of disk space in the past.

@transitive-bullshit transitive-bullshit changed the title significantly optimize temp frame creation optimize temp frame creation and transcoding Jun 30, 2018
@x22n
Copy link

x22n commented Aug 2, 2018

@transitive-bullshit Any plans on merging this PR soon?

@transitive-bullshit
Copy link
Owner Author

transitive-bullshit commented Aug 2, 2018

@XavierTS The PR works fine for 95% of cases, but I've run into an issue with a more involved test case where ffmpeg just halts about 70% of the way through and haven't had time to follow-up.

@daniel-habib
Copy link
Contributor

daniel-habib commented Feb 16, 2019

@transitive-bullshit :
what's preventing you from merging feature/optimize-frames into master?
in other words: how can i help?
just run a ton of use cases? or do you already have specific use-cases that cause issues?

@transitive-bullshit
Copy link
Owner Author

transitive-bullshit commented Feb 16, 2019

The only problem with this approach is that the ffmpeg inputs become more complicated instead of a single stream of frames.

After running a startup focused around video creation for several years, one hard & fast rule I've found when dealing with ffmpeg is that the simpler your can make your inputs, the better your usage will scale in terms of video sizes and different types of inputs.

I have a few test cases for my previous company that this approach will randomly stall on, and aside from it likely being a bug somewhere in ffmpeg, there's not much I can do to resolve the issues.

One possibility would be if I published a fork of ffmpeg-concat that took this fundamentally different approach, as it will still likely be useful for many people.

Thoughts?

@daniel-habib
Copy link
Contributor

daniel-habib commented Feb 16, 2019

ffmpeg and keeping-it-simple - agree. you never know if some rare option was rigorously tested or even designed to support all uses cases.

having 2 active forks sounds bad. maintenance-wise: every fix has to be applied to both, ux-wize: developers would not know which of the forks is better.

i can take this branch, build my app based on that, and run some tests on it for a while if that would help with confidence in ffmpeg current options.
would that help?

@transitive-bullshit
Copy link
Owner Author

transitive-bullshit commented Feb 16, 2019

That'd be great @daniel-habib. I'd expect this branch to work for 95% of use cases.

@owen-m1
Copy link

owen-m1 commented Aug 18, 2019

Any update?

@daniel-habib
Copy link
Contributor

daniel-habib commented Aug 18, 2019

nothing from my side, sorry.
priorities changed, and i did not have resources to test this :(
apologies for not notifying before

@soerface
Copy link

soerface commented Feb 11, 2020

With the version on master, I was not able to take a 10 minute video and concat an intro and an outro to it (244 KiB Intro, 42 MiB talk, 428 KiB Outro). More than 30 GiB of /tmp space would have been required. I can't imagine how much space the one-hour talks would need.

By using this version:

$ nvm use 8
$ npm install -g https://github.com/transitive-bullshit/ffmpeg-concat.git#feature/optimize-frames

It works like a charm, thanks!

@transitive-bullshit
Copy link
Owner Author

transitive-bullshit commented Feb 12, 2020

Great to hear @soerface. You can also use the --frame-format png option to store each frame compressed instead of the faster uncompressed raw format that's the default.

@cipacda
Copy link

cipacda commented May 4, 2020

Is there any plan on merging this?

@dardo82
Copy link

dardo82 commented May 19, 2020

Is this branch still actively developed?
The package.json must be updated!
Swap it with the master branch one.

@badosu
Copy link

badosu commented Aug 15, 2020

This works pretty well for my usage, however for some reason the output does not contain audio.

Since I edit files with 3-30 GB and only want to add an introduction of 10 seconds, this would make it possible to automate this process. Unfortunately its unfeasible to use the version on master even with --frame-format png

@daniel-habib
Copy link
Contributor

daniel-habib commented Aug 16, 2020

@badosu : not having audio on a result is kind of a "feature'
i have tried to address it in #46 which was merged to master once.
one could try to merge master into this and see where it takes us. looks like it is something that needs to be done anyway to resolve conflicts.
i would have done that but:
1/ i am not sure why this was not approved in the first place. if there's a good reason for it and we don't address it - we'll just create a bad master.
2/ some people where not pleased with my "fix". i think it was cases of bad input, but still... we might want to address that before merging here.
in the meantime:
consider running another ffmpeg to concat the into a big audio after you have a big video. and then ffmpeg to merge the 2.

@badosu
Copy link

badosu commented Aug 17, 2020

Well, for often used cases the current functionality might be enough but I would expect to be able to concatenate videos with audio without scaling space usage as much as it is at the moment (with resolution x duration).

If I have to make additional commands it loses a bit of purpose (which I assume to be user friendly to ffmpeg concatenation commands).

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.

None yet

8 participants