Solve issue #2127 (excessive memory use while loading GLB)

[tonihele]

Ok, GLB with the textures included easily causes LWJGL to run out of memory. As we might upload the same texture many times (we also read it many times...). 32Gb of RAM is nothing then. Also slowdowns and if one is into geometry batching.. no can do.

So this caches all the materials read to re-use them. Note that this affects both GLTF and GLB.

Resolves #2127

[tonihele]

Hmm, maybe I'll change the cache to the material read method. As it seems that those others do the same. They cache and no-one needs to know.

[oxplay2]

yes, seems like it will be better.

cache is per load as i see, then cleared.

[tonihele]

One question though. Should we return material.clone() from the cache? Because a geometry might have vertex colors set and that would affect the material. Returning a clone still fixes the original issue...

[oxplay2]

Wait a moment.

But then why GLTF had no 32 GB memory issue?

Both GLB and GLTF use readMaterial right?
While only GLB had issue with multipled materials.

Maybe this should not be readMaterial as a place then?

Because this change affect also GLTF that had no issue like this.

For me this sounds like this loader already implement cache correctly(at least for textures), just not for GLB.

[tonihele]

Wait a moment.

But then why GLTF had no 32 GB memory issue?

Both GLB and GLTF use readMaterial right?
While only GLB had issue with multipled materials.

GLTF/GLB read materials as many times they are referenced. This is fine, until there is also a texture embedded. Like real binary, not a file reference. Then this texture is read each time to the assetManager with loadAssetFromStream which doesn't cache. This is the source of the problem. So there are going to be so much textures in the memory.

It is easier to cache the material in this case in my opinion. It doesn't interfere with non embedded textures (normal GLTF). It is less hassle in this case too. I didnt measure the perfomance of our Material cloning vs reading it all the time again from bytes (latter looks like more expensive operation).

[oxplay2]

Ok, so i understand that GLTF have "sourceindex" that can know what material texture index it is, while GLB do not?

For sure you know this loader better than me, just thought it can be cached on texture level, not material for GLB.

But if its not possible then yes, lets keep on material level, but need clone materials

[tonihele]

Ok, so i understand that GLTF have "sourceindex" that can know what material texture index it is, while GLB do not?

GLB/GLTF are identical. But GLB can contain embedded binary textures. GLTF contains only URLs to textures. Both have indices for textures and materials.

I just guess that original author didn't realize that the assetManager used like that doesn't cache. Creates a new texture with the same key...

[oxplay2]

        if (uri == null) {
            assertNotNull(bufferView, "Image " + sourceIndex + " should either have an uri or a bufferView");
            assertNotNull(mimeType, "Image " + sourceIndex + " should have a mimeType");
            byte[] data = (byte[]) readBuffer(bufferView, 0, -1, null, 1, VertexBuffer.Format.Byte);
            String extension = mimeType.split("/")[1];
            TextureKey key = new TextureKey("image" + sourceIndex + "." + extension, flip);
            result = (Texture2D) info.getManager().loadAssetFromStream(key, new ByteArrayInputStream(data));

        } else if (uri.startsWith("data:")) {
            // base64 encoded image
            String[] uriInfo = uri.split(",");
            byte[] data = Base64.getDecoder().decode(uriInfo[1]);
            String headerInfo = uriInfo[0].split(";")[0];
            String extension = headerInfo.split("/")[1];
            TextureKey key = new TextureKey("image" + sourceIndex + "." + extension, flip);
            result = (Texture2D) info.getManager().loadAssetFromStream(key, new ByteArrayInputStream(data));
        } else {
            // external file image
            String decoded = decodeUri(uri);
            TextureKey key = new TextureKey(info.getKey().getFolder() + decoded, flip);
            Texture tex = info.getManager().loadTexture(key);
            result = (Texture2D) tex;
        }

Ok i think i understand.

So the main reason why GLTF were working is that "loadTexture" cache, while every other case like "loadAssetFromStream" do not cache.

Then why not make GLB work same as GLTF by enabling assetmanager cache?

As i see other options also have TextureKey with index/name/etc of texture so it should be able to cache right?

[tonihele]

Yep, you got it.

Then why not make GLB work same as GLTF by enabling assetmanager cache?
is it possible to cache in assetManager by name or anything or only uri?

I guess it would be doable by:

if(info.getManager(getFromCache(key)) != null) {
...}
texture = (Texture2D) info.getManager().loadAssetFromStream(key, new ByteArrayInputStream(data));
info.getManager().addToCache(key, texture);

But I don't know if it is the same cache. I think normally they go to the weak cache, but this might not be it. I am not an expert on this matter.

[oxplay2]

Im also unsure if it will be same cache. (well it should, but we need be sure, yes)

Then i guess we should ask on HUB maybe :) or mention someone here to help.

[tonihele]

Im also unsure if it will be same cache. (well it should, but we need be sure, yes)

Then i guess we should ask on HUB maybe :) or mention someone here to help.

Well, I read the code. It is the same cache. It is governed by the assetKey cacheType property, which for textureKeys defaults to WeakRefCloneAssetCache.

[tonihele]

So I guess that is all doable. My personal preference is the material cloning though. But I can go either way. Someone just make the call then :)

[oxplay2]

i rethink this a little. (ye i know, crazy, sorry ;p)

Lets say we have 2 Models.

AssetManager cache textures per "game run" not per "model load"

So lets say Both of them use "MyTexture.png", but one as file and second as GLB "binary embeded"

