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

Force linear encoding for WebXR render targets #24698

Merged
merged 3 commits into from
Sep 27, 2022
Merged

Conversation

cabanier
Copy link
Contributor

When the WEBGL_multisampled_render_to_texture extension is not available, three.js falls back to using renderbuffers to get support for multisampled rendering.
However, there was a change that forces the renderbuffer to sRGB. This result in a GL error when it tries to copy the MSAA renderbuffer into the WebXR texture. WebXR is special in that its texture format is RGB but the three.js renderer is set to sRGB.

The fix now correctly flags the getInternalFormat call for WebXR to not choose sRGBA.

This contribution is funded by Meta

@donmccurdy
Copy link
Collaborator

donmccurdy commented Sep 26, 2022

Thanks @cabanier! I'm worried that something feels off about putting an iswebxr flag on the WebGLRenderTarget class, though. Trying to understand what this new override does, is the idea that THREE.WebGLRenderer applies the necessary "Linear-sRGB → sRGB" transform in its material shaders, and so an additional "Linear-sRGB → sRGB" transform at the render target level would be redundant/incorrect? If so, is there something we could do to reverse that, to disable output encoding in GLSL and apply the transform through the render target?

I assume we cannot really be transmitting frames to the WebXR device in Linear-sRGB at 8-bit precision; the quality loss would be large.

Also consider that a WebGL application with multiple post-processing steps might use sRGB for intermediate render targets, even if the last step required Linear-sRGB output, to make better use of the limited 8-bit precision before a final tone-mapping step.

@cabanier
Copy link
Contributor Author

cabanier commented Sep 26, 2022

For WebXR, the quest browser allocates an RGBA texture and then uses the msaa_render_to_texture extension to directly render to multisampled pixels.
This fix is for devices that don't have support for this extension. In that case, we want to render to an RGBA multisampled renderbuffer and then blit the contents to the WebXR Layer's texture. Because blitframebuffer requires the colorspaces to be the same, both renderbuffer and texture need to have the rgba format.

We can't set the WebXR texture to sRGB because that will cause double conversion which makes the output too light.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Sep 26, 2022

If you're hoping to have this change included in r145 this month, it isn't my intention to block the PR. I'll defer to @Mugen87 on a short-term fix.

However, it sounds like our compromise for lack of real color management in the HTML Canvas API in traditional WebGL applications has leaked into our WebXR implementation, if we are relying on inline GLSL in material shaders to apply the Linear-sRGB → sRGB transform here. This is not an ideal solution even for traditional WebGL apps, and is something we'd remove in a heartbeat if the Canvas API allowed us (see #23019).

We can't set the WebXR texture to sRGB because that will cause double conversion which makes the output too light.

If we disabled the initial conversion (currently applied by inline GLSL material shaders), it would be fine to make the WebXR texture and the renderbuffer sRGB, then?

@cabanier
Copy link
Contributor Author

If you're hoping to have this change included in r145 this month, it isn't my intention to block the PR. I'll defer to @Mugen87 on a short-term fix.

No, I'm not in a hurry. Fixing WebXR for non-quest devices also requires patches in the WebXR Layers polyfill.

We can't set the WebXR texture to sRGB because that will cause double conversion which makes the output too light.

If we disabled the initial conversion (currently applied by inline GLSL material shaders), it would be fine to make the WebXR texture and the renderbuffer sRGB, then?

The Quest browser currently ignores requests for sRGB because there were too many three.js experiences that suffered from the double conversion problem.
If you can disable the initial conversion, we can remove that restriction on our side. I think most experiences no longer request sRGB incorrectly so this shouldn't cause too many breakages.

@donmccurdy
Copy link
Collaborator

By default, three.js disables the initial conversion when rendering to any render target. Currently .isXRRenderTarget creates an exception, opting in to the initial conversion. To disable the initial conversion and do output encoding correctly (once the Quest Browser supports sRGB output textures), we'd revert that exception:

dev...donmccurdy:three.js:hotfix/webxr-rtt-srgb

Context for others reading, this goes back to:

The Quest browser currently ignores requests for sRGB because there were too many three.js experiences that suffered from the double conversion problem.

It appears to me that the double conversion would be caused by #23288, and by reverting that change (and adding support for sRGB output in Oculus Browser) we avoid that risk. Does that sound correct?

So the painful part of this change would be that users of older three.js versions will see incorrect colors (double conversion) after Oculus adds sRGB support, until they upgrade to the latest three.js release. That's unfortunate, but disabling sRGB at the browser level also doesn't seem sustainable1. 😕

I don't suppose we can detect support for sRGB output in Oculus Browser somehow, roll this out conditioned on browser support, and give developers time to make the update?


In general I would advise developers using three.js + WebXR to initialize their application with the following settings, from our color management documentation:

renderer.outputEncoding = THREE.sRGBEncoding;
renderer.physicallyCorrectLights = true;
THREE.ColorManagement.legacyMode = false;

Each of these settings will make things easier down the road. When post-processing is involved, things are a bit more complex.


1 Related workaround in Wolvic

@donmccurdy
Copy link
Collaborator

donmccurdy commented Sep 27, 2022

Just to be explicit — colors written by THREE.WebGLRenderer to the texture have Rec. 709 (sRGB) primaries, sRGB transfer function, and D65 white point (assuming .outputEncoding=sRGBEncoding). The encoding of the colors is already nonlinear, despite references to "forcing linear encoding" in these workarounds. If WebXR requires linear data we can provide that (with implications for precision), but it worries me to provide one thing and call it another.

@cabanier
Copy link
Contributor Author

cabanier commented Sep 27, 2022

The Quest browser currently ignores requests for sRGB because there were too many three.js experiences that suffered from the double conversion problem.

It appears to me that the double conversion would be caused by #23288, and by reverting that change (and adding support for sRGB output in Oculus Browser) we avoid that risk. Does that sound correct?

I think the sRGB problem predates that change.

So the painful part of this change would be that users of older three.js versions will see incorrect colors (double conversion) after Oculus adds sRGB support, until they upgrade to the latest three.js release. That's unfortunate, but disabling sRGB at the browser level also doesn't seem sustainable1. 😕

I don't suppose we can detect support for sRGB output in Oculus Browser somehow, roll this out conditioned on browser support, and give developers time to make the update?

We currently print a warning to the console when creating an sRGB layer and create it as RGB instead. Is there a way to query a texture for its format?

FWIW the whole RGB workflow is messed up, at least on Quest in order to match the colors that are produced when drawing to an HTML canvas. I'd be very happy to sit down one day with other vendors and work out what it should look like.

@mrdoob mrdoob merged commit 9e36830 into mrdoob:dev Sep 27, 2022
@mrdoob
Copy link
Owner

mrdoob commented Sep 27, 2022

Thanks!

@mrdoob mrdoob added this to the r145 milestone Sep 27, 2022
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.

4 participants