vpenades / SharpGLTF

glTF reader and writer for .NET Standard
MIT License
458 stars 72 forks source link

MergeBuffers causing "Destination array was not long enough." in _StaticBufferBuilder #101

Closed xPaw closed 3 years ago

xPaw commented 3 years ago

Original issue: https://github.com/SteamDatabase/ValveResourceFormat/issues/346

We call the code here: https://github.com/SteamDatabase/ValveResourceFormat/blob/b277571cd47b5f41f94982170cb07e45c6c5e04a/ValveResourceFormat/IO/GltfModelExporter.cs#L438-L446

It's throwing here: https://github.com/vpenades/SharpGLTF/blob/3c611c63f6f1cb2c9cd988faa4e01e53b11cc095/src/SharpGLTF.Core/Schema2/gltf.BufferView.cs#L386

I tested with WriteSettings.MergeBuffers = false and it exports, it also seems to not happen with other models.

I don't know if this is a mistake on our end or what, but we don't call that Save multiple times, so this shouldn't be a threading issue. That particular model has 732 buffers, so it could be something to do with that.

Stacktrace:

System.ArgumentException: Destination array was not long enough. Check the destination index, length, and the array's lower bounds. (Parameter 'destinationArray')
   at System.Array.Copy(Array sourceArray, Int32 sourceIndex, Array destinationArray, Int32 destinationIndex, Int32 length, Boolean reliable)
   at System.Array.Copy(Array sourceArray, Int32 sourceIndex, Array destinationArray, Int32 destinationIndex, Int32 length)
   at System.SZArrayHelper.CopyTo[T](T[] array, Int32 index)
   at System.Collections.Generic.List`1.InsertRange(Int32 index, IEnumerable`1 collection)
   at SharpGLTF.Schema2._StaticBufferBuilder.Append(Byte[] data)
   at SharpGLTF.Schema2.BufferView._IsolateBufferMemory(_StaticBufferBuilder targetBuffer)
   at SharpGLTF.Schema2.ModelRoot.MergeBuffers()
   at SharpGLTF.Schema2.WriteContext._PreprocessSchema2(ModelRoot model, Boolean imagesAsBufferViews, Boolean mergeBuffers)
   at SharpGLTF.Schema2.WriteContext.WriteTextSchema2(String baseName, ModelRoot model)
   at SharpGLTF.Schema2.ModelRoot.SaveGLTF(String filePath, WriteSettings settings)
   at SharpGLTF.Schema2.ModelRoot.Save(String filePath, WriteSettings settings)
   at ValveResourceFormat.IO.GltfModelExporter.WriteModelFile(ModelRoot exportedModel, String filePath) in ValveResourceFormat\IO\GltfModelExporter.cs:line 445
   at ValveResourceFormat.IO.GltfModelExporter.ExportToFile(String resourceName, String fileName, Model model) in ValveResourceFormat\IO\GltfModelExporter.cs:line 293

Version: SharpGLTF.Toolkit 1.0.0-alpha0022

vpenades commented 3 years ago

Ok, I'll give it a look as soon as I can. Right now I'm working on another issue, so it will take a few days.

Additionally, it's going to be very hard to find unless a test case, or an offending model is provided.

xPaw commented 3 years ago

so it will take a few days.

No worries, there is no rush of course.

What would be the easiest way to share the model with you?

That particular model is from https://steamcommunity.com/sharedfiles/filedetails/?id=928179372 (it's over 300 megs), and I can see how it's not easy to get hold of that if you don't use steam.

vpenades commented 3 years ago

hmmm... I have a steam account, but I barely use it; I tried but I've been unable to download that file

vpenades commented 3 years ago

@xPaw looking again at the exception call stack, I've realized the exception is being thrown within the List .AddRange method... and I would be surprised there's a bug in there.

So, the only thing that makes sense to me, given the size of the model, is that at some point, the List needed to resize its internal buffer, failed to allocate the required memory and threw the exception. (but not an outofmemory exception which is what you would expect in this case)

