Minutes 2023 02 01

GPU Web 2023-02-01/02 APAC-timed

Note that unless stated otherwise this is a GPU for the Web community group meeting and not a working group meeting.

Chair: KG

Scribe: KR

Location: Google Meet

Tentative agenda

• Administrivia
• CTS Update
• “Tacit Resolution” Queue
• Use automatic expiry for imported textures, remove .expired #3633
• Why does depthClearValue default to 0.0? #3777
• WebGPU: Forbid null code point '\0' (U+0000) within strings #3576
• Should the API enforce that writable storage buffer bindings don't alias each other? #1842
  • “Does this validation apply across all bindings in bind groups in the currently-bound pipeline layout at draw/dispatch time. OR does it only apply to bindings that are actually statically used in the shader at draw/dispatch time?”
• It should be possible to unbind a bind group / vertex buffer #3787
• Agenda for next meeting

Attendance

• Apple
  • Dan Glastonbury
  • Myles C. Maxfield
• Google
  • Brandon Jones
  • Gregg Tavares
  • Kai Ninomiya
  • Ken Russell
• Intel
  • Jiawei Shao
  • Shaobo Yan
  • Zhaoming Jiang
• Microsoft
  • Rafael Cintron
• Mozilla
  • Jim Blandy
  • Kelsey Gilbert

Administrivia

• Agenda for the F2F

CTS Update

• KN: Gregg hard at work on tests. MM appear to indicate that work is ongoing on the Webkit side.
• Loko landed a rewrite of the texture helpers. Removes older code, makes them more discoverable. Updated many tests.
• Erich's continuing integration of CTS in Firefox's CI. Collating the many failures. Things like requesting a device not matching how the CTS does it.

“Tacit Resolution” Queue

Use automatic expiry for imported textures, remove .expired #3633

• KG: consensus at Moz was that this seems what we agreed on
• KN: did you read through?
• KG: not in detail, but think this is what we wanted
• KN: nothing to call out. Some notes like - this allows the app to detect duplicate imports, but have to still handle the same frame being wrapped multiple times.
• KG: merged.

Why does depthClearValue default to 0.0? #3777

• KG: consensus is - we picked 0, thinking maybe 1 would have been a better idea
• BJ: as Corentin mentioned - it's common nowadays to see reversed-depth, so 0.0 would be a good choice. From Web IDL stance - desire is that if there's a default, they prefer the default be false-y, for a variety of reasons. Undefined is false-y. If you have a boolean - should phrase the property so the boolean can default to false. I think the same principle applies here. Default of 0.0 will probably be less surprising than 1.0 even if there are many apps that won't use the default.
• KN: guidance - don't make things default to true. Not sure if I've seen guidance to avoid other truthy things, but makes sense.
  • https://webidl.spec.whatwg.org/#idl-operations

  • It is strongly suggested not to use a default value of true for boolean-typed arguments, as this can be confusing for authors who might otherwise expect the default conversion of undefined to be used (i.e., false). [API-DESIGN-PRINCIPLES]

  • bzbarsky:

The reason for that is that I think the behavior of such an argument 
when combined with the conventions for undefined is very surprising. 
Consider a function declared as:

   void foo(optional boolean arg = foo);

and these two patterns of calling it:

   if (myValue) {
     foo(true);
   } else {
     foo(false);
   }

and

   foo(myValue);

These two patterns have different behavior, which seems very unintuitive 
to me and to everyone else I've pointed this out to...

