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

Referencing glb-stored buffer #828

Closed
lexaknyazev opened this issue Feb 3, 2017 · 32 comments
Closed

Referencing glb-stored buffer #828

lexaknyazev opened this issue Feb 3, 2017 · 32 comments

Comments

@lexaknyazev
Copy link
Member

With deprecating KHR_binary_glTF and removing string-based IDs from the core spec, we need to find a proper way to refer to buffer, stored in .glb file after JSON.

.glb layout:
image

glTF 1.0 way to reference "binary data":

{
  "buffers": {
    "binary_glTF": {
      "uri": "data:,",
      "byteLength": 123456
    }
  },
  "bufferViews": {
    "bufferView_0": {
      "buffer": "binary_glTF",
      "byteLength": 123
    }
  }
}

With glTF 2.0 we can't use magic "binary_glTF" ID anymore. Also we can't assume that it's the only buffer used.

Also, previous usage of DataURI isn't quite correct: "data:," means "empty text/plain string" according to RFC.

As one option, we could define a custom URI scheme like this (just possible example):

{
  "buffers": [
    {
      "uri": "glb://#buffer",
      "byteLength": 123456
    }
  ],
  "bufferViews": [
    {
      "buffer": 0,
      "byteLength": 123
    }
  ]
}

As another option, we could refine schema of buffer object.

Make URI optional:

{
  "buffers": [
    {
      "byteLength": 123456
    }
  ]
}

Or make URI nullable:

{
  "buffers": [
    {
      "uri": null,
      "byteLength": 123456
    }
  ]
}

Or add new boolean field:

{
  "buffers": [
    {
      "useGlb": true,
      "byteLength": 123456
    }
  ]
}

@pjcozzi @javagl
What do you think?

@bghgary
Copy link
Contributor

bghgary commented Feb 3, 2017

Also we can't assume that it's the only buffer used.

I would prefer if a glb file is completely self-contained and cannot reference other URIs. This would make it much simpler for native clients to handle files on disk (just like a JPEG file) since they will not have to deal with the network stack or other file references. There are security implications with this as well since sandboxed applications will only be able to access the single file that is chosen by the user and not all the references. Sandboxed applications may not have access to the internet at all, depending on application permissions.

In short, GLB will be the on-disk self-contained file while glTF will be for transmission across the web or other services.

If we agree to that, then you can assume there is only one buffer.

@lexaknyazev
Copy link
Member Author

there is only one buffer

OK, let's evaluate such setup, too. We can:

  • Keep one element in buffers array and choose one of schemes above, OR
  • Remove buffers array, and make bufferView.buffer field undefined (i.e., forbidden) in this case.

@javagl
Copy link
Contributor

javagl commented Feb 3, 2017

The uri is already optional for image, so making it optional for buffer would be the most consistent solution, IMHO. (Making it nullable could also be OK. A flag like useGlb would increase the size of the state space (what should happen when there is a uri and useGlb:true?))

I would prefer if a glb file is completely self-contained and cannot reference other URIs.

While the point is valid and reasonable, I'm not sure whether this should be enforced by the spec. One could just say it's in the responsibility of the asset creator. Right now, external references are explicitly allowed for GLB (as mentioned in the README). In any case: The same arguments could be applied to embedded glTF.

@lexaknyazev
Copy link
Member Author

+1 for making buffer.uri optional. Handling nullable fields is tricky in JS.

I would prefer if a glb file is completely self-contained and cannot reference other URIs.

While the point is valid and reasonable, I'm not sure whether this should be enforced by the spec. One could just say it's in the responsibility of the asset creator.

It's a question about glTF conformance. If we explicitly allow external references (buffers and images) in GLB format, all conformant clients must support them too. Otherwise, different conformance profiles/levels should be defined.

@pjcozzi
Copy link
Member

pjcozzi commented Feb 4, 2017

Some requests

  • Please bump the version field in the header to be 2 so if an app sees a binary blob, it can determine if it is glTF 1.0 + KHR_binary_glTF or glTF 2.0. This will be important to Cesium.
  • For "deprecating" KHR_binary_glTF, can we just add a sentence to it that it is written against the glTF 1.0 spec, and glTF 2.0 has nearly identical functionality in core spec? It's not exactly being deprecated, just like glTF 1.0 is not be deprecated. We just expect everyone will use glTF 2.0.

@pjcozzi
Copy link
Member

pjcozzi commented Feb 4, 2017

@lilleyse can you also review and chime in here since you are familiar with all our binary glTF export and import code.

@pjcozzi
Copy link
Member

pjcozzi commented Feb 4, 2017

I would prefer if a glb file is completely self-contained and cannot reference other URIs.

While the point is valid and reasonable, I'm not sure whether this should be enforced by the spec. One could just say it's in the responsibility of the asset creator.

