Skip to content

Documented guaranteed swap chain formats and validation - #1185

Merged
toji merged 6 commits into
gpuweb:mainfrom
toji:swap-chain-formats
Nov 30, 2020
Merged

Documented guaranteed swap chain formats and validation#1185
toji merged 6 commits into
gpuweb:mainfrom
toji:swap-chain-formats

Conversation

@toji

@toji toji commented Oct 27, 2020

Copy link
Copy Markdown
Member

Adds some text indicating which formats swapchains must support, and uses it in some validation for the swap chain configuration.

I suspect this will need some iteration to get to a happy place, and once the editors are good with the layout we'll probably want to get confirmation from the WG that the documented formats are indeed what everyone agrees on. I have some questions/concerns to start:

  • It feels a bit odd to me to document this kind of requirement in the description of an internal slot, but I'm not sure where else would be appropriate? We could add yet another column to the Plain color formats table but I'm wary of making that too much larger, especially if the column only applies to 2-3 formats.
  • What role does the GPUCanvasContext play in controlling the supported formats? I have all that info stored on the device now, but you wouldn't need to get the preferred format from the context if it didn't have some effect on the outcome. SHould both the device and the context have a list of supported formats and the validation happens against the ANDed union of the two? Similarly should each have a list of supported formats in order of preference and the preferred format returns the first entry that appears in both lists? If so, what is the context's list generated from?
  • My assumption is that we're avoiding returning an explicit list of supported formats via the IDL in order to avoid fingerprinting concerns, correct?

Preview | Diff

@toji
toji requested review from kainino0x and kvark October 27, 2020 16:37
@kainino0x

Copy link
Copy Markdown
Contributor

The idea is that both the device and the canvas may have some impact on the preferred type. You could imagine having a canvas on one display that's HDR/WCG, and one on another display that's not, and the preferred types could be different. In practice I'm not 100% sure how browsers will use this info especially given canvases can be moved between displays.

Probably the best way to store info that's keyed on [device, canvas] pairs is an internal map<canvas, ...> on the device.

The list of supported formats should be very small, and in fact I think we should enforce by spec that the only allowed formats are the guaranteed formats + the single value returned by getSwapChainPreferredFormat. It would be good to even guarantee that you can't use the preferred format unless you've queried it.

I think the returned value of getSwapChainPreferredFormat should be entirely implementation defined except perhaps we say "it must be one of this specified set, but note that the set might expand in later revisions of the spec."

@toji

toji commented Oct 27, 2020

Copy link
Copy Markdown
Member Author

Awesome, thanks for the feedback!

I rearranged things quite a bit after considering your suggestions. I am using a map now, but it's stored on the context and keyed off the device. (Felt like GPUDevice has enough going on in it already.) Also, I like your suggestion of only supporting the guaranteed formats + preferred, and only if the preferred is queried. I constructed the algorithms to reflect that explicitly now. I also indicate that the user agent is free to select whatever preferred format it wants, but the algorithm is structured in such a way that it enforces that you'll only ever get one answer for any given context/device pair.

Comment thread spec/index.bs Outdated
**Returns:** {{Promise}}&lt;{{GPUTextureFormat}}&gt;

Issue: Describe {{GPUCanvasContext/getSwapChainPreferredFormat()}} algorithm steps.
1. Let |promise| be [=a new promise=].

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The more I think about it, the more it seems like this could be synchronous: either the canvas is HDR and the preferred format is rgba16float or rgb10a2unorm depending on how it was configured, or it is not HDR in which case it is best as bgra8unorm on all OSes except Android where it is best rgba8unorm. Basically we know synchronously on canvas context creation what's the best format.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we need to put some more thought into this (with input from platform/canvas experts) because, like I mentioned above, I'm not exactly sure how browsers are going to handle this. Why do we need the canvas - is it because we can check which display the canvas is on? What happens if the canvas moves between displays, does the result here change? Do we instead provide the "max" of all the attached displays? What happens if there's a non-HDR display and an HDR display and you unplug the HDR display?

@github-actions

github-actions Bot commented Nov 9, 2020

Copy link
Copy Markdown
Contributor

Previews, as seen at the time of posting this comment:
WebGPU | IDL
WGSL
63a5e2a

@toji

toji commented Nov 9, 2020

