Allow handling large external buffers #396
Conversation
src/accessor/util.rs
Outdated
| let start = view.offset(); | ||
| let end = start + view.length(); | ||
| let start = usize::try_from(view.offset()).ok()?; | ||
| let end = usize::try_from(start as u64 + view.length()).ok()?; |
There was a problem hiding this comment.
Isn't this going to fail on 32bit systems?
There was a problem hiding this comment.
Yes, this will fail on 32bit systems when the buffer is too large to fit into the 32bit address space, which seems fair to me. I guess technically the part of the buffer to be accessed in this function specifically may fit, but I'm not sure that edge case is worth handling. Would you like to handle it differently?
src/buffer.rs
Outdated
| @@ -91,8 +92,8 @@ impl<'a> Buffer<'a> { | |||
| } | |||
|
|
|||
| /// The length of the buffer in bytes. | |||
| pub fn length(&self) -> usize { | |||
| self.json.byte_length as usize | |||
| pub fn length(&self) -> u64 { | |||
There was a problem hiding this comment.
This is just going to lead to casts every time it is used. I think usize was the right choice. Buffers exceeding u32::MAX length become problematic on 32bit systems. I don't think you can even address a slice in Rust beyond u32::MAX on 32bit systems.
I agree that internally, the length should be stored as u64 but I think the offsets and lengths should be reported as usize.
There was a problem hiding this comment.
Loading the gltf file should fail on 32bit systems if there are buffers with lengths larger than u32::MAX.
There was a problem hiding this comment.
Hmm, I figured it's worth preserving the actual information losslessly as long as possible so users of the interface can decide how they want to deal with it. Do you want to truncate the information here by casting to usize, which may lead to unexpected behaviour, or should this function be fallible?
There was a problem hiding this comment.
An application might want to be able to access a glTF that is too big to fit into memory …
- as a viewer or editor that has a use for showing metadata to the user even without rendering;
- because they will be loading only selected objects from the glTF file (e.g. the scene the user selects, or selected “layers” of a complex dataset visualization; or
- to process a large buffer in a streaming fashion.
Therefore, it's not necessarily the case that loading a glTF containing buffers that won't fit into memory is useless. Note that if a user wants a usize they can call usize::try_from() on the u64 and the result is precisely either a usize or an error that they can .unwrap() or ? to fail on. (But perhaps there's an even better API possible; I haven't looked at the bigger picture.)
There was a problem hiding this comment.
@aloucks Do you have an update on how you would prefer this to be handled? It sounds like you'd like to fail when loading a glTF file that has any buffer that is too large and then just hard cast to usize here later? The use case of working with larger files on 32 bit systems is not worth supporting?
There was a problem hiding this comment.
I'm tempted to settle on usize as 32 bit systems are the exception rather than the norm nowadays. The WASM runtime however is a popular target for this library and 64 bit addressing is unstable. For this reason, I think it's important the user can handle this scenario gracefully somehow.
For the majority of users running on 64 bit systems, the cast from u32 to usize is already a bit annoying, so I suggest few things:
- Create a new type
struct Address(u64)(the name is debatable) in gltf-json. - Implement
ValidateforAddressand report an error on 32 bit systems if it exceeds theusizerange. - In the main/wrapper crate, change the return type for existing
u32offsets/sizes tousize, usingasto cast internally.
This should cover all of the points mentioned previously. 32 bit systems will still be able to process large glTF (and the wrapper via. from_json_without_validation) or report to the user the out of range size otherwise.
@ShaddyDC, would you like to implement this yourself? I can chip in and push to this branch if you like otherwise.
There was a problem hiding this comment.
That seems like a good way to go about it. I can take a shot at an implementation, but if you've got the time to do it, I'd appreciate if you could take care of it. I'm not yet familiar with the validation implementation.
There was a problem hiding this comment.
I've opened a PR on your fork: ShaddyDC#1
There was a problem hiding this comment.
Lgtm, merged! Though I think you should also have commit access
|
@alteous thanks for merging this! When are you planning to do the next release? |
|
Likely today or this weekend. |
Closes #393
I've added a new error type for it to make failures explicit, but you could arguably also just use
as usizeand live with potentially weird behaviour when large values are truncated. Presumably, in those cases you'd crash anyway when trying to load the data into memory, though I haven't looked into it more.I've also silenced some warnings in a separate commit by hiding them behind a
cfgtag because they annoyed me during development, but I'm happy to remove those changes if you prefer.