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

Remove sRGB inline decoding. #23109

Closed
Mugen87 opened this issue Dec 29, 2021 · 16 comments · Fixed by #23129
Closed

Remove sRGB inline decoding. #23109

Mugen87 opened this issue Dec 29, 2021 · 16 comments · Fixed by #23129

Comments

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 29, 2021

I'm currently working on a PR to remove the inline sRGB decode from the shader. We want to do this to avoid wrong texture filtering which was discussed in earlier issues. The removal also simplifies the code base at various places (especially the WebGL 2 code paths).

The idea is to use SRGB8_ALPHA8 with WebGL 2 and a fallback for WebGL 1. In order to use SRGB8_ALPHA8, textures have to be in RGBA format and must use unsigned byte which is true for most color textures. However, there are a couple of issues with that approach and I want to discuss them step by step. The first problem is processing compressed textures holding sRGB encoded values like in:

const loader = new BasisTextureLoader();
loader.setTranscoderPath( 'js/libs/basis/' );
loader.detectSupport( renderer );
loader.load( 'textures/compressed/canestra_di_frutta_caravaggio.basis', function ( texture ) {
texture.encoding = THREE.sRGBEncoding;

How should we handle this use case? Decoding compressed texels values on the CPU is by definition no option. If you do a separate render pass and decode the texture into a render target, we would loose the compression (unless we compress it again which is no good idea either).

As far as I understand we can only support compressed textures with sRGB values if formats like SRGB8_ALPHA8_ASTC_4x4_Format are used. Or we have to keep the inline decode. Are there any other options?

@mrdoob
Copy link
Owner

mrdoob commented Dec 29, 2021

Maybe @richgel999 and/or @zeux have recommendations for what to do here?

@richgel999
Copy link

Not sure I fully follow you (as I'm not super familiar with three.js internals). It sounds like you're currently sampling sRGB textures using linear filtering (i.e. not telling the GPU's texture filtering unit that the texture is actually sRGB), then converting from sRGB->linear in the shader. (Right?)

It should be simple to just change the texture formats you're feeding WebGL to use the sRGB variants, and let the GPU do the filtering itself, with no inline sRGB decode. It shouldn't be necessary to decode the .basis/.ktx2 texture data on the CPU - the texture data should remain unchanged.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Dec 29, 2021

It sounds like you're currently sampling sRGB textures using linear filtering (i.e. not telling the GPU's texture filtering unit that the texture is actually sRGB), then converting from sRGB->linear in the shader. (Right?)

Yes, that is right.

It should be simple to just change the texture formats you're feeding WebGL to use the sRGB variants, and let the GPU do the filtering itself, with no inline sRGB decode.

That is what I had in mind, too. However, this means code like in the above example needs be changed. And three.js has to establish a policy that only certain texture setups support the sRGB workflow.

Looking at the current constants that would be all SRGB8_ALPHA8_ASTC* formats and RGBAFormat (both UnsignedByteType). For all other textures sRGBEncoding can't be used.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Dec 30, 2021

Looking at the current constants that would be all SRGB8_ALPHA8_ASTC* formats and RGBAFormat (both UnsignedByteType). For all other textures sRGBEncoding can't be used.

For Basis and KTX2 to work properly, we need the ability to transcode to the formats listed here. The rough prioritization of those formats is given in a series of charts from the KTX2 guidelines. Can WebGL not do sRGB conversion for compressed formats other than ASTC? 😕

  • ASTC, BC1, BC3, BC4, BC5, BC7, ETC1, ETC2, PVRTC1

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Dec 30, 2021

Can WebGL not do sRGB conversion for compressed formats other than ASTC? 😕

Yes, it can. However, three.js does not support them yet. After looking at the extensions the following sRGB formats could be added:

  • WEBGL_compressed_texture_etc
    • COMPRESSED_SRGB8_ETC2
    • COMPRESSED_SRGB8_PUNCHTHROUGH_ALPHA1_ETC2.
    • COMPRESSED_SRGB8_ALPHA8_ETC2_EAC
  • WEBGL_compressed_texture_s3tc_srgb
    • COMPRESSED_SRGB_S3TC_DXT1_EXT
    • COMPRESSED_SRGB_ALPHA_S3TC_DXT1_EXT
    • COMPRESSED_SRGB_ALPHA_S3TC_DXT3_EXT
    • COMPRESSED_SRGB_ALPHA_S3TC_DXT5_EXT
  • EXT_texture_compression_bptc
    • COMPRESSED_SRGB_ALPHA_BPTC_UNORM_EXT

@donmccurdy
Copy link
Collaborator

At least internally within the rendered I think we should use those, yes – thanks! In terms of the user-facing API, I do have a concern. See #23116 (comment).

@mrdoob
Copy link
Owner

mrdoob commented Dec 31, 2021

Hmm... VideoTexture will be affected by this, right?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Dec 31, 2021

Are video textures supposed to be in sRGB? Maybe we can just state to always use LinearEncoding?

@mrdoob
Copy link
Owner

mrdoob commented Dec 31, 2021

Well, if someone is doing a video player (think YouTube in VR) would be a hard task to recompress their whole library... 🤔

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Dec 31, 2021

It seems when changing the format of a video textures to RGBAFormat, they can use SRGB8_ALPHA8. Tested with webgl_video_panorama_equirectangular.

However, since we are not using gl.texStorage2D() this will run slow on Windows. At least until https://bugs.chromium.org/p/chromium/issues/detail?id=1256340 is solved.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Dec 31, 2021

Live example: https://jsfiddle.net/uzgo9fh4/3/

@mrdoob
Copy link
Owner

mrdoob commented Jan 3, 2022

Could we add inline sRGB decoding only for VideoTextures until that crbug is solved?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jan 4, 2022

I've added a solution here: 57ac477

Should be easy to revert when the Chromium bug is solved.

Just for the protocol: VideoTexture does not use gl.texStorage2D() right now and with the above commit no SRGB8_ALPHA8 format. The sRGB decode happens inline and only if the texture is assigned to the map property.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jan 4, 2022

BTW: Is #21874 (comment) still true? In what way has Chrome no optimized support for video textures?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jan 4, 2022

VideoTexture does not use gl.texStorage2D() right now

A usage is basically possible if we know the dimensions of the texture in advance. Right now, when a video texture is marked for an update in VideoTexture for the first time, the dimensions are 0. However, it is possible to change the code such that videoWidth and videoHeight report the correct dimensions and gl.texStorage2D() does not complain.

@mrdoob
Copy link
Owner

mrdoob commented Jan 4, 2022

I've added a solution here: 57ac477

Excellent!

BTW: Is #21874 (comment) still true? In what way has Chrome no optimized support for video textures?

I'll check.

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

Successfully merging a pull request may close this issue.

4 participants