Notice that in order to merge the buffers it needs to reallocate all the buffers into a single, very large memory chunk.

Questions:

Are you compiling the code for 32bits?

which was the size of the _Data list when the exception happened? and the size of the data being added?

Additionally, Looking for that exception, I've found several threads reporting similar issues when calling List methods from multiple threads, but I don't think this is the case, but if it gives you a lead...

xPaw commented 3 years ago

I am compiling for 64bit.

Unfortunately my VS seems somewhat broken, and I can't properly debug library code (it just refuses to load code from symbols).

image

I did manage to grab that, and it does look like it actually runs over int32.MaxValue?

So sounds like a better structure for this big buffer is needed (or perhaps use IEnumerable so that it's not copied into one big blob?)

vpenades commented 3 years ago

Well... it looks like it's a lot of data you're willing to put into a single buffer.

As far as I know, glTF schema doesn't support buffers larger than what can be indexed by a 32 bit value. I guess it's designed that way to simplify client development.

I think a long time ago it was discussed, and rejected (by the khronos board) to support such large buffers.

So even if I modify the library to allow 64 bit indexing (which woul be a huge change library wide), the written models would be unloadable by many clients.

My suggestion would be to use this code snippet:

long totalSize = ModelRoot.LogicalBuffers.Sum(buffer => (long)buffer.Content.Length);

before attempting to merge the buffers. If the size is larger than a safe value, then disable buffer merging.

Now, don't use int.MaxValue as the threshold value, because List increases its internal size by growing steps, in fact I have no clue which would be the safe value, you'll have to do some trial & error.... but I would bet a safe value is just 1Gb (which is already a VERY LARGE buffer)

This could have been addressed if GLB would allow for multiple buffers, again, this is something that has been discussed and requested in the khronos board.... the fact that GLB only supports a single buffer is a huge problem because it doesn't allow seamless conversion between glb and gltf

xPaw commented 3 years ago

Ahh yeah, that would be one reason for failure.

From the spec: https://github.com/KhronosGroup/glTF/blob/master/specification/2.0/README.md#buffers-and-buffer-views

It sounds like the 32bit limit is only for GLB, so that's worth keeping in mind.

I did disable merging buffer using your suggestion, so that issue is solved in this regard. Is this anything SharpGLTF may improve (in terms of throwing a validation error)?

EDIT: With the 32bit limit, shouldn't it at least be uint32?

vpenades commented 3 years ago

The difference between glTF and GLB is that glTF can have multiple buffers, and GLB only one. But each individual buffer is limited to what can be addressed by a 32bit value.

Furthermore, .NET uses a SIGNED Int32 value for most addressable collections. I'ts widely used in Arrays, Lists, ArraySegments, Spans, etc.

This means that in practice, the number of bits that can be used for addressing is just 31 bits, which means only 2Gb can be addressed.

And again, I suspect many implementations might have the same issue, so it could be said the largest a buffer can grow without risking having trouble is just 2Gb.

Allowing SharpGLTF to grow up to 4GB might require changing lots of Ints by UInts, and writing custom collections that supports large buffers. Just replacing List by a LargeList looks like a lot of work.

My rule of thumb would be: whenever a glTF is bigger than 1gb, store it as a zipped glTF, instead of a GLB

vpenades commented 3 years ago

@xPaw In https://github.com/vpenades/SharpGLTF/commit/4788e4715edffa5fc6a3027950964dd06fa14d13 I've just added a few improvements to handle scenarios with very large models.

When merging to a single buffer, it checks whether the total sum of buffers is less than 2Gb, otherwise it throws a NotSupportedException.

Also I've added new write settings to support merging the buffers up to a given size, splitting the resources into multiple bigger but valid sizes. This allows writing very large models, while keeping a short number of binary files.

An example of that configuration can be found here.

So, right now the approach to write a GLB would be:

Let me know if this allows you to handle these large models.