EXT_meshopt_compression dummy buffer byteLength can be any size
andreasplesch opened this issue · 18 comments
EXT_meshopt_compression decompresses bufferViews of a compressed buffer. It also requires a reference to a corresponding uncompressed fallback buffer which can be an empty dummy but has to have a byteLength.
Currently, the extension spec. does not seem to place any requirements on the byteLength of a dummy buffer. Most generators of compressed gltf such as gltfpack probably assign the total uncompressed byteLength of the buffer after decompression of all bufferViews which reference this dummy buffer. This is very helpful to implementations which may want to decompress a compressed bufferView into an arrayBuffer of the provided byteLength.
But such implementations cannot currently rely on the provided byteLength to be compatible with the uncompressed length of all compressed bufferViews since the byteLength is allowed to be any size.
A spec. sentence such as
The byteLength property of a fallback buffer MUST be sufficiently large to contain all uncompressed bufferViews referencing it.
in the fallback buffer section may address this concern. Since gltfpack likely already behaves in this way and gltfpack is the only current generator (not anymore, see below), full backward compatibility seems to be guaranteed.
Let me add here as well that the properties of the parent bufferView of the extension object would also be expected to be consistent with the uncompressed data but does not seem to be actually required to be so, eg. cannot be relied on to view a dummy buffer populated with uncompressed data.
I'm fine with this clarification, but we shouldn't assume no other generators exist. glTF Transform does the same:
Thanks, indeed, let's not assume that, and it is great that glTF Transform does what for now is perhaps implied but not required. So adding the clarification would be more important in case other generators come along.
Do you have a suggestion for the extension spec. language to add ? An option would be to require an exact fit of the buffer.bufferLength to the combined length of all uncompressed bufferViews (taking also into account the uncompressed byteOffsets). But the sentence above is the most concise language I could think of.
The byteLength property of a fallback buffer MUST be sufficiently large to contain all uncompressed bufferViews referencing it.
Isn't this implied by the existing validity requirements (edit: meaning, I'd assume base glTF should have this requirement unless an extension explicitly relaxes it)? This was definitely the assumption when the extension text was written and it feels safe to add this as an explicit clarification but I don't think there's a risk of invalidating any existing documents.
Thanks, that makes sense.
I checked the core glTF spec. and could not find such validity requirements.
I think it is the concept of dummy buffers that makes it conceivable that an unused dummy could exist with a byteLength of say 0 since an implementation could directly decompress to other storage. That is why it would be helpful to make this an explicit requirement.
- byteLength 0 seems to violate glTF schema restrictions as the value should be 1+
- byteLength 1 results in the following validation errors on current validator:
{
"code": "BUFFER_VIEW_TOO_LONG",
"message": "BufferView does not fit buffer (1) byteLength (1).",
"severity": 0,
"pointer": "/bufferViews/0/byteLength"
},
{
"code": "BUFFER_VIEW_TOO_LONG",
"message": "BufferView does not fit buffer (1) byteLength (1).",
"severity": 0,
"pointer": "/bufferViews/1/byteOffset"
},
I'm on board with adding this requirement; due to the above it should be safe.
It was probably also the intention that the properties of the uncompressed bufferView (such as byteLength or byteOffset) in case they reference a dummy buffer are compatible with the compressed bufferView and are valid suggestions for storage of the decompressed bufferView into a buffer constructed from the dummy buffer. I am not sure if this also should be made explicit.
I don't fully understand the second suggestion :)
The assumptions are:
- The uncompressed (parent) buffer view is a proper subset of the referenced buffer (which is possibly dummy/fallback)
- The compressed (child) buffer view is a proper subset of the referenced buffer (which is a different buffer from pt1 and contains compressed data and therefore can't be dummy/fallback)
- The result of decompressing the compressed data results in the specified number of bytes in the parent buffer view.
It is good that the validator catches this. Severity 0 may mean that this not critical ? It probably relies on the fact that the bufferView.bufferLength is is actually the correct decompressed bufferLength.
Got it. These are the important assumptions to perhaps make explicit.
Severity 0 is the highest (I believe 0 = error, 1 = warning, 2 = informational, or thereabouts)
Well, I did not find the corresponding language in the core spec., perhaps because this is normally implied. But I do think the case of dummy buffers requires more clarification.
- The uncompressed (parent) buffer view is a proper subset of the referenced buffer (which is possibly dummy/fallback)
I think there is a distinction between dummy and fallback buffers, at least semantically. A dummy buffer is empty and could be used to populate with uncompressed data. A fallback buffer is not empty and already has data with correspond to the compressed data. It is a fallback in case the the extension is not supported. So a dummy buffer is not necessarily instanced, it first exists only as a description.
- The result of decompressing the compressed data results in the specified number of bytes in the parent buffer view.
This is what I was looking for, in addition to the buffer.byteLength.
But let's focus on the buffer.byteLength. I can submit a quick PR with the above language if that would be useful.
Ah ok I understand the second suggestion and agree it's important to make as well; it isn't explicitly spelled out either but it was definitely the intent. In essence both of these say that while an application could decompress each buffer view into a freshly allocated buffer, it could also use the provided layout in the outer buffer and decompress everything into one buffer; this means that the dummy buffer must be suballocated correctly even if the contents is absent (even if it's dummy).
I think there is a distinction between dummy and fallback buffers
Yes, / meant "or" in the fragment you quoted. In either case the range requirement should hold, although I suppose it's more obvious that it does if it's a fallback buffer as it mirrors existing [implicit?] glTF restrictions, and a little less obvious for dummy buffers.
Ok, completely agreed. To the second point, it would be good to find concise language also. I liked
provided layout in the outer buffer
So something like
The parent bufferVIew properties must define a layout which can hold the data decompressed (decoded?) from the extension object.
may work well enough ?
The PR above has the discussed changes plus a minor formatting change: Buffer View length to bufferView.byteLength.
Thanks for the quick resolution.