Add option to ignore colorspace information in texture images #9624

j9liu · 2021-06-17T21:27:18Z

The UNPACK_COLORSPACE_CONVERSION_WEBGL flag was not set to NONE as specified in the glTF documentation.

When the Sandcastle from the issue is copy pasted into a local session, it should now look like this. The spheres across each row are identical.

cc @ebogo1 @shehzan10

cesium-concierge · 2021-06-17T21:27:22Z

Thanks for the pull request @j9liu!

✔️ Signed CLA found.
❔ Unit tests were not updated.
- Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

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

lilleyse · 2021-06-17T23:20:04Z

The change seems good for glTF but it could impact rendering of non-glTF images such as imagery on terrain, textured polygons, or billboards. You might notice something if you compare sandcastle examples on this branch vs. master (or maybe not).

The safe thing to do would be to add an option to Texture called skipColorSpaceConversion that only Model.js sets to true.

CC @ptrgags @sanjeetsuhag for the Model.js rewrite

j9liu · 2021-06-18T14:48:52Z

@lilleyse - just pushed some changes, is this better?

Source/Scene/Model.js

Source/Renderer/Texture.js

…s and functions

j9liu · 2021-06-21T21:40:54Z

@lilleyse - what do you think of these changes? I haven't found a workaround for the HTMLCanvasElement situation yet, but I'll keep looking.

I changed the structure of the options in the createImageBitmap and loadImageFromTypedArray functions, so if we keep the changes, I will need to adjust one of the unit tests because the change is causing it to fail.

Source/Core/loadImageFromTypedArray.js

Source/Renderer/Texture.js

Source/Scene/Model.js

Source/Scene/GltfImageLoader.js

Source/Core/Resource.js

Source/Renderer/Texture.js

…s object

CHANGES.md

Source/Renderer/Texture.js

Specs/Renderer/TextureSpec.js

lilleyse · 2021-06-23T15:47:34Z

Source/Core/Resource.js

      crossOrigin,
      deferred,
      flipY,
+      skipColorSpaceConversion,


I did a global search for createImage and noticed that some terrain and imagery tests require updates. See ArcGisMapServerImageryProviderSpec.js and others examples like below - the new parameter needs to be added.

Resource._DefaultImplementations.createImage( request, crossOrigin, deferred, true, true );

lilleyse · 2021-06-23T15:54:38Z

Specs/Core/loadImageFromTypedArraySpec.js

