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

Add view_formats in TextureDescriptor #3237

Merged
merged 20 commits into from
Jan 18, 2023
Merged

Conversation

jinleili
Copy link
Contributor

@jinleili jinleili commented Nov 26, 2022

Checklist

  • Run cargo clippy.
  • Run RUSTFLAGS=--cfg=web_sys_unstable_apis cargo clippy --target wasm32-unknown-unknown if applicable.
  • Add change to CHANGELOG.md. See simple instructions inside file.

Connections
WebGPU spec: https://www.w3.org/TR/webgpu/#dom-gputexturedescriptor-viewformats

Description

Closes #3030

Testing
Tested on Metal, Vulkan(--features vulkan-portability) and Dx12 backend.

@jinleili jinleili force-pushed the view_formats branch 2 times, most recently from 735a35f to bf9c1c6 Compare November 26, 2022 13:22
@codecov-commenter
Copy link

codecov-commenter commented Nov 26, 2022

Codecov Report

Merging #3237 (8bbccdd) into master (4400ff8) will decrease coverage by 0.00%.
The diff coverage is 57.81%.

@@            Coverage Diff             @@
##           master    #3237      +/-   ##
==========================================
- Coverage   64.64%   64.63%   -0.01%     
==========================================
  Files          86       86              
  Lines       42722    42768      +46     
==========================================
+ Hits        27616    27645      +29     
- Misses      15106    15123      +17     
Impacted Files Coverage Δ
wgpu-core/src/present.rs 0.00% <0.00%> (ø)
wgpu-core/src/resource.rs 25.13% <ø> (ø)
wgpu/src/lib.rs 51.04% <33.33%> (-0.03%) ⬇️
wgpu/src/backend/direct.rs 56.98% <40.00%> (+0.11%) ⬆️
wgpu-core/src/device/mod.rs 66.73% <57.14%> (-0.07%) ⬇️
wgpu-types/src/lib.rs 87.45% <60.52%> (-0.62%) ⬇️
wgpu-core/src/command/clear.rs 89.58% <100.00%> (ø)
wgpu-core/src/command/transfer.rs 75.14% <100.00%> (ø)
wgpu-core/src/init_tracker/texture.rs 93.65% <100.00%> (ø)
wgpu-core/src/hub.rs 60.98% <0.00%> (-0.16%) ⬇️
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@jinleili jinleili requested a review from crowlKats as a code owner November 26, 2022 13:34
@crowlKats
Copy link
Collaborator

Could the deno webgpu part be implemented fully/properly?

Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for tackling this one!

The code is fine, but there are two extra parts we need (besides the deno stuff).

First can you add a test to make sure the feature works (read a unorm texture as srgb and validate it does the conversion - and vise versa)

We also need a downlevel feature for this, as gles doesn't support texture views. And afaik we can't polyfill this feature in any way.

wgpu-types/src/lib.rs Outdated Show resolved Hide resolved
wgpu-types/src/lib.rs Outdated Show resolved Hide resolved
wgpu-core/src/resource.rs Outdated Show resolved Hide resolved
wgpu-core/src/device/mod.rs Outdated Show resolved Hide resolved
@jinleili
Copy link
Contributor Author

@crowlKats I'm sorry, I don't know how to properly implement view_formats in deno_webgpu. deno_webgpu's webgpu.idl doesn't have viewFormats field, is it enough to add viewFormats field to deno_webgpu::texture::CreateTextureArgs?

@crowlKats
Copy link
Collaborator

crowlKats commented Nov 27, 2022

no, its slightly more than just that. but no worries, i will do it then

wgpu-types/src/lib.rs Outdated Show resolved Hide resolved
wgpu-types/src/lib.rs Outdated Show resolved Hide resolved
@jinleili jinleili force-pushed the view_formats branch 6 times, most recently from f4dd6fc to 45ea4ca Compare December 1, 2022 05:13
@crowlKats crowlKats mentioned this pull request Dec 3, 2022
37 tasks
@khyperia
Copy link

Hi! I'm just a random user of wgpu, so take this whole comment with a grain of salt. Proper sRGB support in the webgpu backend of wgpu is blocked on specifying the view format of the Surface's current_texture (the swapchain image). In webgpu, this is done by setting GPUCanvasConfiguration.viewFormats to the formats you'd like to view the current_texture as, then specifying that format in create_view. (I guess this GPUCanvasConfiguration.viewFormats field is a sort of backdoor to pass in view_formats into the system's internal create_texture for the current_texture?)

https://www.w3.org/TR/webgpu/#canvas-configuration

In wgpu, the GPUCanvasConfiguration is created here, and I suppose mapped.view_formats() should be called, similar to what this PR is already doing in the device_create_texture function in web.rs

wgpu/wgpu/src/backend/web.rs

Lines 1037 to 1041 in aa46e82

let mut mapped =
web_sys::GpuCanvasConfiguration::new(&device.0, map_texture_format(config.format));
mapped.usage(config.usage.bits());
mapped.alpha_mode(alpha_mode);
surface.0.configure(&mapped);

I suppose SurfaceConfiguration should get a new view_formats: Vec<TextureFormat> field, similar to how TextureDescriptor got that field in this current PR? I haven't looked into how other backends other than webgpu handle the view format of the surface current_texture, and if they support alternate views at all, and how they're configured/enabled.

If you could implement this in this PR or a followup, that would be amazing, but please ping me if you don't plan on doing so and I can try looking into implementing it!

@jinleili
Copy link
Contributor Author

The implementation of this PR is not yet accepted and may need to be modified. I have plans to implement view_formats in SurfaceConfiguration after this PR.

Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another round, we're very close!

wgpu-hal/src/gles/adapter.rs Outdated Show resolved Hide resolved
wgpu-core/src/device/mod.rs Outdated Show resolved Hide resolved
wgpu-types/src/lib.rs Outdated Show resolved Hide resolved
wgpu/src/lib.rs Outdated Show resolved Hide resolved
wgpu/tests/shader_view_format/mod.rs Show resolved Hide resolved
@jinleili jinleili force-pushed the view_formats branch 2 times, most recently from d9db149 to 5cd7541 Compare January 11, 2023 08:49
Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really good stuff, we're almost ready to go!

wgpu-types/src/lib.rs Outdated Show resolved Hide resolved
wgpu-types/src/lib.rs Outdated Show resolved Hide resolved
wgpu/src/backend/direct.rs Outdated Show resolved Hide resolved
wgpu/src/util/device.rs Outdated Show resolved Hide resolved
@cwfitzgerald
Copy link
Member

Thank you for all the hard work!

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.

Question: Can we create a linear-space views of SRGB textures or SRGB views of linear-space textures?
5 participants