Skip to content

Minutes 2019 06 24

Jump to bottom
Corentin Wallez edited this page Jan 4, 2022 · 1 revision

GPU Web 2019-06-24

Chair: Dean

Scribe: Ken

Location: Google Meet

TL;DR

  • #317 - Indirect draw calls accepted for now. Apple will come back with information on whether that excludes too many macOS devices. Still an open question on GLES 3.2 devices. AI: Myles to investigate.
  • #313 - merged as is. ccw is the default.
  • #314 - add vertex slot - we’ll stick with sparse arrays and reject the PR
  • #327 is now #336 - delayed until we can have the bigger discussion on what is required and what isn’t/what the default values are. AI: Dzmitry to file a new issue about defaults
  • #337 - AI: Dean to update this PR to have a github pages repository that can be used as webgpu.io/webgpu.dev, including spec and implementation status
  • #339 - Accept the PR. Requires a bit more validation but allows the common path to be fast.
  • Myles to make a proposal for shading language binding to discuss next week

Tentative agenda

  • PR Burndown
  • Agenda for next meeting

Attendance

  • Apple
    • Dean Jackson
    • Justin Fan
    • Myles C. Maxfield
    • Robin Morisset
  • Google
    • Austin Eng
    • Idan Raiter
    • Kai Ninomiya
    • Ken Russell
    • Shrek Shao
    • Ryan Harrisson
  • Intel
    • Yunchao He
  • Microsoft
    • Chas Boyd
    • Rafael Cintron
    • Damyan Pepper
  • Mozilla
    • Dzmitry Malyshau
    • Jeff Gilbert
  • Mehmet Oguz Derin
  • Timo de Kort

Admin

  • DJ: In week 2 of call for spec editors. Will finalize things in week 3. Name your victims now.