+          expect(window.createImageBitmap).toHaveBeenCalledWith(blob, {
+            imageOrientation: "none",
+            premultiplyAlpha: "none",
+            colorSpaceConversion: "default",


Excellent tests throughout!

Source/Renderer/Texture.js

lilleyse · 2021-06-23T16:02:54Z

Source/Renderer/Texture.js

 * or an object with width, height, and arrayBufferView properties.
- *
- * @param {Object} source The source {@link ImageData}, {@link HTMLImageElement}, {@link HTMLCanvasElement}, or {@link HTMLVideoElement},
+ * @param {Object} options Object with the following properties:


For the sake of API symmetry we should also make the same changes to CubeMapFace.prototype.copyFrom.

lilleyse · 2021-06-23T16:03:27Z

@j9liu thanks for the updates, they look really good overall. Just a few small things left and this PR should be ready to go.

…s.createImage

j9liu · 2021-06-23T20:12:18Z

@lilleyse - I think I addressed all your comments; let me know if I missed anything!

Also, I noticed that these two tests fail when I run them -- should I look into them? They were failing before I made the major changes to Texture and Resource, so I'm not sure if it is related to what I did.

lilleyse · 2021-06-23T22:15:03Z

Source/Renderer/loadCubeMap.js

 *
 * @param {Context} context The context to use to create the cube map.
 * @param {Object} urls The source URL of each image.  See the example below.
+ * @param {Boolean} skipColorSpaceConversion If true, any custom gamma or color profiles in the images will be ignored.


Suggested change

* @param {Boolean} skipColorSpaceConversion If true, any custom gamma or color profiles in the images will be ignored.

* @param {Boolean} [skipColorSpaceConversion=false] If true, any custom gamma or color profiles in the images will be ignored.

lilleyse · 2021-06-23T22:21:27Z

Specs/Scene/ArcGisMapServerImageryProviderSpec.js

          Resource._DefaultImplementations.createImage(
            request,
            crossOrigin,
-            deferred
+            deferred,
+            true,
+            true


Ah I should have been clearer about this. The only functions that need to be updated are those that were already passing in the optional parameters like here. Since createImage now takes 6 parameters code like below needs to be changed

Resource._DefaultImplementations.createImage( request, crossOrigin, deferred, true, true );

changes to

Resource._DefaultImplementations.createImage( request, crossOrigin, deferred, true, false, true, );

lilleyse · 2021-06-23T22:24:14Z

Source/Scene/SkyBox.js

    if (typeof sources.positiveX === "string") {
      // Given urls for cube-map images.  Load them.
-      loadCubeMap(context, this._sources).then(function (cubeMap) {
+      loadCubeMap(context, this._sources, false).then(function (cubeMap) {


Fine to use the default value here

Suggested change

loadCubeMap(context, this._sources, false).then(function (cubeMap) {

loadCubeMap(context, this._sources).then(function (cubeMap) {

lilleyse · 2021-06-23T22:51:04Z

Also, I noticed that these two tests fail when I run them -- should I look into them? They were failing before I made the major changes to Texture and Resource, so I'm not sure if it is related to what I did.

It looks like the tests pass in master so I think it's a regression in this PR. There's a good chance that once #9624 (comment) is addressed they will be fixed.

j9liu · 2021-06-24T01:17:02Z

@lilleyse - sorry for the misunderstanding! I reverted the changes. You were right, the fix for the missing parameter made those two tests pass.

lilleyse · 2021-06-24T17:10:23Z

I push a commit to revert one last test: 99f7a7b

Otherwise this looks good. I ran tests locally, tested TextureEncodingTest.gltf and TextureEncodingTest.glb with ImageBitmap enabled and disabled, and spot checked sandcastle examples.

Thanks @j9liu!

Ignore colorspace information in texture images

035cf5b

Add PR number to CHANGES.md

b9462aa

Add skipColorSpaceConversion as texture option that Model sets true

4a7df57

lilleyse mentioned this pull request Jun 19, 2021

Enable ImageBitmap again for external images in Model #9627

Merged

lilleyse reviewed Jun 19, 2021

View reviewed changes

Source/Scene/Model.js Show resolved Hide resolved

Source/Renderer/Texture.js Outdated Show resolved Hide resolved

Source/Renderer/Texture.js Show resolved Hide resolved

Add management of colorspace conversion in ImageBitmap-related classe…

e276700

…s and functions

lilleyse reviewed Jun 22, 2021

View reviewed changes

Add tests for fetchImage (Resource) and loadImageFromTypedArray

8141cc7

j9liu changed the title ~~Ignore colorspace information in texture images that interferes with glTF rendering~~ Add option to ignore colorspace information in texture images Jun 22, 2021

Janine Liu and others added 2 commits June 22, 2021 15:56

Consolidate parameter for Texture.prototype.copyFrom to single option…

1dd5d56

…s object

Merge branch 'master' into texture-encoding-gltf-bug

2bc8994

lilleyse reviewed Jun 23, 2021

View reviewed changes

Modify copyFrom in cubeMapFace and update specs that rely on Resource…

33cca61

…s.createImage

lilleyse reviewed Jun 23, 2021

View reviewed changes

Delete extraneous parameters from createImage calls

958bac5

Revert one imagery test

99f7a7b

lilleyse merged commit 4fb9fbb into master Jun 24, 2021

lilleyse deleted the texture-encoding-gltf-bug branch June 24, 2021 17:10

lilleyse mentioned this pull request Feb 17, 2022

Don't use ImageBitmap unless colorSpaceConversion option is supported #10111

Merged

	* @param {Boolean} skipColorSpaceConversion If true, any custom gamma or color profiles in the images will be ignored.
	* @param {Boolean} [skipColorSpaceConversion=false] If true, any custom gamma or color profiles in the images will be ignored.

	loadCubeMap(context, this._sources, false).then(function (cubeMap) {
	loadCubeMap(context, this._sources).then(function (cubeMap) {

Add option to ignore colorspace information in texture images #9624

Add option to ignore colorspace information in texture images #9624

Uh oh!

Conversation

j9liu commented Jun 17, 2021

Uh oh!

cesium-concierge commented Jun 17, 2021

Uh oh!

lilleyse commented Jun 17, 2021

Uh oh!

j9liu commented Jun 18, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

j9liu commented Jun 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lilleyse Jun 23, 2021

Choose a reason for hiding this comment

Uh oh!

lilleyse Jun 23, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lilleyse Jun 23, 2021

Choose a reason for hiding this comment

Uh oh!

lilleyse commented Jun 23, 2021

Uh oh!

j9liu commented Jun 23, 2021

Uh oh!

lilleyse Jun 23, 2021

Choose a reason for hiding this comment

Uh oh!

lilleyse Jun 23, 2021

Choose a reason for hiding this comment

Uh oh!

lilleyse Jun 23, 2021

Choose a reason for hiding this comment

Uh oh!

lilleyse commented Jun 23, 2021

Uh oh!

j9liu commented Jun 24, 2021

Uh oh!

lilleyse commented Jun 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

j9liu commented Jun 21, 2021 •

edited

Loading