Copy link
Copy Markdown
Member Author

Update this PR to take into account conclusions from today's call. PTAL!

@kainino0x kainino0x left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I thought one of the outcomes from today's call was that we would have fixed sets of allowed formats, consistent across all systems, and the preferred format would always be one of them. So for the default case, this might just be bgra8unorm and rgba8unorm. Maybe we should write the spec for now such that only those two are allowed?

In particular, I'm not sure whether it makes sense to have the -srgb variants in there by default. They might only be available when the canvas context has been created with CanvasColorEncodingEnum "unorm8-srgb"? See https://github.com/WICG/canvas-color-space. (Also, note it currently says "Support for "unorm8" is mandatory. All other encodings are optional." I would very much like to be able to require srgb support...)

HDR is probably just rgba16float, though maybe we need rgb10a2unorm, I'm not sure (note CanvasColorEncodingEnum doesn't have such a thing now). Similarly we may only allow these formats if the canvas context has been created with some special flags in the first place.

Alternatively, maybe we shouldn't use the CanvasColorEncodingEnum part of canvas-color-space at all, and instead express it using our own (more concrete) texture formats in swap chain creation.

Comment thread spec/index.bs Outdated
Comment thread spec/index.bs Outdated
@Kangz

Kangz commented Nov 10, 2020

Copy link
Copy Markdown
Contributor

I thought one of the outcomes from today's call was that we would have fixed sets of allowed formats, consistent across all systems, and the preferred format would always be one of them. So for the default case, this might just be bgra8unorm and rgba8unorm. Maybe we should write the spec for now such that only those two are allowed?

I think @jdashg shared that on some hardware the optimal format for canvases is bgra8unorm-srgb so it could make sense to have both that format and rgba8unorm-srgb in the list too.

@toji
toji force-pushed the swap-chain-formats branch from 3f9cfc3 to e7f5c1b Compare November 10, 2020 16:39
@github-actions

Copy link
Copy Markdown
Contributor

Previews, as seen at the time of posting this comment:
WebGPU | IDL
WGSL
3f9cfc3

@github-actions

Copy link
Copy Markdown
Contributor

Previews, as seen at the time of posting this comment:
WebGPU | IDL
WGSL
e7f5c1b

@toji

toji commented Nov 10, 2020

Copy link
Copy Markdown
Member Author

I thought one of the outcomes from today's call was that we would have fixed sets of allowed formats, consistent across all systems, and the preferred format would always be one of them. So for the default case, this might just be bgra8unorm and rgba8unorm. Maybe we should write the spec for now such that only those two are allowed?

I missed that particular distinction, thanks for catching it. I've limited the list to bgra8unorm, rgba8unorm, and their sRGB variants (on Corentin's suggestion) and changed the preferred format algorithm to only select one of those formats. This simplifies things quite a bit, though I expect some complexity to be added back once we start handling HDR.

Comment thread spec/index.bs Outdated
Comment thread spec/index.bs
Comment thread spec/index.bs Outdated
@toji

toji commented Nov 17, 2020

Copy link
Copy Markdown
Member Author

Updated to simplify the preferred format and move it to the adapter as Kai suggested. Left the sRGB formats in the supported list since that's the outcome I recall from yesterday's call. I also vaguely remember someone (maybe @jdashg?) suggesting that a FP format should be in that list as well but couldn't find reference to that in the minutes so I've omitted it for now.

@kainino0x

Copy link
Copy Markdown
Contributor

Still LGTM, though we (at least I) still need to do some work understanding how srgb should work exactly.

We should also figure out separately:

  • HDR and float16 etc
  • getSwapChainPreferredFormat with a list of "formats the app can understand"
  • How this interacts with CanvasColorEncodingEnum, if we expose both
  • How to expose display-specific preferred formats that could change when a window is moved between displays

Comment thread spec/index.bs Outdated
Comment thread spec/index.bs Outdated
@toji
toji force-pushed the swap-chain-formats branch from eb05f01 to 0ef0862 Compare November 30, 2020 23:26
@toji

toji commented Nov 30, 2020

Copy link
Copy Markdown
Member Author

Updated to remove the [[preferred_format]] slot on the GPUAdapter as we discussed on the call. Merging.

@toji
toji merged commit f5b0567 into gpuweb:main Nov 30, 2020
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