-
-
Notifications
You must be signed in to change notification settings - Fork 50
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(lib): smarter files reversing #439
base: main
Are you sure you want to change the base?
Conversation
Implement a smarter generation of reversed files by splitting the video into smaller segments. Closes #434
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #439 +/- ##
==========================================
+ Coverage 80.37% 80.59% +0.22%
==========================================
Files 22 22
Lines 1824 1850 +26
==========================================
+ Hits 1466 1491 +25
- Misses 358 359 +1 ☔ View full report in Codecov by Sentry. |
I have to hand it to you, you are very efficient! I didn't think this would get addressed so fast, so thanks a ton! A few follow-up questions:
To be clear, I think this PR more than covers the initial bug, but the above might be worth considering. |
Thanks for your comment! This is PR is not ready yet (IMO there is room for improvement)
I can increase or reduce it, but I did not notice any noticeable change. Reversing files seems pretty slow in general... But indeed, my plan is to provide configuration of all those parameters through class config. I need to rework this, see #441.
FFmpeg is maybe multithreaded, but PyAV isn't (at least to my knowledge, by default, or to some extent, see https://pyav.org/docs/develop/cookbook/basics.html#threading). Using all my Still, the performance changes are not great (something like x2 to x4 for 16 threads), and I am open for help if you want to try finding a better combo :-) Using |
I think you are right, this is likely IO bound anyways... However, I know that ffmpeg threads are low priority and designed to not bogg down a system (i.e: they show up in purple in htop), I don't think python multiprocessing acts like this so having this spin up a process per core might be problematic in that sense. Ultimately, there's no right way to do this, having options via #441 seems like the best path forward. |
I'll put this PR on hold until I can implement #441. Maybe Python's multiprocessing isn't the right solution, or the most beneficial one (the speed-up is very low vs the number of processes used). |
Update: this PR is on hold because reversing slides with intermediate concatenation seems to introduce issues with video encodings... |
Implement a smarter generation of reversed files by splitting the video into smaller segments.
Closes #434