-
-
Notifications
You must be signed in to change notification settings - Fork 35.9k
WebGPURenderer: Add warning when video textures with invalid color spaces are used. #31569
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
Conversation
📦 Bundle sizeFull ESM build, minified and gzipped.
🌳 Bundle size after tree-shakingMinimal build including a renderer, camera, empty scene, and dependencies.
|
src/renderers/common/Textures.js
Outdated
|
||
if ( texture.isVideoTexture && texture.colorSpace !== SRGBColorSpace ) { | ||
|
||
console.warn( 'Textures: VideoTexture is rarely used with non-SRGB color space.' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about:
console.warn( 'WebGPURenderer: VideoTexture must be used with sRGB color space.' );
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would we not allow putting data into a video, annotated with NoColorSpace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not with the current implementation. The copy of video data uses copyExternalImageToTexture()
and that method only support sRGB and Display-P3 data. See related PR #31416.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see! Ok, good to know. Hopefully something we can loosen later, but no objection to the warning in the meantime.
src/renderers/common/Textures.js
Outdated
|
||
if ( texture.isVideoTexture && texture.colorSpace !== SRGBColorSpace ) { | ||
|
||
console.warn( 'Textures: VideoTexture is rarely used with non-SRGB color space.' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would we not allow putting data into a video, annotated with NoColorSpace?
src/renderers/common/Textures.js
Outdated
@@ -326,6 +326,14 @@ class Textures extends DataMap { | |||
|
|||
this.info.memory.textures ++; | |||
|
|||
// | |||
|
|||
if ( texture.isVideoTexture && texture.colorSpace !== SRGBColorSpace ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you may want to check for the sRGB transfer functions, not color space. For example Display P3 uses sRGB transfer functions, and should work fine. Example:
if ( texture.isVideoTexture && texture.colorSpace !== SRGBColorSpace ) { | |
if ( texture.isVideoTexture && ColorManagement.getTransfer( texture.colorSpace ) !== SRGBTransfer ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would we not allow putting data into a video, annotated with NoColorSpace?
I initially considered this use case, so the warning only mentioned rare scenarios...
Refine warning.
This PR continues the work from #31534