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

Add support for glTF EXT_texture_webp #7486

Merged
merged 14 commits into from
Jan 31, 2019
Merged

Add support for glTF EXT_texture_webp #7486

merged 14 commits into from
Jan 31, 2019

Conversation

OmarShehata
Copy link
Contributor

@OmarShehata OmarShehata commented Jan 15, 2019

This PR provides a reference implementation for the glTF extension EXT_image_webp (KhronosGroup/glTF#1534). It also provides a sample model (CesiumBoxWebp.gltf) that uses this extension.

The extension is defined on the texture node:

"textures": [
    {
      "sampler": 0,
      "source": 0,
      "extensions": {
        "EXT_texture_webp": {
          "source": 1,
          "sampler": 1
        }
      }
    }
  ],
  "images": [
    {
      "name": "cesium logo",
      "bufferView": 4,
      "mimeType": "image/png"
    },
    {
      "name": "cesium logo",
      "bufferView": 3,
      "mimeType": "image/webp"
    }
  ],

FeatureDetection will check if WebP is supported in this browser, and if so, use the provided WebP, otherwise it will fallback to the PNG. If the glTF defines EXT_image_webp in extensionsRequired and this browser does not support WebP, it will currently throw a runtime error.

To test in Sandcastle:

var viewer = new Cesium.Viewer('cesiumContainer');

var position = Cesium.Cartesian3.fromDegrees(-123.0744619, 44.0503706, 5000.0);
var entity = viewer.entities.add({
    position : position,
    model : {
        uri : '../../../Specs/Data/Models/Box-Textured-Webp/CesiumBoxWebp.gltf',
        minimumPixelSize : 128,
        maximumScale : 20000
    }
});
viewer.trackedEntity = entity;

localhost url. Current browser support:

  • Chrome will correctly load the WebP.
  • Firefox will fallback to the PNG.
  • Edge will crash, because it reports that WebP is supported, but fails when you try to use it with WebGL (I believe due to this bug).
  • IE11 will fallback to PNG (but it crashes right now due to IBL IBL Example crashes on IE11 #7485 ).

To Do

  • Add tests. Test that it works in glb as well.
  • Update changes.md
  • The way FeatureDetection.supportsWebp works is a little weird because it's the only async function in there. I haven't found a way to synchronously test this. This might be a good time to have something where we do a series of quick canvas checks on startup that'll be cached (where we could check WebGL extensions that are reported to be supported but aren't actually).
    • This is currently why Travis is failing, no tests are failing (I'm calling this feature check on module load, when it should be called somewhere on init, maybe Scene?)
  • Consider decoding WebP in the browser with libwebpjs if it's not supported? Will address in a future PR

@cesium-concierge
Copy link

Thanks for the pull request @OmarShehata!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 15, 2019

Thanks @OmarShehata. When you open the glTF extension PR:

  • Please consider if this should be named EXT_image_webp or EXT_webp. I would have thought the later, but it looks like either could work given the current extension names.
  • For the png/jpg/etc fallback, please be super clear that this is an optional fallback - we can't expect it to be present given the increased disk space usage and the increased payload for .glb files and loaders that download all referenced images.
  • I suspect you will also get feedback from the glTF community about if the EXT_image_webp object should just have the mimeType property or if the EXT_image_webp object needs to exist at all, e.g., if the presence of the extension just means that "image/webp" is an allowed mimeType.

Please don't discuss the above here; the glTF repo is the right forum when you open the PR.


Also, as we discussed offline, please have a warning of some sort when WebP isn't supported - and, I assume, when it is "supported" but crashes - and submit an issue to remove the warning when the browser vendors issue a fix.

@likangning93
Copy link
Contributor

Consider decoding WebP in the browser with libwebpjs if it's not supported?

@OmarShehata let me know if you want to investigate a WASM decoder instead, I have code that might help you get started.

@mramato
Copy link
Contributor

mramato commented Jan 30, 2019

Firefox 65 is now out with official WebP support, so Safari is probably the only major browser we ultimately need to worry about (since all iOS browsers are Safari under the hood.)

@mramato
Copy link
Contributor

mramato commented Jan 30, 2019

Fixes #6935

@mramato
Copy link
Contributor

mramato commented Jan 31, 2019

@OmarShehata what's the plan for this PR? I assumed we wanted it in the next release (sans fallback) but is that not the case?

@OmarShehata
Copy link
Contributor Author

OmarShehata commented Jan 31, 2019

You are correct, we don't want this merged here in this release. I was wrong, we are getting this in for release!

@OmarShehata OmarShehata changed the title Add support for glTF EXT_image_webp Add support for glTF EXT_texture_webp Jan 31, 2019
@mramato
Copy link
Contributor

mramato commented Jan 31, 2019

That's all I have for this pass.

@OmarShehata
Copy link
Contributor Author

Ok @lilleyse I think all of Matt's comments have been resolved now, this should be ready. The only thing I have not been able to test yet is whether it correctly detects WebP support on Safari/on a Mac.

@lilleyse
Copy link
Contributor

In Safari it correctly throws the error "Loaded model requires WebP but browser does not support it." if the extension is in extensionsRequired and otherwise uses the png fallback.

Specs/Data/Models/Box-Textured-Webp/CesiumBoxWebp.gltf Outdated Show resolved Hide resolved
Source/Scene/ClassificationModel.js Show resolved Hide resolved
Source/Scene/ClassificationModel.js Outdated Show resolved Hide resolved
Source/Scene/Model.js Outdated Show resolved Hide resolved
Specs/Core/FeatureDetectionSpec.js Show resolved Hide resolved
@OmarShehata
Copy link
Contributor Author

Thanks @lilleyse ! Should be all good now.

@lilleyse lilleyse merged commit 7fbc795 into CesiumGS:master Jan 31, 2019
@OmarShehata OmarShehata deleted the gltf-webp branch January 31, 2019 19:06
@yoyomule
Copy link

Can we also consider the avif format, which is twice as small as webp.

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

Successfully merging this pull request may close these issues.

7 participants