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

BasisU: Update to 1.50.0 and add HDR support #97582

Merged
merged 1 commit into from
Oct 14, 2024

Conversation

BlueCube3310
Copy link
Contributor

@BlueCube3310 BlueCube3310 commented Sep 28, 2024

Updates BasisUniversal to 1.50.0 and adds support for transcoding to BC6/ASTC HDR 4x4 RGB.
If neither format is supported, the image gets decompressed to RGB9E5.

TODO:

  • Apply previous patches,
  • Use Godot's tinyexr,
  • Remove unused dependencies (qoi, tinydds)
  • Tune the settings to minimize the compression time,
  • Clean up the code,
  • Make the multiples-of-4 padding code account for RGBAF,
  • Do quality comparisons

Quality:
Image: https://polyhaven.com/a/empty_play_room

BasisU vs. Betsy (diff) Basisu vs. Original (diff)
vsbetsy basisu

@BlueCube3310 BlueCube3310 force-pushed the basisu-hdr branch 3 times, most recently from ffb5eee to 170ca0b Compare September 28, 2024 11:15
@AThousandShips AThousandShips added this to the 4.x milestone Sep 28, 2024
@BlueCube3310 BlueCube3310 force-pushed the basisu-hdr branch 2 times, most recently from b630f76 to cf4b3d6 Compare September 29, 2024 08:38
@BlueCube3310 BlueCube3310 marked this pull request as ready for review September 29, 2024 08:39
@BlueCube3310 BlueCube3310 requested review from a team as code owners September 29, 2024 08:39
@BlueCube3310
Copy link
Contributor Author

BlueCube3310 commented Sep 29, 2024

I think this is more or less complete, on an optimized Windows editor build it adds ~250kb, this should be minimized by #85321.

The only big issue are the compression times, on an optimized build it takes ~21s to compress an 8k HDRi without mipmaps with the default settings. The latest push uses the 'fastest' preset which I haven't tested yet, so that may have improved a bit.

@fire
Copy link
Member

fire commented Sep 29, 2024

17 seconds sounds like our previous times without gpu compute, not sure how much we can squeeze to make it faster.

@BlueCube3310
Copy link
Contributor Author

BlueCube3310 commented Sep 29, 2024

CVTT would take about 60-70s for the same task, so this is about 3x faster (though obviously still not desired)

Edit: It now takes ~15s, so changing the settings actually results in some improvement.

Edit2: Verbose mode now prints the encoding/transcoding times.

@fire
Copy link
Member

fire commented Oct 3, 2024

There are three missing items on the list left to go!

  • Remove unused dependencies (qoi, tinydds)
  • Tune the settings to minimize the compression time,
  • Do quality comparisons

@BlueCube3310
Copy link
Contributor Author

I don't think the performance can get any better (without SSE4/AVX), since the lowest settings are already used.
Quality-wise, it's comparable to Betsy.
Transcoding to ASTC should be nearly instant (BasisU HDR is just ASTC), so slowdowns are not expected on mobile. BC6 is slightly slower, though not really noticeable on Desktop hardware.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally, it works as expected.

Optimized, stripped Linux x86_64 editor binary size is about 300 KB larger (this is with #85321 taken into account). Release export template is 86 KB larger.

Benchmark

Performance on LDR texture imports seems to be roughly identical. Here's an exercept on a few 4K textures:

Before

EditorFileSystem: "res://blue_metal_plate_disp_4k.jpg" import took 439 ms.
EditorFileSystem: "res://blue_metal_plate_diff_4k.jpg" import took 1094 ms.
EditorFileSystem: "res://blue_metal_plate_arm_4k.jpg" import took 1438 ms.
EditorFileSystem: "res://blue_metal_plate_nor_gl_4k.jpg" import took 1495 ms.

After

BasisU: Encoding a 4096x4096 image with 12 mipmaps took 371 ms.
EditorFileSystem: "res://blue_metal_plate_disp_4k.jpg" import took 449 ms.

BasisU: Encoding a 4096x4096 image with 12 mipmaps took 776 ms.
EditorFileSystem: "res://blue_metal_plate_diff_4k.jpg" import took 1157 ms.

BasisU: Encoding a 4096x4096 image with 12 mipmaps took 828 ms.
EditorFileSystem: "res://blue_metal_plate_arm_4k.jpg" import took 1256 ms.

BasisU: Encoding a 4096x4096 image with 12 mipmaps took 1193 ms.
EditorFileSystem: "res://blue_metal_plate_nor_gl_4k.jpg" import took 1503 ms.

Quality

Click to view at full size. Texture size was limited to 1024 on import to make the difference more noticeable.

Lossless VRAM Compressed Low Quality
Screenshot_20241004_203403 png webp Screenshot_20241004_203409 png webp
VRAM Compressed High Quality Basis Universal
Screenshot_20241004_203416 png webp Screenshot_20241004_203423 png webp

@akien-mga akien-mga modified the milestones: 4.x, 4.4 Oct 4, 2024
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, just missing this diff for vendoring instructions:

diff --git a/thirdparty/README.md b/thirdparty/README.md
index c9a0cefb05..bbf4bcecb7 100644
--- a/thirdparty/README.md
+++ b/thirdparty/README.md
@@ -64,7 +64,8 @@ Files extracted from upstream source:
 
 Files extracted from upstream source:
 
-- `encoder/` and `transcoder/` folders, minus `jpgd.{cpp,h}`
+- `encoder/` and `transcoder/` folders, with the following files removed from `encoder`:
+  `jpgd.{cpp,h}`, `3rdparty/{qoi.h,tinydds.h,tinyexr.cpp,tinyexr.h}`
 - `LICENSE`
 
 Applied upstream PR https://github.com/BinomialLLC/basis_universal/pull/344 to

Regarding QOI support, we have #91263 so if it gets approved we could consider re-enabling support for that, if it's deemed useful.

@BlueCube3310
Copy link
Contributor Author

Regarding QOI support, we have #91263 so if it gets approved we could consider re-enabling support for that, if it's deemed useful.

BasisU's image loaders are never used anyway, nor are they exposed to the user; this would only simplify the patches.

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Changes to modules/basis_universal look good to me!

@Repiteo Repiteo merged commit 56ed76a into godotengine:master Oct 14, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Oct 14, 2024

Thanks!

@BlueCube3310 BlueCube3310 deleted the basisu-hdr branch October 18, 2024 09:14
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.

7 participants