Skip to content

Expose near lossless WebP encoding#67918

Draft
DeeJayLSP wants to merge 1 commit intogodotengine:masterfrom
DeeJayLSP:near_lossless_webp
Draft

Expose near lossless WebP encoding#67918
DeeJayLSP wants to merge 1 commit intogodotengine:masterfrom
DeeJayLSP:near_lossless_webp

Conversation

@DeeJayLSP
Copy link
Copy Markdown
Contributor

@DeeJayLSP DeeJayLSP commented Oct 26, 2022

Bugsquad edit removed, as it is now invalid.
I still intend to use this PR for the implementation, so I ask to keep it open.

Adds a checkable option to texture importers called Near Lossless, when Mode is set to Lossy:

image

It also affects other Lossy importers:

image

When checked, importing will use the same workflow as Lossless, but the value of Lossy Quality will be passed to config.near_lossless, which decreases image quality in order to reduce its size (while keeping RGB(A) color space instead of using lossy's YUV(A)420) when below 100. This is a built-in feature in libwebp.

A few things to note:

  • WebP Compression Level project setting will affect images imported as Near Lossless. Overhaul WebP packer and split compression options #67948 kinda solves this.
  • The image will be imported as true lossless if Lossy Quality is set to 1.0 (config.near_lossless' default is 100) or the image being imported is too simple.

@DeeJayLSP DeeJayLSP requested review from a team as code owners October 26, 2022 19:06
Comment thread modules/webp/webp_common.cpp Outdated
@Calinou
Copy link
Copy Markdown
Member

Calinou commented Oct 26, 2022

Out of curiosity, how does Near Lossless differ from Lossless Lossy quality 100 in practice? This should be documented in the class reference.

@Calinou Calinou added this to the 4.0 milestone Oct 26, 2022
@DeeJayLSP
Copy link
Copy Markdown
Contributor Author

DeeJayLSP commented Oct 26, 2022

Out of curiosity, how does Near Lossless differ from Lossless quality 100 in practice? This should be documented in the class reference.

It doesn't. Near Lossless at 100 is true lossless. Maybe you meant Lossy quality 100?

Lossy encodes images using YUV420, while Near Lossless and Lossless use RGB. That's the main difference.

@DouglasNPD DouglasNPD requested a review from a team as a code owner October 26, 2022 19:55
@DeeJayLSP DeeJayLSP changed the title Add near lossless WebP encoding Expose near lossless WebP encoding Oct 26, 2022
@Calinou
Copy link
Copy Markdown
Member

Calinou commented Oct 26, 2022

I meant Lossy indeed.

Lossy encodes images using yuv420/yuva420p, while Near Lossless and Lossless use RGB/RGBA. That's the main difference.

I see, that makes sense. I've used lossy quality 100 for godot-docs images that were too large to use lossless compression, and it still looks pretty good while having file sizes smaller than JPEGs with lower quality.

Comment thread doc/classes/PortableCompressedTexture2D.xml Outdated
@DeeJayLSP
Copy link
Copy Markdown
Contributor Author

DeeJayLSP commented Oct 26, 2022

Despite the fact it works, I didn't like how messy the code came out. May do a complete refactor later by using helper functions that will likely reduce diff.

@DeeJayLSP
Copy link
Copy Markdown
Contributor Author

DeeJayLSP commented Oct 27, 2022

Refactor done.

The main functionality of the webp lossless function was turned into a helper function, while the original calls this helper by providing a near lossless value of 1.0 (actually lossless).

The lossy function, that previously called the modified lossless function, now calls the helper.

Everything that was changed to match the new lossless function was reverted.

@DeeJayLSP DeeJayLSP requested a review from fire October 27, 2022 00:37
fire
fire previously approved these changes Oct 27, 2022
Copy link
Copy Markdown
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.

Looks good.

As per custom:

  • needs a contributor review
  • needs style and architecture review

Comment thread doc/classes/PortableCompressedTexture2D.xml Outdated
mhilbrunner
mhilbrunner previously approved these changes Oct 28, 2022
Copy link
Copy Markdown
Member

@mhilbrunner mhilbrunner left a comment

Choose a reason for hiding this comment

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

The feature is IMO nice, the code looks good and I spotted no style issues or problems and is fairly straightforward. I left a few nitpicks as a suggestion for the documentation above. Thanks for contributing!

@DeeJayLSP
Copy link
Copy Markdown
Contributor Author

DeeJayLSP commented Oct 28, 2022

I have changed the helper function a bit to better match my follow-up PR.

WebPConfig.quality is a float value, but WebPConfig.near_lossless is an integer.

@DeeJayLSP DeeJayLSP requested a review from mhilbrunner October 28, 2022 11:47
@DeeJayLSP
Copy link
Copy Markdown
Contributor Author

DeeJayLSP commented Oct 28, 2022

I'm not thinking about closing this in favor of #67948 as its parts are more obvious, and also sort of fixes compression level (which is now replaced) affecting near lossless directly.

But changes done there may not be approved like this one.

This PR will be on wait for #67948 (and maybe some changes by reduz) to be done, as it's better to be done on top of it.

@akien-mga akien-mga removed this from the 4.0 milestone Nov 1, 2022
@akien-mga akien-mga added this to the 4.x milestone Nov 1, 2022
@clayjohn clayjohn dismissed stale reviews from fire and mhilbrunner November 10, 2022 17:56

stale, OP has further plans for PR

@DeeJayLSP DeeJayLSP marked this pull request as draft February 11, 2023 03:21
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.

6 participants