SaveImage: multithread image saving#13636
SaveImage: multithread image saving#13636alexheretic wants to merge 2 commits intoComfy-Org:masterfrom
Conversation
Speeds up saving of multiple images at once
alexheretic
requested review from
Kosinkadink,
comfyanonymous and
guill
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe SaveImage.save_images method now submits per-image PNG encoding and file writes to a ThreadPoolExecutor instead of performing them inline. A new module-level save_png(image, full_output_folder, filename, prompt, extra_pnginfo, compress_level) encapsulates the conversion, optional metadata assembly, and img.save(...) call. save_images constructs filenames and results, increments the counter during submission, then waits on each future (f.result()) to propagate exceptions. concurrent.futures was added as an import. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nodes.py`:
- Around line 1644-1655: The ThreadPoolExecutor submits save_png tasks but
discards their Future objects so exceptions are lost; collect the Futures
returned by exe.submit (e.g., into a local list like futures), then after
submission iterate over them (or use concurrent.futures.as_completed) and call
future.result() to surface any exceptions and handle/log/raise them
appropriately; ensure this check happens before returning results so disk I/O
errors from save_png are propagated to the caller (references:
ThreadPoolExecutor, exe.submit, save_png, results).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
|
Hi @alexheretic and thank you for your contribution. We are currently reworking the Save Image node to support additional file formats and advanced parameters such as increased bit depth per channel and color spaces. I would suggest to close this PR and submit a new one once the new version of the node comes out (hopefully in a couple of days). Thanks again. |
2 participants
Speeds up saving of multiple images at once by saving each image using a thread pool.
Motivation: I have a workflow where I upscale a video and save each frame (e.g. 81 frames) as a png. The current
SaveImagesequential saving of each frame can be slow.Impl notes
It may be slightly more efficient to check if there is just one image and omit the thread pool, but in my testing this doesn't seem to make much difference so I opted to keep the code simpler by always using the pool.
Benchmark:
SaveImageto save 81 3024x1360 frames as pngsTested on Arch Linux, Ryzen 7 5800X
before (sequential)
#6 [SaveImage]: 30.01sPR (multithread)
#6 [SaveImage]: 6.69sSignificantly faster using multithreading.
Different max_workers results
Trying different max workers suggests the default is good and should adjust well to users CPUs (mine has 16 logical cores).
PR (max_workers=2)
#6 [SaveImage]: 17.60sPR (max_workers=5)
#6 [SaveImage]: 8.73sPR (max_workers=64)
#6 [SaveImage]: 6.22sBenchmark:
SaveImageto save a single 3024x1360 frame as a pngbefore (sequential)
#6 [SaveImage]: 0.35sPR (multithread)
#6 [SaveImage]: 0.38sSame speed either way.