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

Import: Cleanup and optimize etcpak compression method #47917

Merged
merged 1 commit into from
Apr 18, 2021

Conversation

akien-mga
Copy link
Member

@akien-mga akien-mga commented Apr 15, 2021

Avoid unnecessary allocation of temporary buffers for each mip, and redundant
Image create where we can assign the pointer directly.
Also renames variable and reorders code for clarity.

Clarify that squish is now only used for decompression.

Documented which formats can be decompressed in Image.

Follow-up to #47370.

Notes

Lossy quality

etcpak doesn't provide any options for configuring the quality. Reading the upstream README, it says:

etcpak is an extremely fast Ericsson Texture Compression and S3 Texture Compression (DXT1/DXT5) utility. Currently it's best suited for rapid assets preparation during development, when graphics quality is not a concern, but it's also used in production builds of applications used by millions of people.

i.e. it's not geared towards perfect quality, but then that's also aligned with our default lossy import with quality 0.7. That means that if some users eventually want higher quality, we might need to provide another way to encode their assets (possibly via a thirdparty plugin).

But it also states:

Why there's no image quality metrics? / Quality comparison.

And the image comparison does look fairly good to my untrained eye.

So now that we import ETC/ETC2 and S3TC through an importer that has no configurable quality settings, maybe we should consider removing the lossy_quality option?

The PVRTC importer we use also doesn't handle lossy_quality, so only cvtt handles it for BPTC.

Decompression

etcpak does provide support for decode ETC, ETC2, DXT1, and DXT5.

It's not part of the minimal Process* code we extracted from the library, but part of the BlockData.cpp which depends on etcpak's own Bitmap representation and everything else: task scheduler, modified libpng, etc.

https://github.com/wolfpld/etcpak/blob/1dc7b300453ae3d20cc43d906c89bcfcd8854cd3/BlockData.cpp#L694-L711

We could likely extract only the relevant Decode*Part methods and hook them with our own Image bitmap representation: https://github.com/wolfpld/etcpak/blob/1dc7b300453ae3d20cc43d906c89bcfcd8854cd3/BlockData.cpp#L723-L857

That would require either some copy-paste work which makes it harder to sync with upstream, or splitting the upstream BlockData.cpp into two files (one part with only the byte decoding, and another part with the higher level Bitmap stuff) and have them agree to merge it.

That would allow use to drop Squish (used to decode DXT) and add decoding support for ETC1/ETC2.

@akien-mga akien-mga added this to the 4.0 milestone Apr 15, 2021
@akien-mga akien-mga requested review from a team as code owners April 15, 2021 12:03
@akien-mga akien-mga force-pushed the squish-decompress-only branch 2 times, most recently from c53c789 to b269c9d Compare April 15, 2021 12:16
@akien-mga akien-mga requested a review from a team as a code owner April 15, 2021 12:16
@akien-mga akien-mga force-pushed the squish-decompress-only branch 2 times, most recently from 512426a to 3a603b4 Compare April 15, 2021 12:30
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 wanted to restore etc 1 and 2 decompression but that is another pr. Looks good from a textual review. I think g4 android doesn’t work.

@akien-mga akien-mga changed the title squish: Rename file, now only used for decompression Import: Cleanup etcpak compression, rename squish only used for decompression Apr 15, 2021
@akien-mga akien-mga force-pushed the squish-decompress-only branch 2 times, most recently from 67d4dea to b58795d Compare April 15, 2021 12:56
@akien-mga akien-mga changed the title Import: Cleanup etcpak compression, rename squish only used for decompression Import: Cleanup _compress_etcpak, clarify squish only for decompress Apr 15, 2021
@akien-mga akien-mga force-pushed the squish-decompress-only branch 2 times, most recently from 26f0fc8 to 793d70d Compare April 15, 2021 13:43
@akien-mga akien-mga changed the title Import: Cleanup _compress_etcpak, clarify squish only for decompress Import: Cleanup and optimize etcpak compression method Apr 15, 2021
@akien-mga akien-mga force-pushed the squish-decompress-only branch 2 times, most recently from c3b55d3 to 591762f Compare April 15, 2021 14:14
@akien-mga akien-mga marked this pull request as draft April 15, 2021 14:15
@akien-mga akien-mga marked this pull request as ready for review April 16, 2021 09:27
@akien-mga
Copy link
Member Author

I went back to a safer approach closer to the original code (each mip gets written to a temporary buffer and memcpy'ed) as I wasn't sure how to get the full size needed for the new image to pre-allocate it.

Doing so I found that I was mixing metrics of the base and dest images which could have explained the crash I was getting with the old method. I might poke at it further but in the meantime this version should be somewhat safer. It imports all textures from the TPS demo fine.

@akien-mga akien-mga force-pushed the squish-decompress-only branch 3 times, most recently from 2720593 to aee76ed Compare April 16, 2021 11:10
@akien-mga
Copy link
Member Author

I went back to a safer approach closer to the original code (each mip gets written to a temporary buffer and memcpy'ed) as I wasn't sure how to get the full size needed for the new image to pre-allocate it.

In the end I managed to find a way to avoid the temporary buffers. As in the original code, we create the new Image beforehand, so the data buffer is pre-allocated and we just need to write to it directly.
@fire told me the initial implementation was done with intermediate arrays so that it could run multi-threaded and be joined in the end, but since we'll now have threaded import of multiple resources in parallel with #47343, this is likely not needed.

@akien-mga
Copy link
Member Author

akien-mga commented Apr 16, 2021

Tested before/after with an optimized build to ensure that this PR actually helps or doesn't reduce performance.

Importing all .stex for TPS demo with an optimized build (production=yes target=release_debug).

  • Before: 0m49,701s 0m50,410s 0m50,549s
  • After: 0m58,678s 0m56,395s 0m57,866s

So it seems I'm actually losing some performance here, surprisingly. I'll have to look into it further.

Avoid unnecessary allocation of temporary buffers for each mip, and creates
only one Image with the compressed data.
Also renames variable and reorders code for clarity.

Clarify that squish is now only used for decompression.

Documented which formats can be decompressed in Image.
@akien-mga
Copy link
Member Author

Cleaned up further with advice from @reduz so we no longer need to create an intermediate Image to pre-allocate the right data. Image has all the relevant static methods to get the image size and mipmap offset and dimensions for a given width/height/format.

With these changes, I get the same performance before this PR and after on my hardware (~49-50 s). Hard to say if there's an actual speed gain as numbers fluctuate significantly depending on the start CPU load and heat on my laptop.

For the reference, to test I do:

  1. Import TPS demo fully one time, exit
  2. rm .godot/imported/*.stex -f
  3. time godot -e and I manually abort (Ctrl+C) once I reach 100%
  4. Repeat 2. and 3. with the other version being tested

@akien-mga akien-mga requested review from reduz and fire April 16, 2021 15:22
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.

The best test is that you said it imports tps correctly.

@akien-mga akien-mga merged commit dfe4feb into godotengine:master Apr 18, 2021
@akien-mga akien-mga deleted the squish-decompress-only branch April 18, 2021 15:00
@madmiraal madmiraal mentioned this pull request May 1, 2021
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.

2 participants