• BJ: will copy in that advice if I can find it.
• KG: happy to go to the TAG about choosing a truthy value. Most people do this, and the setup's usually that if you choose 0.0 you'll often wind up with a blank screen.
• KN: few reasons I don't want to default to 1. Not opposed to requiring it. Little different from the other clear values. Not convinced it's worth making a change here right now.
• KG: I think it is worth making a change here. Think it'll prevent footguns in a way that's more valuable than the initial-default guidance.
• DG: just contacted Mike - he raised the issue about removing the default?
• KG: therefore requiring you to state a preference? Generally like that - if we have too many "pure" aspects like this we end up writing too much boilerplate. If we envision it's written most of the time, a way to enforce that.
• DG: other than that, Mike says it's fine to leave it as 0.0 for consistency, but agree that it's a potential footgun.
• JB: default value should probably be the "quiescent" value - the one that doesn't do anything. 1.0 is that value for depth buffers. 0 is the "most true" value for depth buffers.
• BJ: looked at other APIs. Metal, OpenGL and D3D default to 1.0. Trying to find it in Vulkan.
• KG: that's where my strong preference comes from.
• BJ: Vulkan appears to be the only one that doesn't have a clear. Implicitly means that Vulkan defaults to 0 - because you want to zero out your structs before you use them.
• KG: hard to resolve tiny things like this live. Would like to change this to 1. How strongly do people feel about keeping it as 0.0?
• KN: also don't want to change the default. I prefer a more loudly breaking change at this time.
• BJ: I agree with that. Trying to wrap up the rest of the API. Could easily sneak in and surprise people.
• KG: I'll more strongly propose that we do something neither of us want - make it required.
• KN: I like that better.
• BJ: probably the way to go.
• Resolved - make depthClearValue required.
• KN: reminds me of - one other depth-related default that's odd - depth write enabled. Defaults to false.
• KG: that should be true.
• KN: want things to be more aligned with the way the APIs work, not more WebIDL-like.
• KR: just checked OpenGL - depth writes are enabled by default there.
• KN: a bit of an oddity. Is changing that to true the right decision?
• KG: when we previously decided these things, we probably considered the false-y criterion too strongly.
• KN: this is one that shows up frequently in code reviews. Myles probably proposed these defaults, maybe Metal defaults to false.
• KG: depth write enabled - true default in OpenGL, false in Metal. Are these defaults too centric on Metal's behavior?
• KN: probably 2.5 years ago…
• MM: don't know whether group discussed the defaults. Depth write enabled defaulting to false seems logical to me - same as blending, which defaults to false. Just want to get a triangle on the screen. But, a weak argument.
• KG: seems compelling.
• BJ: if you provide a depth buffer, most of the time you'd want to do depth writes. Can go either way.
• KN: noise from making it required isn't that big. Only specify it when you specify that. Don't want silent breaking changes. Would lean toward making this required. Don't feel strongly.
• MM: how'd we go from clear value to depth writes?
• KN: it reminded me of the topic. :)
• KG: absent strong feelings, think we should leave depth writes enabled alone.
• GT: think there are a bunch of parts of the API where it tries to the right thing with its defaults. Here, if I make a depth buffer, I want to use it and would like it to just work. Have to set compare to less, depth writes to true, and default clear to 1. My ideal world - create the buffer, let it do its default thing.
• KG: that's my preference too. Feels heavyweight compared to classic depth buffer setup code.
• MM: our opinions are stronger about the default clear value. 1.0's better.
• KG: is requiring it and not having a default an acceptable option?
• MM: it's worse. When someone wants to do depth testing, they just want to create the depth buffer. While people do reversed-depth, it's not common.
• KG: think we should still be open to change. Compromise of requiring it could be revisited later.
• KN: if we have a breaking change, I'd prefer it be loud.
• MM: think it would be acceptable to make it required for a bit, then change the default to 1.0.
• BJ: happy with that strategy.
• KN: mixed feelings. Discussing this earlier, there was a sense that we shouldn't treat forward-depth more specially than reversed-depth given that reversed-depth works equally well or better than forward-depth. Do we need the defaults set you up for forward depth?
• MM: the word "depth" is the key thing. Larger things - bigger values.
• KG: plan of record: change to required for now, and either in post-V1-polish or before V1, change the default again to 1.0.
• BJ: i like the idea of making it required - gives us flexibility. Defaulting to either one - can't tell which was a good/bad idea.
• KG: proposal: think we should do it for depth clear and depth write enabled. Gives us the chance to choose things later.
• KN: I'll file an issue about it.
• MM: is the context - some people want depth writes defaulting to different values?
• KN: more that we just started talking about it.
  • Filed #3798

WebGPU: Forbid null code point '\0' (U+0000) within strings #3576

• GT: wasn't here for earlier discussions, apologies if duplicate - there's a webgpu.h. Going to break everybody so that we print an error if you pass in \0. It's wrong almost all the time. Why cause all that pain to reject something?
• KG: we have to handle this in our impls, so error anyway. Just have to be cognizant of when they pass \0. In web API, we want to reject the string. At Dawn level…
• ZJ: difference between them - 1) thrown error 2) validation error. 1) can be implemented in browser / JS level. We prefer the first for now (as this is my initial proposal)?
• ZJ: not modifying the C API? And, for these four cases, we can do validation in browser level, and throw a JS error (TypeError?) if the input's invalid?
• GT: there's still a way to return a WebGPU-like error. Also, the idea of throwing exception for those cases. Either's fine.
• Discussed the various options in depth. Also async errors vs. throwing. Happy with either.
• MM: proposal seems to point out that the null codepoint's handled within (...).
• ZJ: we can not pass in shaders with \0 in C API (since we assume that C API always use null-terminated string). Can't do it in the browser / web IDL - type may allow us to do it. We can do the validation at the JS level when creating the shader module.
• KG: leave this as errata for the C API you use?
• MM: imagine shader source is valid. Create me a pipeline, entry point name has \0 in it. Today, that entry point name is valid to have a \0 character in it.
• Looking in shader source - no such function exists. Currently well defined.
• Downside - error's reported with ErrorScopes. Complex ordering. In browser, looking at entry point string - it has \0 in it, so….
• For that impl to be spec compliant, can't just early-out.
• GT: that's correct. We can do that already in Chrome. Not sure about other browsers. Not sure how much work it is. Coming from JS side, it'd be trivial. :)
• ZJ: if we follow existing spec - pass string with \0 in it to the backend, or do validation error in the browser - initial proposal I made was - make it a JS TypeError. Don't send to backend at all.
• KN: wish Web IDL had a string type that disallows null.
• KG: since it doesn't, all of our browsers have to manually handle null chars. Like to say - deal with your impl. I don't care much.
• MM: think updating webgpu.h should be negligible. OTOH, if someone told me they want to throw TypeError for strings with \0 - I'd say, OK.
• GT: there are ~15 projects using webgpu.h.
• KG: understand your preference but backward compatibility isn't the criterion.
• KG: quickest - add a step to the spec, scan and validate incoming strings for createShaderModule.
• KG: hard to convince myself to do this.
• KN: think we should have some offline discussion. I'm remembering thoughts from when we last had this discussion.
• KG: let's do that.

