-
Notifications
You must be signed in to change notification settings - Fork 1.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
Implement multithreading #698
Conversation
Python 3.6 Travis seems to fail on something unrelevant to the changes |
Utilizing all 12 of my cores with this implementation allowed me to go from The current implementation is rather gimmicky, but the performance boost is very much worth exploring |
I agree, great research. Multithreading is not always a great advantage for various reasons (sometimes the ffmpeg encoding is the bottleneck, and sometimes the make_frame function already uses all the core) but in your case it seems to make a big difference. Some remarks: the user should have the choice of multithreading in e.g. write_videoclip. If the user decides to use only one thrad it should be the current moviepy code running, and if more, fall back to your parralel workers. Also in the final commit please make sure that the code is clean, pep8, and that there is no additional files like test2.py. Can you also add some formal tests in the test suite ? Thanks for this contribution ! |
Yeah I agree, threading should definitely be an opt-in. By my research the best ratio of python threads to ffmpeg threads is 1:3, so in my case I gave 9 threads to ffmpeg and 3 to python. EDIT: Ratio is heavily dependant on what kind of edits you have applied Ideally I think the Right now I'm trying to tackle the issue of subprocesses not exiting if the main process is killed (CTRL+C for example). For some reason calling |
9461417
to
0196ac4
Compare
0196ac4
to
f0a2c22
Compare
Unfortunately I don't think I can get this to work in a very user friendly manner. The issue is serializing/deserializing the clips so that they can be re-created on the parallel processes. Naturally we want to open new file handles on the parallel processes, and this is the part that becomes problematic. |
This is the best and most user friendly solution I could come up with. Can be improved if someone figures out a good way to serialize/deserialize clips. Usage example: # Single threaded
def concat_clips():
files = [
"myclip1.mp4",
"myclip2.mp4",
"myclip3.mp4",
"myclip4.mp4",
]
clips = [VideoFileClip(file) for file in files]
final = concatenate_videoclips(clips, method="compose")
final.write_videofile("output.mp4") # Multi threaded
from moviepy.multithreading import multithread_write_videofile
def concat_clips():
files = [
"myclip1.mp4",
"myclip2.mp4",
"myclip3.mp4",
"myclip4.mp4",
]
multithread_write_videofile("output.mp4", get_final_clip, {"files": files})
def get_final_clip(files):
clips = [VideoFileClip(file) for file in files]
final = concatenate_videoclips(clips, method="compose")
return final |
f0a2c22
to
767b6ee
Compare
Added a simple test now, let's see if it works @Zulko thoughts on the current state? |
767b6ee
to
c89117d
Compare
c89117d
to
39b993f
Compare
Implement multithreaded clip rendering to file Allow for more CPU utilization especially on heavily edited clips
Remove unnecessary iterations from test_clips_array_duration
39b993f
to
18e1bd8
Compare
Everything is cleaned up and seems to be working correctly now, let me know what you think |
1 similar comment
@Zulko sorry for bugging you, but could I get any comments on this? 😄 |
try: | ||
return self.process.start() | ||
except BrokenPipeError as e: | ||
print("=" * 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.
this looks like a code smell to me. It's very ugly and hard to read.
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 do more or less agree, what would you suggest as an alternative?
Additionally, I never did figure out the actual cause of this broken pipe error, but I assume it has to do with how Python handles multiple processes and passing data between them.
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'd use dedent with a unique multiline text
With the usage example you posted I'm getting the error: AttributeError: 'VideoFileClip' object has no attribute 'endswith' |
Based on your reported error, it's trying to perform a str operation against the |
@alexebbs I can't remember any longer if I ever tried actually running my usage example, however the test does work. You can take example off of it here: https://github.com/MythicManiac/moviepy/blob/18e1bd817b6e554319567b82f921fa6f09ab0530/tests/test_multithreading.py |
Great work @MythicManiac - any potential performance improvement would be welcomed in this camp. I'm using moviepy for tasks that can take > 1 hour depending on the inputs. Now I don't know if multithreading would help, but would love to try. Is this PR on the roadmap? |
I would say this PR is only suitabe as a proof of concept, I ran into several issues while running it myself using my own fork. There definitely is performance to be gained, especially when there are lots of effects involved, but I have doubts of this implementation being the proper way to achieve that. @zamirkhan I'd suggest you to try out the fork and see how it works for you if you have lots of edits in your clips. Multithreading moviepy sadly doesn't offer much advantage if there's not much edits applied to the clips you're processing, in that case more threads should be allocated to ffmpeg. |
Hey @MythicManiac, could I ask what's the status of this implementation? I also found multi-threading to not work with Does you fix help implement multi-threading on the frame-making bit? |
@jingxlim multithreading does make rendering faster, but what the optimal ratio between ffmpeg and moviepy threads is depends on how many effects you have and what kind of effects are they. The implementation in this PR has a few issues, but it did work for me when I needed it. It however has not been updated since I opened the PR, so results may vary. |
hey @MythicManiac can you help me with the multithreading on moviepy with your library ? |
@shenhavmor10 I'm not able to provide support on this given that it's already over 2 years old and was never merged, not to mention it was more of a proof of concept anyway. https://github.com/MythicManiac/moviepy/blob/18e1bd817b6e554319567b82f921fa6f09ab0530/tests/test_multithreading.py the test here is a working example, refer to that and the code itself. |
Since this was a proof of concept and is not clear that could be suitable for moviepy, I'm closing this for now. Anyone, feel free to use it to implement some multithreaded rendering system. Anyway, thank you for the attempt @MythicManiac! |
Implement multithreading
Implement multithreaded clip rendering to file
Allow for more CPU utilization especially on heavily edited clips
Refs #584, #139