-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Significantly reduce rendering time with a separate thread for writing frames to stream #3888
Conversation
Woah this was on our to-do list for a bit recently, thanks so much for implementing it :) |
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.
The implementation looks great to me!
I left a few comments, so I would appreciate it if you could take a look at those. Other than that, the changes look good to me! Thanks for making this change!
updates: - [github.com/astral-sh/ruff-pre-commit: v0.5.4 → v0.5.5](astral-sh/ruff-pre-commit@v0.5.4...v0.5.5) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
ManimCommunity#3890) * Replace the TypeError message code in the _typecheck_input method in the DrawBorderThenFill class. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
- This is probably leftover from back when manim used subprocess to write frames to FFmpeg via stdin
Hi @JasonGrace2282, Thank you for taking the time to review. I added type hints the instance variables, method parameters, and return types of methods (only the ones I modified). Please let me know if there is anything else that needs to be addressed. Thank you!! |
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.
Overall, looks good to me!
I left a couple (pretty pedantic!) comments, so I would appreciate it if you could take a look at those.
Other than that, looks good to me! I'll leave this open for a few days in case anyone else wants to review it :)
Hi @JasonGrace2282 , I checked in the changes you requested. Please let me know if anything else needs to be amended. Thank you!! |
…g frames to stream (ManimCommunity#3888) * Add separate thread for writing frames to stream * [pre-commit.ci] pre-commit autoupdate (ManimCommunity#3889) updates: - [github.com/astral-sh/ruff-pre-commit: v0.5.4 → v0.5.5](astral-sh/ruff-pre-commit@v0.5.4...v0.5.5) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * Replace the TypeError message code in the _typecheck_input method in … (ManimCommunity#3890) * Replace the TypeError message code in the _typecheck_input method in the DrawBorderThenFill class. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * Remove print statements used for debugging * Remove writing process termination - This is probably leftover from back when manim used subprocess to write frames to FFmpeg via stdin * Add type hints to modified methods & instance vars * Fix inline code in docstring & type hint for queue --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Irvanal Haq <[email protected]>
Overview: What does this pull request change?
Hi, I apologize for dropping this enhancement for almost 9 months now. On the bright side, I'm glad I was able to come back to this after the change to using
av
for writing frames. As a show of good faith, I made sure to run benchmarks on two different machines, while accounting for all rendering quality levels. I will also attach the benchmark script to this PR in order to allow for others to verify on different setups/machines.This pull request introduces a significant performance improvement to the
SceneFileWriter
class in Manim by utilizing multithreading for the frame encoding process.Motivation and Explanation: Why and how do your changes improve the library?
The current implementation of
SceneFileWriter
encodes frames serially, which can be a bottleneck during the rendering process, especially for high-resolution videos. By introducing a separate thread for writing the video frames, we increase throughput, and we decrease the overall rendering time.Links to added or changed documentation pages
N/A
Benchmark Results
I carried out benchmarks on two different machines: an m2 MacBook Air and a Debian server that runs on a AMD Ryzen 9 7950X. The results show a noticeable reduction in rendering times across various quality levels with the multithreaded implementation as compared to the current implementation (main).
M2 MacBook Air
Debian Server with AMD Ryzen 9 7950X
Summary
System Information
Graphs above were annotated with system information.
Further Information and Comments
Steps to Reproduce
This assumes you're using my fork of manim.
Please put render.py and benchmark.py in the manim root. Note that you'll need to set the absolute path to the manim repo root using
REPO_PATH
Install dependencies:
benchmark.py
render.py
Reviewer Checklist