Should the API enforce that writable storage buffer bindings don't alias each other? #1842

• KG: think we discussed and resolved this already.
• MM: “Does this validation apply across all bindings in bind groups in the currently-bound pipeline layout at draw/dispatch time. OR does it only apply to bindings that are actually statically used in the shader at draw/dispatch time?”
• MM: I have no strong opinion.
• KN: on the thread I commented: generally don't think we look at metadata of what a shader uses. Shader -> create pipeline, but pipeline declares its interface. Multiple pipelines, same layout, different shaders -> same requirements. More restrictive, but more expected. Also how the currently written algorithm works.
• KN: if we do proposed optimization in bind group creation time - doing this de-pessimization will need extra (runtime) work. At bindgroup creation time, you only know the layout. Need extra work.
• MM: that optimization is something we'd like to do. Analysis needs to not involve the source code of the shader.
• KG: ahead of time optimization is verify that nothing in a bindgroup aliases each other. Shader group that doesn't use one of the referenced ones - we'll find there's no alias after all, and things will render.
• MM: think it turns into an interference graph.
• KN: will already have this somewhat. Want to optimize checks among multiple bindgroups. After checking set overlap, was it actually fine? I think we should base it on the layout.
• KG: also - it's something where we can loosen it later. Layout could allow these things after all. So I think** we should ignore static_use for now**.
• MM / KN: agreed.

It should be possible to unbind a bind group / vertex buffer #3787

• MM: maybe not enough time in total.
• KG: FWIW we're +1 on supporting unbinding.
• MM: in Metal - maybe in other APIs - vertex buffers and bindgroups use the same underlying resources. Today, not a problem because an impl guarantees a max of ~8 VBs and ~4 BGs. 8+4 is less than the # of "slots" Metal has. Use all VBs and BGs? OK.
• MM: this group has resolved to add another limit - # VBs + BGs. Reason - convenient to say - we have a total of 31 of these, you can allocate them however you want.
• MM: started writing a PR. Realized - wait, once you let apps allocate these, it's imp't for them to decrease the count of the # sets that add up to that total. Only way to do this - app has to be able to say, for now, use 30 bind groups - and for next draw call, unbind them to make room for VBs.
• MM: think it's generally useful too.
• KG: that's more compelling than my use case, of reversibility. +1 here.
• KN: we discussed on Monday. Interesting problem. Haven't thought about it more since then. Unbinding for the sake of validation - implies there's validation happening in the encoder. Something we don't have now. Don't have to validate whether you can bind a bind group, set a vertex buffer, etc. Later, when you draw, we check that the set of things is compatible with the currently set pipeline. This introduces add'l validation in setVertexBuffer/setBindGroup, or validation against the pipeline. That's validation that might be free - not sure.
• KG: Let's think more.
• BJ: feels to me like enabling vertex attribs in OpenGL/WebGL. Something someone else might set, and out of an abundance of caution, you want to undo everything they did. One of the first times we introduce that sort of needing to care about the state of a pass. Unfortunate if we start down that path. One of my favorite features is not having to do that kind of state management. If we can make it so I don't care, great. If not - why do we need to unset it in the first place?
• MM: KN, your commnet - is that about the concept of having a limit that's the sum of two others, or about unsetting directly by itself?
• KN: it's about unsetting as a solution to the problem. Not a problem with the limit - validated during pipeline creation anyway. For unset to be useful we'll add validation to the encoder, where you want to stream these to the MTLCommandEncoder - need validation at setVB/setBG time. Don't think it can be folded into pipeline compatibility check.

Agenda for next meeting

• One more meeting before F2F. Non-APAC.