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

glTF: Fix parsing image data with mimeType undefined #42504

Merged
merged 1 commit into from
Oct 5, 2020

Conversation

akien-mga
Copy link
Member

@akien-mga akien-mga commented Oct 2, 2020

The glTF 2.0 spec only makes mimeType mandatory for bufferView image data,
so the previous logic to handle URIs with base64-encoded images could fail if
mimeType is undefined.

The logic was documented and refactored to better handle the spec, notably:

  • uri and bufferView are now mutually exclusive, and only the latter fails
    if mimeType is undefined.
  • uri with a file path will now respect the mimeType if defined, and thus
    attempt loading the file with the specified format (even if its extension is
    not the one expected for this format). So we can support bad extensions (PNG
    data with .jpg extension) or custom ones (PNG data in .img file for
    example).
  • uri with base64 encoded data will infer MIME type from data:image/png or
    data:image/jpeg if it was not documented in mimeType initially.
  • uri with base64 encoded data, no mimeType and application/octet-stream
    or application/gltf-buffer will fall back to trying both PNG and JPEG
    loaders.

Fully fixes #33796 (and fixes up #42501).
Fixes #37514.

@fire
Copy link
Member

fire commented Oct 2, 2020

fixes: #37514

The glTF 2.0 spec only makes `mimeType` mandatory for `bufferView` image data,
so the previous logic to handle URIs with base64-encoded images could fail if
`mimeType` is undefined.

The logic was documented and refactored to better handle the spec, notably:

- `uri` and `bufferView` are now mutually exclusive, and only the latter fails
  if `mimeType` is undefined.
- `uri` with a file path will now respect the `mimeType` if defined, and thus
  attempt loading the file with the specified format (even if its extension is
  not the one expected for this format). So we can support bad extensions (PNG
  data with `.jpg` extension) or custom ones (PNG data in `.img` file for
  example).
- `uri` with base64 encoded data will infer MIME type from `data:image/png` or
  `data:image/jpeg` if it was not documented in `mimeType` initially.
- `uri` with base64 encoded data, no `mimeType` and `application/octet-stream`
  or `application/gltf-buffer` will fall back to trying both PNG and JPEG
  loaders.

Fully fixes godotengine#33796 (and fixes up godotengine#42501).
@akien-mga akien-mga force-pushed the glTF-fix-image-loading branch from 1359300 to 2e99d0b Compare October 5, 2020 12:03
@akien-mga
Copy link
Member Author

I did some additional testing and it seems to work well. I can import the whole https://github.com/KhronosGroup/glTF-Sample-Models repo with one exception:

The BrainStem model triggers this crash #42569, so it should be removed or ignored before importing the repo. Same thing with its DAE version in sourceModels.

@akien-mga
Copy link
Member Author

Tested a local cherry-pick on 3.2 and it also seems to import the whole repo of Khronos samples fine (including BrainStem).

@akien-mga akien-mga merged commit 7d31244 into godotengine:master Oct 5, 2020
@akien-mga akien-mga deleted the glTF-fix-image-loading branch October 5, 2020 13:02
@akien-mga
Copy link
Member Author

Cherry-picked for 3.2.4.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Oct 5, 2020
akien-mga added a commit to akien-mga/godot that referenced this pull request Jan 5, 2021
We should first attempt loading as external files, thus creating a dependency.
Loading as a buffer should only be used as fallback to support manually loading
as PNG or JPEG depending on the defined mimeType.

Fixes godotengine#44309, was a regression from godotengine#42504.
akien-mga added a commit to akien-mga/godot that referenced this pull request Jan 5, 2021
We should first attempt loading as external files, thus creating a dependency.
Loading as a buffer should only be used as fallback to support manually loading
as PNG or JPEG depending on the defined mimeType.

Fixes godotengine#44309, was a regression from godotengine#42504.

(cherry picked from commit e268a8e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GLTF2 model can't load glTF importer tries to use base64 text as file path
2 participants