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

Enable fast saving mode for PNGs #74324

Closed
wants to merge 1 commit into from
Closed

Enable fast saving mode for PNGs #74324

wants to merge 1 commit into from

Conversation

myaaaaaaaaa
Copy link
Contributor

@myaaaaaaaaa myaaaaaaaaa commented Mar 3, 2023

Makes saving PNGs 2.5x faster for me at only 2% (!) increase in filesize, although please note that I only did basic testing. Presumably, more compressible images would end up with a bigger filesize penalty.

Helps with Movie Maker, see godotengine/godot-proposals#4751

Fixes #51868

@myaaaaaaaaa myaaaaaaaaa requested a review from a team as a code owner March 3, 2023 21:20
@Calinou Calinou added this to the 4.1 milestone Mar 3, 2023
@fire
Copy link
Member

fire commented Mar 3, 2023

I'm interested. Not able to review this week due to a brief break from godot engine 4.0 release.

@Calinou
Copy link
Member

Calinou commented Apr 26, 2023

Benchmark

CPU: Intel Core i9-13900K
GPU: GeForce RTX 4090
OS: Fedora 37

Time taken to render https://github.com/Calinou/godot-movie-maker-demo:

Type AVI PNG - master PNG - this PR
Editor 1280×720 1m07s 6m25s 1m41s (3.81× faster than master)
Editor 3840×2160 6m49s 48m00s 10m44s (4.47× faster than master)
Release template 1280×720 39s 6m36s 1m48s (3.67× faster than master)
Release template 3840×2160 3m31s 47m06s 9m37s (4.9× faster than master)

File size of first frame in 3840×2160:

PNG - master PNG - this PR
7,173 KB 9,432 KB

Considering saving time is the most important criterion in Movie Maker, I'd say this increase in file size is fine. It's common for games in general to take large, unoptimized screenshots to ensure they save quickly (so they don't interrupt the action for too long).

Script used for testing
#!/bin/bash
set -euo pipefail
IFS=$'\n\t'

DEMO_DIR="$HOME/Documents/Godot/godot-movie-maker-demo"
mkdir -p "$DEMO_DIR/out"
export MANGOHUD=0

echo "#################################"
echo "Editor 1280x720"
echo "#################################"
echo "AVI:"
time bin/godot.linuxbsd.editor.x86_64.master --path "$DEMO_DIR" --write-movie "out/out.avi"
echo "End AVI."
echo "Master:"
time bin/godot.linuxbsd.editor.x86_64.master --path "$DEMO_DIR" --write-movie "out/out.png"
echo "End master."
echo "This PR:"
time bin/godot.linuxbsd.editor.x86_64 --path "$DEMO_DIR" --write-movie "out/out.png"
echo "End this PR."

echo "#################################"
echo "Release export template 1280x720"
echo "#################################"
echo "AVI:"
time bin/godot.linuxbsd.template_release.x86_64.master --path "$DEMO_DIR" --write-movie "out/out.avi"
echo "End AVI."
echo "Master:"
time bin/godot.linuxbsd.template_release.x86_64.master --path "$DEMO_DIR" --write-movie "out/out.png"
echo "End master."
echo "This PR:"
time bin/godot.linuxbsd.template_release.x86_64 --path "$DEMO_DIR" --write-movie "out/out.png"
echo "End this PR."

echo "#################################"
echo "Editor 3840x2160"
echo "#################################"
echo "AVI:"
time bin/godot.linuxbsd.editor.x86_64.master --path "$DEMO_DIR" --write-movie "out/out.avi" --fullscreen
echo "End AVI."
echo "Master:"
time bin/godot.linuxbsd.editor.x86_64.master --path "$DEMO_DIR" --write-movie "out/out.png" --fullscreen
echo "End master."
echo "This PR:"
time bin/godot.linuxbsd.editor.x86_64 --path "$DEMO_DIR" --write-movie "out/out.png" --fullscreen
echo "End this PR."

echo "#################################"
echo "Release export template 3840x2160"
echo "#################################"
echo "AVI:"
time bin/godot.linuxbsd.template_release.x86_64.master --path "$DEMO_DIR" --write-movie "out/out.avi" --fullscreen
echo "End AVI."
echo "Master:"
time bin/godot.linuxbsd.template_release.x86_64.master --path "$DEMO_DIR" --write-movie "out/out.png" --fullscreen
echo "End master."
echo "This PR:"
time bin/godot.linuxbsd.template_release.x86_64 --path "$DEMO_DIR" --write-movie "out/out.png" --fullscreen
echo "End this PR."

Calinou
Calinou previously approved these changes Apr 26, 2023
@fire
Copy link
Member

fire commented Apr 26, 2023

! Amazing. Will need to wait for a production team review.

@clayjohn
Copy link
Member

Won't this increase the filesize of every PNG saved by the engine? Not just the ones created by the MovieMaker feature?

@fire
Copy link
Member

fire commented Apr 26, 2023

It's common for games in general to take large, unoptimized screenshots to ensure they save quickly (so they don't interrupt the action for too long).

I think @myaaaaaaaaa is saying that's a good tradeoff.

@Calinou
Copy link
Member

Calinou commented Apr 27, 2023

Won't this increase the filesize of every PNG saved by the engine? Not just the ones created by the MovieMaker feature?

It will, but the engine defaults to saving lossless WebP when importing images since 3.5. As a result, this PR doesn't affect the size of the .godot/ folder and exported PCK by default.

That said, we could make save_png() accept a compression mode parameter (uncompressed, fast compression, best compression).1

Footnotes

  1. Uncompressed PNG may still be slower to save than saving a dedicated uncompressed format such as TGA or BMP, due to general format inefficiencies. That said, we don't have TGA or BMP encoders in Godot yet, while enabling libpng's uncompressed mode is a 1-line change.

@akien-mga akien-mga modified the milestones: 4.1, 4.x Jun 15, 2023
@PeterMarques
Copy link

PeterMarques commented Jul 1, 2023

In the "Marshalls.raw_to_base64(vp.get_texture().get_data().save_png_to_buffer())" that is very slow, and have several bottleneck points, one being that get_texture() itself, that can be better implemented to have bigger delay but lower cpu usage, the Save_png_to_buffer(), is still the one that consumes the most CPU time.

#76465
#75877

@YuriSizov YuriSizov modified the milestones: 4.x, 4.2 Jul 25, 2023
@akien-mga
Copy link
Member

That said, we could make save_png() accept a compression mode parameter (uncompressed, fast compression, best compression).

I agree we should make this a flag in save_png(). Also discussed it with @reduz who agreed.

Could default to fast compression, but users should still have a way to opt in to best compression if they value size over compression speed.

@akien-mga akien-mga modified the milestones: 4.2, 4.x Sep 26, 2023
@akien-mga akien-mga dismissed Calinou’s stale review September 26, 2023 12:43

Needs more work after discussion.

@myaaaaaaaaa myaaaaaaaaa closed this Oct 4, 2023
@myaaaaaaaaa myaaaaaaaaa deleted the png branch October 4, 2023 21:06
@AThousandShips AThousandShips removed this from the 4.x milestone Oct 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Image function save_png is extremely slow
8 participants