PR Burndown

  • Indirect commands #317
    • IR: adding DrawIndirect and DispatchIndirect to spec. That’s pretty much it.
    • DM: need way to tell users that these are not available.
    • KN: why? https://developer.apple.com/metal/Metal-Feature-Set-Tables.pdf
    • DM: on some Metal, indirect commands are not supported. On Vulkan, BaseVertex may not be available in indirect commands.
    • KN: Not available on Metal GPU family 1, Apple 1 and Apple 2. Apple 2 doesn’t have indirect draws.
    • DJ: I’m OK dropping support for “Apple II”. :)
    • KN: Apple 1 is A7, Apple 2 is A8.
    • DJ: might be iPhone 7 or 6.
    • KN: Common 1 is probably the same.
    • DJ: that’s one we just added at WWDC. Not sure what it corresponds to.
    • (more discussion about what the Metal feature tables mean)
    • DJ: we will need some way to detect whether this is supported.
    • KN: A8 = iPhone 6.
    • DJ: we support that right now, don’t drop it in this year’s releases.
    • KN: BaseInstance is not available on all Vulkan.
    • DM: roughly 11% of implementations don’t support this including Qualcomm. On Metal, iOS 9 seems to be required. We’d rather set it as a baseline or find a way to say it’s not supported.
    • DJ: we won’t support iOS versions smaller than what we initially ship on. On Mac we do ship on older OSs though.
    • JF: I think iOS 13 only supports the iPhone 6S and later. Drops support for the A8 and earlier processors.
    • DJ: probably hard to find out what MTLGPUFamilyMac1 GPUs corresponds to.
    • KN: OK. Should we figure out a way to put this in an extension?
    • DM: would be nice to support it unconditionally, if possible.
    • KN: BaseInstance is unfortunate.
    • JG: same as OpenGL. BaseInstance has less coverage.
    • KR: BaseInstance is fairly easy to emulate on OpenGL. Not sure about on Vulkan.
    • DM: it’s easy to support if you do application-level vertex pulling.
    • KN: I like the idea of removing vertex push from the API completely, but not sure if it’s practical.
    • JG: if that’s the way we polyfill it, that’s not the worst.
    • DJ: how do we make progress?
    • DM: can we say: BaseInstance not supported right now, merge this, and later we figure out the situation with Metal devices?
    • JG: Metal devices don’t support this at all. Depends on whether or not Apple thinks it’s OK to ship this. Do you want more time to look at affected devices?
    • DJ: think so. 1) Put it in everywhere, 2) Don’t put it in everywhere, or 3) Have some way to detect at runtime. I’d prefer (1), but for Apple it depends on how many devices we’d be excluding from WebGPU completely.
    • JG: also related to support going back to ES 3.2 implementations. Depends on extension support there.
    • KN: it’s easy enough to say this entry point doesn’t work. Doesn’t have to be an extension in that case.
    • KR: Shrek (SS) just did an investigation for WebGL’s BaseVertex / BaseInstance extensions. Thought indirect calls in ES 3.2 did support this.
    • KN: there’s a field in the indirect draw call struct “RestrictedMustBeZero”.
    • JG: resolved: no BaseInstance for now.
    • KR: actually, “RestrictedMustBeZero”.
    • MM: we have to validate it anyway.
    • KN: some Metal devices don’t support indirect calls at all.
    • DJ: so Idan is responsible for merging from here?
    • MM: think adding this in for now, but for next meeting I’ll take an AI to see how many devices this would exclude. If you don’t hear back from me it’s OK to merge.
  • Set initial frontFace value to “ccw” #313
    • DJ: Francois proposed this?
    • DM: last state of this was to make it optional. Required for triangles, not for non-triangles.
    • JG: happy with that.
    • MM: backface culling not enabled by default?
    • KN: believe it’s not enabled by default.
    • JG: I’m happy to make a PR to make this validation change, if that’ll move this forward.
    • MM: what’s the validation change?
    • JG: spec that FrontFace is optional, and adding validation text saying it generates an error if you try to render triangles and don’t set a winding direction.
    • MM: what’s the point of making it optional if you have to specify it?
    • JG: it can be optional for points. Various people found it unpalatable to set it to ccw explicitly. Also unsuitable to require it for e.g. POINTS, where it’s meaningless.
    • RC: whether you use triangles / lines isn’t in this dictionary right?
    • JG: right. Maybe / Nullable in your validation code. Would know FrontFace was never specified. When specifying draw commands later, would know no FrontFace was set.
    • MM: don’t you need this info when you compile RenderPipelineDescriptor?
    • KN: yes. But we’d say that this render pipeline can only be used with POINTS and LINES.
    • JG: I’m also in favor of just setting it to ccw.
    • MM: My prefs: 1) say ccw, 2) this compromise middle solution, 3) what we have today.
    • JG: isn’t Metal the only one to use clockwise?
    • MM: I’m OK with spec’ing to ccw.
    • DM: it’s not a problem to tell the author what the rules are.
    • DJ: does anyone disagree with merging as is?
    • DM: I disagree. Not strongly though.
    • JG: let’s just merge this.
  • Add slot into VertexBuffer #314
    • YH: when implemented in Dawn, found right now we don’t have the slot. For developers, if we want to spec the slot, have to have some null (?) object. Not straightforward. They have the IDL for buffer slot. If we add a slot then we need to validate that the slot isn’t duplicated with previous one.
    • MM: think the argument that developers have in their minds that there is a buffer slot is not compelling. Think developers think, here are my buffers and they’re laid out in an array.
    • JG: what I remember from this discussion is, there are some ergonomic concerns about using sparse arrays. Workable enough, ergonomic enough. Fan of keeping it that way.
    • MM: having index values that have to correspond between two different calls in the API is confusing. The more complicated the API gets. Naming helps with that, but it’s a place where the type system doesn’t help.
    • JG: I’d prefer to close this pull request unmerged.
    • KN: the type system helping us doesn’t mean anything to the developer.
    • JG: it makes a difference how they think about the API.
    • KN: if they just try things and get an error, it’s the same either way.
    • JG: want to make it as easy from reading the spec to being able to write code. Having slots that have to correspond to other slots is a little weird. In Vulkan there’s a FamilyIndex which has to match up in a couple of places, which is weird.
    • YH: I’m OK with this being closed.
  • Make sampler descriptor optional #327
    • DJ: this actually becomes #336. Make sampler filters required in CreateSampler.
    • DM: is there a reason why CLAMP_TO_EDGE is the default?
    • JG: it’s the default OpenGL would use if they could drop all historical compatibility baggage. It used to be REPEAT. Most validation is likely to pass if it’s CLAMP_TO_EDGE.
    • KN: I think CLAMP_TO_EDGE is a fine default. Most models won’t go out of bounds. If they do, they must have intentionally wanted REPEAT. Unfortunate we can’t just say “black” because we don’t have borders. I’m fine with CLAMP_TO_EDGE.
    • DM: BTW, D3D12 the helper CD3DX12 has wrap mode by default. :)
    • KN: they all make as much sense if you aren’t going out of bounds anyway. Sort of like ccw.
    • JG: if we want to investigate that more I’d like to do it in a different PR.
    • DM: I will write something up.
    • JG: I’ve read this PR and don’t understand why it’s desired.
    • DM: because we never discussed these defaults. Unless proven otherwise, it’s not a default.
    • RC: so if you put in null you’d get all defaults? Have to have an option. Before, everything had a default. Now the PR says, 3 are required. Can’t be optional any more. I don’t understand, is it because the person wants to have an opinion about one of them? Or are these 3 that important?
    • KN: he doesn’t say. This is the only create for which these are optional. (And CreateCommandEncoder.)
    • JG: looks like this happened because we were postponing discussion until group discussed defaults again. Should have that discussion before moving forward with either of these PRs. Not clear why we’d change those to be required (in #336).
    • DM: there was no reason to make the original code have defaults.
    • MM: there was - simplicity for new developers.
    • JG: and having sane defaults. Having to specify every time is pain that’s largely unnecessary. Having NEAREST the default is great because that’s the basic operation anyway.
    • DM: that makes sense. I would agree, but would also like to check the defaults in the native APIs.
    • MM: in Metal they match with these. In D3D, looks like default is anisotropic, which we don’t have. In Vulkan there are no defaults.
    • JG: in OpenGL the default mag filter is LINEAR and the min filter is NEAREST_MIPMAP_LINEAR I think.
    • MM: there are 2 levels in most APis, the lowest level, and helpers on top of those.
    • DP: the lower ones do have defaults, defaulting to NEAREST.
    • JG: it’s very common pain point in OpenGL to get black textures because of the min filter.
    • DM: I think NEAREST is a reasonable default because it’s cheapest for the GPU.
    • JG: ES 3.0 really doesn’t want to filter int formats. You will get black for your textures. Not an error we surface to people; it’s an incomplete texture. Maybe don’t make it an error. Would like our behavior to be, the texture’s blocky.
    • DM: we would still need to detect that integer textures are used with samplers.
    • KN: but at least it’ll render something.
    • JG: OK, we can close this without merging.
    • DJ: so reject this and continue discussions about what the defaults would be?
    • DJ: Dzmitry, you’ll take an AI to file a new PR?
    • DM: yes. I’ll file either new PR with new addressing mode defaults or something else.
  • Mixins and splitting device #315 / #316
    • #315 becomes #329 (below) or #334
    • #334 is a task for integrating Travis CI, which has been done (thanks Francois!)
    • DJ: #316:
      • KN: sorry I haven’t updated this yet. Will rebase.
    • KN: #315 was already done in #334. Not relevant.
    • DJ: #329: expose GPU device and target to worker.
      • Merged this as well.
    • JG: OK, so Kai will rebase #316.
  • Expose "GPUDevice : EventTarget" to worker #329
    • Already closed.
  • Update spec URL #337
    • DJ: on this one, think it’s doing the right thing but I can take the AI to make a more general PR so that gpuweb.github.io gets redirected to the domain Corentin registered.
    • KN: webgpu.io.
    • KN: at this moment, webgpu.io points at the impl status page of the wiki. webgpu.dev points at the spec.
    • DJ: think io and dev should point at the same thing, a landing page which has link to spec and impl status.
    • DJ: webgpu.io could proxy for the other.
    • JG: think you can set a URL in Github pages. We should just do that.
    • DJ: I’ll take the AI to do that.
    • KN: guess we should move impl status there.
    • DJ: yes. Move impl status to a page in that repo. A bit annoying but you can still edit with Github’s editor.
  • DM: “Provide texture dimension in GPUBindGroupLayoutBinding” #339 ?
    • KN: interesting one. Sounds necessary to me.
    • RC: if we need it to make Metal work well, then suppose we have to do it.
    • DM: also need to know if it’s a MS texture or not. View dimension won’t necessarily be.
    • RC: you need to know more than just the type because you have to use argument buffers, and in arg buffers you need the dimension too?
    • DM: arg buffers = 2 steps. 1) Declare what arg encoder would do. Similar to BindGroupLayout. 2) Putting real values in real buffer. We do this in CreateBindGroup. Creating the encoder is where you describe the args. Not just the dimensionality, but also whether it’s multisampled or not.
    • DM: if Myles wants to do investigation we can wait.
    • MM: this is required to put textures in a Metal arg buffer. There are some specific types of textures (writable textures) which can’t be put in arg buffers. It’s just impossible. This means that for some resources you can put in arg buffers and some you can’t. Without this API sampleable textures can’t be put in arg buffers. Given that we can’t put 100% of resources in arg buffers, should webgpu devs have to add more metadata to get them on the fast path? Think in this case yes. Sampleable textures are much more common than writeable textures. If just using sampled textures, drawing to the screen, is slow, that’s a poor API design.
    • MM: upshot is that I support this PR.
    • DM: could you post a link saying why writeable textures are not supported in arg buffers?
    • MM: don’t think there’s public documentation saying why, but there are public docs saying that they aren’t.
    • RC: so all this comes with more validation? If you use texture that doesn’t match the dimensions, you have to create a new BindGroupLayout?
    • MM: we already have validation there. If BindGroup says this is a buffer but you put in a texture, our validation would say that’s illegal. Another check inside our existing validation code.
    • RC: OK.
    • DJ: nobody’s against accepting the PR?
    • DJ: consider it merged.
    • MM: procedural question: this probably requires explanation text outside of a comment in the IDL. Would require a paragraph of English.
    • DM: where would it go?
    • MM: in the spec itself. Like the WebGL spec, it will have to have lots of IDL with lots of spec text in between.
    • RC: I agree with that. For this PR, could we make it: if you want to use a sampleable texture (and we don’t need this texture dimension thing?)
    • MM: we do need it for sampleable textures. Writable ones can’t work with arg buffers.
    • RC: never mind then.

Agenda for next meeting

  • JG: more PR burndown.
  • MM: by 2 days before next meeting I’m going to come up with API proposal - months ago we had a heated discussion about binding models of WebGPU vs. shading language. Came up with conclusion that there would be extra API in JS to describe how to map shading language’s binding model with WebGPU’s
    • JG: looking forward to it.
    • KR: that API was somewhat D3D11 specific, correct?
    • MM: yes. Shader says register s0, u0, etc.
    • KN: or we could introduce syntax in the shaders that sidesteps the problem.
    • MM: we discussed that in the earlier concall. JS API was deemed the better way forward.
    • KN: should this maybe wait until the next shader language discussion? It’s not really blocked on it.
    • JG: it would be nice to have it written out before that discussion at least.
    • DJ: we’ll discuss it next week then.
Clone this wiki locally