Now if binary embeded one will load first, and will be little different than file based one, the next GLTF model will have "mismatch texture"

So i think we should implement "per-load" cache, for texture for both:

if (uri == null) {

and

} else if (uri.startsWith("data:")) {

only.

No material cache, No assetmanager cache, but local cache only for binary textures in this exactly places.

Second way is to cache in assetmanager but with some very specific prefix/index, tho still riscy.

In perfect world, assetmanager should cache exact file, so i think we should go first solution to cache per loading for GLB(binary embeded textures) only.

[tonihele]

It is all about the asset keys. The embedded ones will not get unique keys, file ones have basically unique keys. Well not unique but all will point to the same file so it is fine if another model uses that.

This material cloning will work since the embedded textures wont be cached as their uniqueness can't be vouched for anyway.

But if you load the same model multiple times, with embedded texture... you will get multiple (same) textures loaded. So... Kinda could figure a unique name for them, if possible, and then cache them to asset manager as well...

[oxplay2]

But if you load the same model multiple times, with embedded texture... you will get multiple (same) textures loaded. So... Kinda could figure a unique name for them, if possible, and then cache them to asset manager as well...

Yes if this would be possible that cache would work for exact GLB file, but not image file or another GLB file with same texture name, then i think it would be fine.

[tonihele]

To get close to unique name for the embedded textures... I guess they could contain the URI, same as the actual file references. But here the file is the current GLB file, this can be extracted from AssetInfo.AssetKey.

[oxplay2]

sounds good. GLB file URI + texture URI(or whatever glb store as name/index)?

Ofc still GLTF advantage is its files might use same tex file, while 2 GLB files using same texture 2x times is 2x time more memory usage. But i see no solution for this, because it might break if we cache under same name. Also anyway i belive its similar for j3o?

[tonihele]

Well... Lol, this doesn't really work. I mean, I totally did it. And wondered why it doesn't trigger... Because if one loads the whole model the second time, it comes from... the... cache. And it is just cloned. So I suspect that this is just all very fine with the material caching.

[pspeed42]

The URI proposal is very similar to have Java JAR files work with internal file URIs.

re: "while 2 GLB files using same texture 2x times is 2x time more memory usage."

...yeah, because without computing some kind of hash, there is no way to know that they are the same texture. Name is not enough.

For lots of reasons, I've switched to using gltf exclusively here... way easier to debug if something goes wrong. But yeah, it would be nice if glb can work better than it currently does.

[tonihele]

I loaded the BistroExterior.glb (960Mb, with 360Mb of textures embedded) 100 times to a scene and it looks to be fine. Textures are shared. FPS is terrible :)

[tonihele]

But yeah, it still needs to cache the textures locally. If some textures are shared between the materials. But I think this is better done with the local cache than the AssetManager cache. Since reading the texture data (from memory!) is still somewhat slow. I'll optimize this reading with the other ticket I made.

I'll just cache the textures locally and that I think covers all the cases, right?

These embedded textures are unique to the GLB model, and the model itself is cached anyway in the AssetManager for future use.

[oxplay2]

ye, just cache textures for GLB locally (leave loadTexture case of GLTF because it cache anyway) and it should be done.

Ofc i know that GLB enthusiasts would prefer GLB be better than GLTF, but it seems to be opposite.

[tonihele]

Should be good to go!

[stephengold]

I re-read the entire discussion just now and was impressed by all the thought and care that went into this pull request. Thank you to all who participated!

Unless there's further substantial discussion, I plan to integrate this PR in about 48 hours.

[oxplay2]

Looking at code it seems fine. GLTF or GLB store same textures as indexes, so materials just use same indexes, and code here just cache by indexes, so it should work fine.

While one line "dataStream.close();" removal should be fine, since based on stackoverflow "DataInputStream holds no system resources of its own, so not closing it will not cause any leaks. You can simply leave it open. Alternatively you can return it from the method so that the caller can close it."

So from my perspective this PR is ready, tho it would be cool if someone else would look too.

This file in general have missing JavaDocs, but it have nothing to do with this fix.

[tonihele]

The stream is closed. Previously if failed, it was never closed. I fixed this failing case. I used Java feature called try-with resource. Which does the try finally if not null then close thing but cleaner syntax. Im sorry I explained it poorly the first time.

…

On Thu, 26 Oct 2023, 12:20 oxplay2, ***@***.***> wrote: Looking at code it seems fine. GLTF or GLB store same textures as indexes, so materials just use same indexes, and code here just cache by indexes, so it should work fine. While one line "dataStream.close();" removal should be fine, since based on stackoverflow "DataInputStream holds no system resources of its own, so not closing it will not cause any leaks. You can simply leave it open. Alternatively you can return it from the method so that the caller can close it." So from my perspective this PR is ready, tho it would be cool if someone else would look too. — Reply to this email directly, view it on GitHub <#2128 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB7VJPQAOB6RV7ASHP4KNCTYBITMVAVCNFSM6AAAAAA6MOZQNWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOBQG4ZTGOJZG4> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[tonihele]

And we should always close all the streams, we code for the interface. We don't need to understand how the stream actually works for this. (Auto)closeable needs closing. Even if it is bytearraystream or some other NOP close stream.

Here is a link to try-with resource info which explains this far better than me: https://www.baeldung.com/java-try-with-resources

[stephengold]

Unless there's further substantial discussion, I plan to integrate this PR in about 24 hours.

[stephengold]

Thank you @tonihele for the solution and @oxplay2 for code review.