It's a question about glTF conformance. If we explicitly allow external references (buffers and images) in GLB format, all conformant clients must support them too. Otherwise, different conformance profiles/levels should be defined.

Although I see the appeal, if a glb is self-contained, it misses a really important use case: storing textures separately, which you do not want gzipped as part of the .glb. For specifics, see: https:/KhronosGroup/glTF/tree/master/extensions/Khronos/KHR_binary_glTF#performance-results

It also misses out on a number of other use cases like not loading buffers until the bounding volume is visible.

@lilleyse
Copy link
Contributor

lilleyse commented Feb 7, 2017

A buffer without a uri seems ok to me as long as the spec is clear that only one of these can exist, and that it only applies when the model is a glb.

Really any of the solutions proposed don't present a problem for our import/export.

@sbtron
Copy link
Contributor

sbtron commented Feb 9, 2017

I think we are in agreement here:

  • make buffer.uri optional
  • continue to allow external references in the glb. Not all implementations will be able to support it for e.g. viewing files on disk will only work with embedded or on disk references. Web scenarios can continue to link to other uris.

Can we close this?

@lexaknyazev
Copy link
Member Author

@sbtron

viewing files on disk will only work with embedded or on disk references

Does it mean that desktop GLB viewer should support relative (to the asset's path) URIs or file:// scheme?

@sbtron
Copy link
Contributor

sbtron commented Feb 9, 2017

Does it mean that desktop GLB viewer should support relative (to the asset's path) URIs or file:// scheme?

yes. This would be similar to say an obj pointing to other textures files on disk.
Edit: I should add I have no good reasons on why someone would want to do this. But if the spec allows it then a desktop viewer could support it.

@javagl
Copy link
Contributor

javagl commented Feb 9, 2017

There seems to be a misunderstanding here. But maybe it's on my side.

The URIs in glTF will almost always be relative. An absolute URI makes virtually no sense, particularly, a file:/ URI. But for the usual case of relative URIs, it does not matter whether they appear in a glTF or in a GLB, or whether they appear in a web application or on a desktop viewer.

What point am I missing here?

@lexaknyazev
Copy link
Member Author

@javagl
There could be good reasons to have absolute URIs in dynamically-created glTF assets for online environment. E.g., server could bake in full paths to images, stored in CDN.

@sbtron
I'm not sure about full support of file:// URI scheme in offline clients.

So, for the client conformance, I propose to require only relative URI support. Everything else would be app-specific anyway.

There could be privacy/security concerns with URIs from user-provided or intentionally malicious assets.

@bghgary
Copy link
Contributor

bghgary commented Feb 15, 2017

Can we enforce that the binary (non-external) buffer be the first buffer (i.e. index 0)? That would make implementation a bit easier.

@lexaknyazev
Copy link
Member Author

There could be a case with GLB container without internal buffer. So requirement would be like:

If GLB file has embedded binary buffer, it is referred via the first element (i.e., with index 0) of buffers array with uri property undefined.

@pjcozzi

@pjcozzi
Copy link
Member

pjcozzi commented Feb 15, 2017

Can we enforce that the binary (non-external) buffer be the first buffer (i.e. index 0)? That would make implementation a bit easier.

Probably OK, but can you explain how it makes implementation easier?

@bghgary
Copy link
Contributor

bghgary commented Feb 15, 2017

@pjcozzi

I started typing this:
After loading the header and content, I need to decide whether to continue reading the rest of the file or fail due to unexpected extra bytes at the end. Thus, I need to know if there is a binary buffer or not. I used to be able to get this by name, but now I have to search the buffers and look for one that doesn't have a URI.

But now that I think about it more:
We don't need a byteLength on the buffer. It's already computable from the header or the file size. We don't even need an entry in the buffers for the binary buffer. Maybe omit the buffer property for bufferViews when referencing the binary buffer. That avoids this problem completely.

{
  "bufferViews": [
    {
      "byteOffset": 123
      "byteLength": 123
    }
  ]
}

Thoughts?

@pjcozzi
Copy link
Member

pjcozzi commented Feb 16, 2017

If I'm following your concern, it is that the header fields define the embedded binary buffer, and the buffer object in the glTF JSON also defines it. Is the issue that the later is redundant? I am cautious about allowing any incorrect glTF (e.g., this bufferView doesn't require a buffer) or requiring any external file size knowledge as binary glTF can be embedded in a byte stream.

Is this problem to unique to desktops? Or mainly because we went from object properties to arrays for buffers?

I am hesitant here because this is widely used in Cesium and we went through several iterations to get binary glTF "right."

@lexaknyazev
Copy link
Member Author

lexaknyazev commented Feb 16, 2017

I would be against removal of buffer object representing GLB data because we need buffer.byteLength for following reasons:

  • Without it, we can't validate JSON against binary data (if, e.g., binary part of GLB was substituted or truncated).
  • One buffer could be used by many bufferViews and we must be sure that all of them fit.

I need to decide whether to continue reading the rest of the file or fail due to unexpected extra bytes at the end.

How about putting bufferLength before binary data?

uint32 magic
uint32 version
uint32 length
uint32 contentLength
uint32 contentFormat
ubyte[] content
uint32 bufferLength // equals buffer.byteLength
uint32 bufferFormat // may be reserved for future use, like LZMA
ubyte[] buffer

@javagl
Copy link
Contributor

javagl commented Feb 16, 2017

Once more, I'm too blind to see what seems to be obvious for others - namely, why buffer.length == (length - 20 - contentLength) is not sufficient to determine the expected number of bytes for the buffer.

@bghgary
Copy link
Contributor

bghgary commented Feb 16, 2017

@pjcozzi

Is the issue that the later is redundant?

Yes. Boiling it down, that is the underlying issue.

Is this problem to unique to desktops?

No. I'm currently working on BabylonJS's glTF loader.

get binary glTF "right."

Yes, totally understand. I don't know the history of this, but I suspect the reason why buffer is necessary for the binary extension before is because it is an extension. The binary extension must follow the pattern of the core spec which requires a buffer to be there.

@lexaknyazev

we can't validate JSON against binary data

No argument there, but there are currently multiple ways to get the size of the binary buffer.

  1. buffer.byteLength where buffer.uri is undefined
  2. header.length - 20 - header.contentLength
  3. fileSize - 20 - header.contentLength

I agree it's not a good idea to rely on fileSize, so 3 is out, though loaders should verify that it matches header.length if possible.

I don't see any reason why 2 can't work.

Do we still need 1?

@lexaknyazev
Copy link
Member Author

My perspective on JSON validity is that it mustn't depend on external means.
So, validating each bufferView's offset and length against buffer object should be possible within JSON.

Additionally, we must require that only the first buffer from "buffers" array can have "uri" undefined, and no other buffer can omit "uri".

I'm not quite sure about assuming all after-header data to be just one buffer.
Of course, it is one buffer right now, but such approach (lack of explicit buffer length in GLB) prevents us from extending GLB format in the future without breaking header structure.

@bghgary
Copy link
Contributor

bghgary commented Feb 16, 2017

My perspective on JSON validity is that it mustn't depend on external means.

Okay. I can see how this might be easier to handle if there is some shared code that validates the JSON by itself.

all after-header data to be just one buffer.

Isn't that what bufferViews are for, to split up the buffer into chunks if necessary?

@lexaknyazev
Copy link
Member Author

Isn't that what bufferViews are for, to split up the buffer into chunks if necessary?

They are for internal glTF needs. We were thinking about additional external metadata, like asset's digital signature (which obviously couldn't be referred by bufferView). It's not on the radar now, but I'd avoid locking GLB format from possible extensions.

