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

WebGLRenderer: Avoid default color space conversion. #21336

Merged
merged 2 commits into from
Feb 22, 2021

Conversation

takahirox
Copy link
Collaborator

Related issue: #21318, #21323, #21324

Description

This PR is an alternative of #21323 and #21324, and resolves #21318. Please refer to them for the detail of this change.

@mrdoob mrdoob added this to the r126 milestone Feb 22, 2021
@mrdoob mrdoob changed the title No color space conversion Avoid color space conversion Feb 22, 2021
@mrdoob mrdoob merged commit 0038988 into mrdoob:dev Feb 22, 2021
@mrdoob
Copy link
Owner

mrdoob commented Feb 22, 2021

Thanks!

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 22, 2021

@mrdoob I wonder if UNPACK_COLORSPACE_CONVERSION_WEBGL was the root cause for #20786. Maybe RGBM files can now be loaded correctly on iOS even with TextureLoader 🤔 ?

TBH, UNPACK_COLORSPACE_CONVERSION_WEBGL confused me quite a bit. I'm surprised that color textures with encoding set to sRGBEncoding were decoded from srgb to linear not twice in the past.

The WebGL standard also mentions that it is up to the browser if a color space conversion is applied on how the conversion is implemented. Sounds quite unreliable. One reason more to disable it.

@Mugen87 Mugen87 changed the title Avoid color space conversion WebGLRenderer: Avoid default color space conversion. Feb 22, 2021
@lexaknyazev
Copy link
Contributor

were decoded from srgb to linear not twice in the past

AFAIR, all that setting does is comparing the image file profile (when present) with some "system default" profile (e.g. sRGB), performing color-matching, and (re)encoding corrected values (usually to 8-bit sRGB).

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 22, 2021

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 22, 2021

AFAIR, all that setting does is comparing the image file profile (when present) with some "system default" profile (e.g. sRGB), performing color-matching, and (re)encoding corrected values (usually to 8-bit sRGB).

Okay, that would at least explain why the sRGB decode works as expected in the past. But the stackoverflow thread and the RGBM issue on iOS seems to be related to UNPACK_COLORSPACE_CONVERSION_WEBGL.

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 22, 2021

The following fiddle is the RGBM example with the dev build and TextureLoader. Can somebody with an iOS device check if the same banding occurs like in #20786?

https://jsfiddle.net/6hjtzy8x/

@mrdoob

This comment has been minimized.

@mrdoob
Copy link
Owner

mrdoob commented Feb 22, 2021

Okay, found the reason Safari is broken: e7ba155
And after fixing that I get this on Safari/macOS:
https://mrdoob-sandbox.glitch.me/webgl_loader_texture_rgbm.html

Screen Shot 2021-02-22 at 8 51 59 PM

@lexaknyazev

This comment has been minimized.

@mrdoob

This comment has been minimized.

@takahirox takahirox deleted the NoColorSpaceConversion branch February 22, 2021 23:17
@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 23, 2021

Okay, then #20786 is indeed unrelated to UNPACK_COLORSPACE_CONVERSION_WEBGL. After some more testing, it's about premultiplying alpha which behaves differently on iOS. So we definitely need a custom RGBM loader.

@mrdoob
Copy link
Owner

mrdoob commented Feb 23, 2021

@Mugen87 It's worth noting that Safari Technical Preview seem to have this issue fixed:

Screen Shot 2021-02-23 at 2 07 46 PM

So I guess we'd just need the workaround until summer?

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 23, 2021

Yes, probably. Maybe revisiting this issue in second half of 2021?

@takahirox
Copy link
Collaborator Author

Does anyone here know if embedded custom color space is often used in practical? I'm curious to know how much this change affects the existing Three.js applications. And this is a breaking change so I'm going to write a note that we recommend pre apply the transformation in Migration guide.

@lexaknyazev
Copy link
Contributor

Based on my experience, here are the most often cases:

  • a regular sRGB profile embedded just in case (safe to ignore on the web);
  • a wide gamut profile is embedded in screenshots taken on P3 displays on macOS (ignoring it leads to compressing image colors);
  • photos not prepared for web (may contain anything);
  • images authored for printing (may even use CMYK for pixel values).

@takahirox
Copy link
Collaborator Author

Thanks for the explanation. It sounds like to me that embedded extra image information is beyond the use of web 3D and avoid default color space conversion and request pre apply the color transformation may be a good direction as Web based 3D engine for portability and performance.

I added the note to Migration Guide. Please feel free to reword it because English is not my first language.

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.

TextureEncodingTest.gltf is different from expected result
4 participants