mrdoob / three.js

JavaScript 3D Library.
https://threejs.org/
MIT License
102.3k stars 35.35k forks source link

GLTFExporter: jpeg textures on export binary and embedImages=false cause validation error #23962

Closed 852Kerfunkle closed 2 years ago

852Kerfunkle commented 2 years ago

Heya,

when exporting a material with a jpeg texture as a .glb with these options:

{
    binary: true,
    embedImages: false
}

The resulting .glb has a validation error:

      {
        "code": "IMAGE_MIME_TYPE_INVALID",
        "message": "Recognized image format 'image/jpeg' does not match declared image format 'image/png'.",
        "severity": 0,
        "pointer": "/images/0"
      },

It seems three.js exports the texture with the image/png mime type, regardless of what type the image actually is: ...,"images":[{"mimeType":"image/png","uri":"data:image/jpeg;base64,....

I had a brief look around but couldn't find a way to set the mime type manually (but maybe I missed something...).

Working example: https://framer.tz1and.com/ Source: https://github.com/tz1and/image-framer/blob/main/src/index.js

Thanks!

Mugen87 commented 2 years ago

The problematic line in GLTFExporter is:

https://github.com/mrdoob/three.js/blob/47098e62a5a5706996fc93b446db64720e18cf6f/examples/jsm/exporters/GLTFExporter.js#L1140

It is only used when embedImages is set to false. Normally, imageDef.uri should hold a URL to the image. In your case, a data URI is used instead which is of course not right. The image's src property points to a data URI since the texture is imported via FileReader.

embedImages === false does also not work if GLTFLoader loads the images via ImageBitmapLoader. In this case, imageDef.uri is undefined since image bitmaps do not have a src property.

@elalish @donmccurdy Because of the broken character of embedImages === false, would it be okay to remove this option from the exporter? I don't see an easy way to fix this for all use cases.

donmccurdy commented 2 years ago

So in other words — there's no reliable way to trace a THREE.Texture back to an external URL, but external URLs would be required for embedImages === false to work? Scenes loaded from GLBs, or containing KTX2 textures (not to mention files loaded from other formats) would probably be additional edge cases here. Related: #21328.

I think I agree about removing this option, if that's OK with others. /cc @takahirox

Mugen87 commented 2 years ago

there's no reliable way to trace a THREE.Texture back to an external URL, but external URLs would be required for embedImages === false to work?

Correct!

Scenes loaded from GLBs, or containing KTX2 textures (not to mention files loaded from other formats) would probably be additional edge cases

Yes. So the exporter would need to handle image bitmaps, compressed textures and data URIs correctly.

elalish commented 2 years ago

I'm in favor of removing that option. Single-file GLBs are way more popular anyway, and if someone really wants to separate them they can always use gltf-pipeline or similar.

takahirox commented 2 years ago

Removing the option sounds ok to me because I prefer to make the exporter as simple as possible and, as @elalish mentioned, users may use post processing tools like gltf-transform. And also they might be able to write the separated texture file plugin with the exporter plugin API? I didn't check in details if the APIs greatly fit to that purpose yet tho.

donmccurdy commented 2 years ago

Ensuring that all textures have unique names, and then post-processing to known URLs based on those names, should be straightforward even without additional libraries. Given the gltf result of GLTFExporter with binary=false,

// set image URLs ...

for ( const textureDef of gltf.textures ) {

  const imageDef = gltf.images[ textureDef.source ];

  imageDef.uri = `https://example.com/path/to/${textureDef.name}.png`;
  imageDef.mimeType = 'image/png';

}

// ... then save to .gltf
Mugen87 commented 2 years ago

and then post-processing to known URLs

How would you come up with such URLs? Is that something the user has to configure via exporter options?

Even if the exporter does this, I assume most users would expect the exporter also stores the textures next to the glTF asset.

donmccurdy commented 2 years ago

I've assumed that was always the point of embedImages=false — the user needs to manage the image hosting themselves and keep track of those URLs. Certainly harder to use than embedded images, but could be useful if you are exporting a bunch of glTF files that share the same textures.

But in any case, it's not clear what we'd really be supporting by keeping this option, so +1 for removing it.

852Kerfunkle commented 2 years ago

The problematic line in GLTFExporter is:

https://github.com/mrdoob/three.js/blob/47098e62a5a5706996fc93b446db64720e18cf6f/examples/jsm/exporters/GLTFExporter.js#L1140

It is only used when embedImages is set to false. Normally, imageDef.uri should hold a URL to the image. In your case, a data URI is used instead which is of course not right. The image's src property points to a data URI since the texture is imported via FileReader.

Makes perfect sense to me, I certainly misused this feature little (semi-intentionally).

What I was really trying to achieve is to have the source textures embedded. With embedImages set to true, jpegs are saved uncompressed with 4 channels - which is larger than a 3-channel uncompressed png. I don't recall checking if they are actually saved as png.

Relating to this... Is there a proper way to embed the source textures?

donmccurdy commented 2 years ago

Possibly https://github.com/mrdoob/three.js/pull/23998 would help you, if the JPEG size increase is new with the r140 release?

But no, GLTFExporter does not have access to original texture data. It tries to export reasonably compact textures at the expense of being a bit lossy, but it doesn't try multiple formats and wouldn't know when PNG could be smaller than JPEG.

If your use case happens to be...

  1. load one or more glTF files
  2. view and/or edit in three.js
  3. export back to glTF

... then @gltf-transform/view would be a lossless (but still experimental) option that preserves the original texture data.

852Kerfunkle commented 2 years ago

Thank you, @donmccurdy.

I'm actually on 139.2, but possibly that regression applies to it as well. I've observed a nearly 2.5x increase in size on a very simple model with a jpeg texture assigned. Sure sounds about right.

I'll make sure to take a very close look at the behaviour with embedImages === true when I circle back to this (and leave a comment where it applies - or create a new issue).

Thanks!

Mugen87 commented 2 years ago

23592 could help. You can store the original mime type in Texture.userData which will be honored by GLTFExporter.

I'm actually on 139.2, but possibly that regression applies to it as well. I've observed a nearly 2.5x increase in size on a very simple model with a jpeg texture assigned.

When you are using 139.2, you are not affected by the latest regression.

However, since three.js does not support RGB formats anymore, GLTFExporter was updated to always export PNG (see #23385). This approach produces larger files size however #23592 was introduced to bypass this issue.

852Kerfunkle commented 2 years ago

@Mugen87 Thank you for resolving this bug.

And just to leave a related comment: #23592 works great. I suppose it would be nice to have some way of preserving the source image. Quality loss on jpeg recompression and all that. Maybe something akin to #23592 can be done - store the original texture in userData. Just an idea, I'm certainly happy with the status quo.