Skip to content

Conversation

@lilleyse
Copy link
Contributor

@lilleyse lilleyse commented Feb 17, 2022

We were seeing some artifacts in 3D Tiles Next demos that use feature id textures on Safari desktop and mobile. Interestingly the S2 demo, which also uses feature id textures, didn't have these artifacts. After more investigation the only difference was that the first two demos had images with embedded color profiles, and the S2 demo did not.

Screen Shot 2022-02-16 at 2 04 42 PM Screen Shot 2022-02-16 at 1 20 17 PM

The glTF spec requires that embedded color profiles in images are ignored since it's the material definition that determines whether images are considered sRGB or linear. The same rule extends to feature ID textures. There are two ways of ensuring that:

  • if ImageBitmap is not supported then gl.pixelStorei(gl.UNPACK_COLORSPACE_CONVERSION_WEBGL, gl.NONE) should be called before the HTMLImageElement is passed to gl.texImage2D
  • otherwise if ImageBitmap is supported colorSpaceConversion: "none" should be passed to createImageBitmap, since UNPACK_COLORSPACE_CONVERSION_WEBGL has no effect for ImageBitmap

Most of this was already done in #9624. The problem is that Firefox and Safari support ImageBitmap but they don't support the colorSpaceConversion option so they would always apply some sort of color correction, which really breaks feature id textures. We don't want gamma correction applied to non-color data 😄

So this PR disables ImageBitmap if the colorSpaceConversion option isn't supported. Unfortunately there's no way for the browser to tell us this, made worse by the fact that the createImageBitmap promise resolves even when a non-default colorSpaceConversion option is passed in, so the approach here is for Resource.supportsImageBitmapOptions to create an ImageBitmap from a PNG with an embedded color profile twice, once with colorSpaceConversion: "none" and again with colorSpaceConversion: "default", and check that the results are different.

Downside is that we can no longer use ImageBitmap in Firefox and Safari. But at least feature ID textures are working properly now.

Fixes #9875

@cesium-concierge
Copy link

Thanks for the pull request @lilleyse!

  • ✔️ 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.

@j9liu
Copy link
Contributor

j9liu commented Feb 23, 2022

@lilleyse - this looks good to me! I tested the feature-picking sandcastles in Firefox and they appear as they do in Chrome.

I forgot to skip CI for the CHANGES.md change but once that's done, I'll merge.

@lilleyse
Copy link
Contributor Author

At the last minute I realized that getImagePixels existed, so I'm using that now.

@j9liu
Copy link
Contributor

j9liu commented Feb 23, 2022

Went through the same tests and it works the same, so I'll merge now.

@j9liu j9liu merged commit 4a81b82 into main Feb 23, 2022
@lilleyse lilleyse deleted the require-color-space-conversion-for-image-bitmap-support branch February 23, 2022 20:03
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.

Revert early returns on image bitmap specs in Firefox and Safari

4 participants