@bghgary
Copy link
Contributor

bghgary commented Feb 16, 2017

Okay, sounds reasonable.

@pjcozzi
Copy link
Member

pjcozzi commented Feb 23, 2017

@lexaknyazev this sounds good to me: #828 (comment)

Is there anything else left to decide on for this issue?

@lexaknyazev
Copy link
Member Author

this sounds good to me: #828 (comment)
Is there anything else left to decide on for this issue?

@pjcozzi
That comment referred to updated GLB header. Are you agree with that also?

@pjcozzi
Copy link
Member

pjcozzi commented Feb 24, 2017

@pjcozzi
That comment referred to updated GLB header. Are you agree with that also?

For reference, it is proposed as

uint32 magic
uint32 version
uint32 length
uint32 contentLength
uint32 contentFormat
ubyte[] content
uint32 bufferLength // equals buffer.byteLength
uint32 bufferFormat // may be reserved for future use, like LZMA
ubyte[] buffer

The value of moving bufferLength is not obvious to me. It seems that a more cohesive design is to keep the the header sub-contained. right?

@lexaknyazev
Copy link
Member Author

lexaknyazev commented Feb 24, 2017

bufferLength was added, not moved.
Prepending each chunk of data with its length is common practice (e.g., PNG, MP4). It allows easy introduction of new chunks without breaking compatibility.

For more uniform "chunk" definition, we could rename:

  • contentLength and bufferLength to chunkLength;
  • contentFormat and bufferFormat to chunkType with global scope of "chunk types".

This would make GLB layout almost identical to aforementioned containers.

@pjcozzi
Copy link
Member

pjcozzi commented Feb 24, 2017

Ah, I see, OK with me. As for the "chunk" rename I'll defer to whatever you think common practice is.

@pjcozzi
Copy link
Member

pjcozzi commented Jun 15, 2017

Updated in #826

@Adnn
Copy link

Adnn commented Mar 4, 2022

@lexaknyazev I was looking at Buffer's reference, and could not find explanation regarding why uri field might be optional. Maybe a note should be added at this point in the reference?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants