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

Add support for saving WebP images #61770

Merged
merged 1 commit into from
Jun 21, 2022
Merged

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Jun 7, 2022

Godot already has code to save WebP images, but it's not exposed to the user. This PR refactors this code, adding and exposing APIs for saving WebP images.

The bulk of the saving code already existed in image_loader_webp.cpp, it has been moved to a new file webp_common.cpp, which is used by both the loader and the new file resource_saver_webp.cpp. This mimics the structure of the PNG code.

In addition to moving this code, this PR also fixes a bug @lyuma and @Macksaur discovered where Godot was adding an extra WEBP prefix that made the saved file not be a valid WebP file. This bug wasn't affecting us before because Godot only used this code to save stuff internally and then we stripped that prefix when loading internally, but it's technically a pre-existing bug that resulted in compressed textures being invalid WebPs and 4 bytes bigger than they needed to be.

The exposed methods are in the Image class. They mimic the API for saving PNG files, except that WebP has both lossy and lossless so there is an extra parameter for the quality setting. Passing in true for the lossy parameter will save lossy, and the quality can be set to a value between 0.0 and 1.0. Users can write image.save_webp("res://test.webp") to save a lossless WebP file, or image.save_webp("res://test.webp", true) to save a lossy WebP file at 75% quality, or image.save_webp("res://test.webp", true, 0.5) to save a lossy WebP file at 50% quality.

Special thanks to @lyuma and @Macksaur. My work on this was sponsored by The Mirror.

@Macksaur
Copy link
Contributor

Macksaur commented Jun 7, 2022

I like the proposed API with default parameters, +1. It makes for concise code and I'll never need to remember what true / false do.

Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm approving this on a proposal standpoint, because webp is already one of godot accepted formats and is used widely for (Godot) lossless images.

It's open to technical review.

core/io/image.cpp Outdated Show resolved Hide resolved
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me overall, still have to do a closer review.

core/io/image.h Outdated
@@ -34,6 +34,7 @@
#include "core/io/resource.h"
#include "core/math/color.h"
#include "core/math/rect2.h"
#include "modules/modules_enabled.gen.h"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed anymore.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed in PR review meeting, we agreed with @reduz that the API should be:

save_webp(String path, bool lossy = false, float lossy_quality = 0.75)

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍

@akien-mga akien-mga merged commit c18d0f2 into godotengine:master Jun 21, 2022
@akien-mga
Copy link
Member

Thanks!

@aaronfranke aaronfranke deleted the webp branch June 21, 2022 16:04
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.

None yet

4 participants