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 HDR settings to Particles sample #432

Merged
merged 6 commits into from
Jul 23, 2024

Conversation

beaufortfrancois
Copy link
Collaborator

@beaufortfrancois beaufortfrancois commented Jul 10, 2024

Results:

  • Works on Chrome Canary with --enable-blink-features=WebGPUHDR
  • Flickers a lot on Safari Tech Preview with experimental WebGPU support
  • Did not work on Firefox Nightly before this PR

@beaufortfrancois beaufortfrancois marked this pull request as draft July 10, 2024 14:53
@beaufortfrancois beaufortfrancois force-pushed the toneMapping branch 4 times, most recently from c738625 to a380fa4 Compare July 11, 2024 10:56
@beaufortfrancois beaufortfrancois marked this pull request as ready for review July 11, 2024 10:58
@beaufortfrancois
Copy link
Collaborator Author

@ccameron-chromium Please have a look as well

@beaufortfrancois beaufortfrancois changed the title Add toneMapping extended mode to Particles sample Add HDR settings to Particles sample Jul 11, 2024
Copy link
Collaborator

@greggman greggman left a comment

Choose a reason for hiding this comment

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

I think adding a HDR sample is great but I wonder if the particle example is a good example for it vs some dedicated sample? How will people know to go look at "particles" (guess you could rename it particles(HDR)?)

Also, I couldn't see any difference at a glance. I ran with

/Applications/Google\ Chrome\ Canary.app/Contents/MacOS/Google\ Chrome\ Canary --use-data-dir=$T/delme-canray-6933 --enable-unsafe-webgpu --enable-webgpu-developer-features --enable-blink-features=WebGPUHDR

So it makes me wonder if rather than a toggle, whichever HDR sample we end up with (could be this one) shows 2 canvases. One HDR, one not, side by side. Then, that they are different would stick out?

sample/particles/main.ts Show resolved Hide resolved
@beaufortfrancois
Copy link
Collaborator Author

I think adding a HDR sample is great but I wonder if the particle example is a good example for it vs some dedicated sample? How will people know to go look at "particles" (guess you could rename it particles(HDR)?)

I've added the HDR bit to make it clearer. Thanks for the suggestion!

Also, I couldn't see any difference at a glance. I ran with

/Applications/Google\ Chrome\ Canary.app/Contents/MacOS/Google\ Chrome\ Canary --use-data-dir=$T/delme-canray-6933 --enable-unsafe-webgpu --enable-webgpu-developer-features --enable-blink-features=WebGPUHDR

I tried your command line and it works great on the Built-in Liquid Retina XDR Display of my Apple M1 Pro device. When plugged to an external non-HDR display, there's no difference as the display doesn't support HDR at all as expected. Note that you only need /Applications/Google\ Chrome\ Canary.app/Contents/MacOS/Google\ Chrome\ Canary --enable-blink-features=WebGPUHDR.

So it makes me wonder if rather than a toggle, whichever HDR sample we end up with (could be this one) shows 2 canvases. One HDR, one not, side by side. Then, that they are different would stick out?

I think the particles sample really shines with HDR. Traditional HDR demos like https://webkit.org/blog-files/color-gamut/ are "okay" to understand what is HDR but I think the webgpu-samples should focus on how the WebGPU API works in regards to HDR with toneMapping and format.

@beaufortfrancois
Copy link
Collaborator Author

FYI I've just added a warning if the display is not HDR compatible to set user's expectations.

sample/particles/index.html Show resolved Hide resolved
sample/particles/main.ts Show resolved Hide resolved
sample/particles/particle.wgsl Show resolved Hide resolved
@kainino0x kainino0x merged commit 8d143c2 into webgpu:main Jul 23, 2024
1 check passed
kainino0x added a commit that referenced this pull request Jul 23, 2024
@mwyrzykowski
Copy link
Contributor

Just tried it out on a MBP - works great! 🎉

@beaufortfrancois
Copy link
Collaborator Author

I've tested Android as well and it works great there also!
@mwyrzykowski Does https://webgpu.github.io/webgpu-samples/?sample=particles also work on iOS?

@mwyrzykowski
Copy link
Contributor

It does but WebKit doesn’t support HDR canvas yet. But it appears even on an iPhone 15 pro the sample thinks HDR is not supported:

IMG_1336

@beaufortfrancois
Copy link
Collaborator Author

I'm using window.matchMedia('(dynamic-range: high)') to test whether the display is HDR compatible or not. https://developer.mozilla.org/en-US/docs/Web/CSS/@media/dynamic-range#browser_compatibility says it should be supported on Safari on iOS 🤷 .

@mwyrzykowski
Copy link
Contributor

I'm using window.matchMedia('(dynamic-range: high)') to test whether the display is HDR compatible or not. https://developer.mozilla.org/en-US/docs/Web/CSS/@media/dynamic-range#browser_compatibility says it should be supported on Safari on iOS 🤷 .

Oh interesting, I reloaded the page and the 'display not compatible' banner disappeared, looks like the detection works

@beaufortfrancois
Copy link
Collaborator Author

It does but WebKit doesn’t support HDR canvas yet.

Is there a way to detect in JavaScript whether HDR canvas is supported instead of using this media query?

@mwyrzykowski
Copy link
Contributor

Is there a way to detect in JavaScript whether HDR canvas is supported instead of using this media query?

Apologies for the delay but I looked around and didn't see anything conclusive. Could we check for context.colorMetadata === undefined as that appears required by the HDR canvas specification? https://github.com/w3c/ColorWeb-CG/blob/main/hdr_html_canvas_element.md#add-hdr-rendering-behavior-and-hdr-metadata-to-canvasrenderingcontext2dsettings

Or should one be able to use HDR in WebGPU without an higher bit depth canvas (e.g., R10G10B10A2 type formats)?

@beaufortfrancois
Copy link
Collaborator Author

context.colorMetadata is not implemented in Chromium sadly.

And GPUCanvasConfiguration is a dictionary, not an interface, so I can't check without calling configure() first whether toneMapping is implemented.

I wonder if I should leave it as is.

@mwyrzykowski
Copy link
Contributor

It’s certainly fine with me to leave as-is 👍

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