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

Improve memory usage for image import and PortableCompressedTexture2D #92179

Merged
merged 1 commit into from
May 22, 2024

Conversation

aaronp64
Copy link
Contributor

When importing images, we store a compressed version of the image to a .ctex file with ResourceImporterTexture::save_to_ctex_format. When importing many large images at once, this can use a large amount of memory, especially when the .ctex file uses WebP format.

This change is for ResourceImporterTexture::save_to_ctex_format to use the original Image object instead of p_image->get_image_from_mipmap(0), to avoid creating a copy of the full uncompressed image when looping through the base Image and mipmaps. This reduces the import memory usage for large images by around 10% when using WebP, and 35-40% when Project Settings/Rendering/Textures/Lossless Compression/Force PNG is enabled, may vary depending on the image and number of import threads running. Same change applied to PortableCompressedTexture2D::create_from_image, which has similar logic.

This helps with #92084, but does not fully resolve the issue on its own, as compressing with WebP on many threads can still use a large amount of memory - this just lowers that amount, and makes it more likely that enabling "Force PNG" will reduce memory usage enough to import the files.

When importing images, we store a compressed version of the image to a .ctex file with ResourceImporterTexture::save_to_ctex_format.  When importing many large images at once, this can use a large amount of memory, especially when the .ctex file uses WebP format.

This change is for ResourceImporterTexture::save_to_ctex_format to use the original Image object instead of p_image->get_image_from_mipmap(0), to avoid creating a copy of the full uncompressed image when looping through the base Image and mipmaps.  This reduces the import memory usage for large images by around 10% when using WebP, and 35-40% when Project Settings/Rendering/Textures/Lossless Compression/Force PNG is enabled, may vary depending on the image and number of import threads running.  Same change applied to PortableCompressedTexture2D::create_from_image, which has similar logic.

This helps with godotengine#92084, but does not fully resolve the issue on its own, as compressing with WebP on many threads can still use a large amount of memory - this just lowers that amount, and makes it more likely that enabling "Force PNG" will reduce memory usage enough to import the files.
@aaronp64 aaronp64 requested a review from a team as a code owner May 20, 2024 23:50
@fire
Copy link
Member

fire commented May 21, 2024

What's a good dataset to test this on?

@aaronp64
Copy link
Contributor Author

Any large image files should show a difference in memory usage (smaller files should use less memory too, but would probably be harder to see). I mostly tested with a 3811x4081 and a 7700x6300 file, using a bunch of copies of one of them when testing multiple import threads. Using the same file made memory comparisons easier, knowing that each thread was working on the same data, but it would probably be good to test with a larger set of files.

The PR that added lossless WebP encoding (#47835) mentioned a few projects that had images to test with, I can try testing with some of those tomorrow. Even if they're too small to see a memory difference, would be good to confirm the resulting .ctex files are identical before/after the code change.

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.

This looks good as a stopgap measure to help memory usage. That being said, I'm not sure how much of a challenge it would be to modify webp_lossless_packer to pack all mip levels from an image.

It seems to me the trouble comes from the fact that WebPCommon::_webp_lossless_pack only operates on the base mip level. So we end up copying the same memory several times:

  1. Our base image (fully in memory before compression)
  2. 1 copy of each mip level
  3. WebP creates 2 copies of each mip level at one point.
  4. WebP creates and returns 1 copy (this only overlaps with one of the previous copies)

Accordingly, we are keeping 1 copy of the entire image and 4 copies of each mip level in turn. This PR removes number 2 for the base mip level (which accounts for the most memory usage) but leaves the other levels alone. If we could extend _webp_lossless_pack to take a a specific mip level (or handle all mip levels) then we could remove 2 altogether and have a max of 3 copies at a time.

That being said, I think what I describe above is future work, as it will require some refactoring and will have less of an impact than what you have already done.

@akien-mga akien-mga modified the milestones: 4.x, 4.3 May 22, 2024
@akien-mga akien-mga merged commit 4c96dcf into godotengine:master May 22, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@aaronp64 aaronp64 deleted the image_import_memory branch June 4, 2024 19:46
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.

5 participants