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

outputEncoding not handled correctly when using sRGB render target in WebGL2 #23251

Closed
chubei-oppen opened this issue Jan 17, 2022 · 11 comments · Fixed by #23253
Closed

outputEncoding not handled correctly when using sRGB render target in WebGL2 #23251

chubei-oppen opened this issue Jan 17, 2022 · 11 comments · Fixed by #23253
Labels

Comments

@chubei-oppen
Copy link
Contributor

Describe the bug

In #23129, the outputEncoding is calculated differently from before. I believe the previous way is correct.

To Reproduce

See following code and fiddle.

Code

const renderer = new THREE.WebGLRenderer();
renderer.setSize( window.innerWidth, window.innerHeight );
renderer.outputEncoding = THREE.sRGBEncoding;
renderer.setPixelRatio( window.devicePixelRatio );
document.body.appendChild( renderer.domElement );

const geometry = new THREE.PlaneGeometry( 2, 2 );
const material = new THREE.MeshBasicMaterial();
const mesh = new THREE.Mesh( geometry, material );

const renderTarget = new THREE.WebGLRenderTarget( window.innerWidth, window.innerHeight, { encoding: THREE.sRGBEncoding } );

const camera = new THREE.OrthographicCamera();
camera.position.set( 0, 0, 1 );

new THREE.TextureLoader().load( 'https://threejs.org/examples/textures/crate.gif', (texture) => {

  texture.encoding = THREE.sRGBEncoding;

  material.map = texture;
  renderer.setRenderTarget( renderTarget );
  renderer.render( mesh, camera );

  material.map = renderTarget.texture;
  renderer.setRenderTarget( null );
  renderer.render( mesh, camera );

} );

Live example

Expected behavior

dev should behave the same as r136.

Screenshots

r136
image

dev
image

Platform:

  • Device: Desktop
  • OS: Linux
  • Browser: Chrome
  • Three.js version: dev
@chubei-oppen
Copy link
Contributor Author

Looks like the same problem also exists at

const encoding = ( _currentRenderTarget === null ) ? _this.outputEncoding : _currentRenderTarget.texture.encoding;
.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 17, 2022

The inline shader sRGB encode only needs to be done for the default framebuffer. Let me fix this with a PR.

Right now, the render target is sRGB encoded two times (inline and by WebGL).

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 17, 2022

I want to make clear that the fiddles are great for reproducing the issue.

However, the fiddles are probably no good approach for code used in production. In almost all cases, you only want to apply sRGB encoding when rendering to screen. Otherwise you encode/decode sRGB values in each pass of your FX chain since normally you want linear values in the shaders. This encoding/decoding overhead is waste from my point of view.

The fiddle should actually look like so: https://jsfiddle.net/2j4nzcra/

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 17, 2022

Related #23019.

@chubei-oppen
Copy link
Contributor Author

The fiddle should actually look like so: https://jsfiddle.net/2j4nzcra/

In production I'll still use code like the original fiddle.

Per my understanding, the purpose of allowing framebuffer color attachment format to be srgb8 is to make the limited representation power of 8 bit color better fit the perception of human eye. Encoding and decoding have performace overhead but often is worth it, especially when the scene is dark. I can make a fiddle with dark gradients to show the difference but you probably know what it's going to be like so I'll save the effort.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 17, 2022

I've referred with my comment to FX setups with multiple passes. sRGB should be applied at the end. Intermediate frame buffers should be linear.

@chubei-oppen
Copy link
Contributor Author

I've referred with my comment to FX setups with multiple passes. sRGB should be applied at the end. Intermediate frame buffers should be linear.

Sorry which comment?

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 17, 2022

#23251 (comment)

@chubei-oppen
Copy link
Contributor Author

Yes, we want linear values in the shaders, but srgb values in 8 bit images. I'll make a fiddle tomorrow to show the case.

@vanruesc
Copy link
Contributor

To provide some additional information on the topic:

Storing linear colors in 8bit buffers causes color degradation and banding. Linear colors should only be stored with at least 12 bits of precision per channel to avoid this (reference).

I've referred with my comment to FX setups with multiple passes. sRGB should be applied at the end. Intermediate frame buffers should be linear.

This is correct, but it requires HalfFloatType buffers to avoid banding which can have an impact on performance and may not be supported on some devices. Unity stores intermediate results in 8bit buffers for non-HDR post-processing setups by converting colors from linear to sRGB and back to linear (reference, see "Linear color space and non-HDR"). This obviously works best with hardware encoding/decoding support via SRGB8_ALPHA8 buffers, which was only recently added to three.

@chubei-oppen
Copy link
Contributor Author

To show how the effect looks like in three:

linear framebuffer. Color is quantized in linear space then converted to sRGB space for displaying. Notice the banding effect.
image

srgb framebuffer. Color is first converted to sRGB space then quantized and displayed. Much smoother.
image

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

Successfully merging a pull request may close this issue.

3 participants