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

ExternalTexture in WASM #232

Open
1 task
kainino0x opened this issue Oct 12, 2023 · 8 comments
Open
1 task

ExternalTexture in WASM #232

kainino0x opened this issue Oct 12, 2023 · 8 comments
Labels
non-breaking Does not require a breaking change (that would block V1.0) wasm Issues with WebAssembly targets

Comments

@kainino0x
Copy link
Collaborator

kainino0x commented Oct 12, 2023

WASM-specific part of #188.

We need a way to expose the JS GPUExternalTexture API.

I think most likely this will share code with its native equivalent #192 (cc @Kangz): use the same WGPUExternalTexture type, same bind group/bind group layout parts, but different WGPUExternalTextureDescriptors. If so it'll be easiest to do this one first, and the native one later.

I think this will all be via chained structs making it non-breaking / post-v1, but it's also needed for completeness of the API in WASM, so I'm going to not add the non-breaking label. Also just in case any core changes are needed.

@kainino0x kainino0x added the wasm Issues with WebAssembly targets label Oct 12, 2023
@sasmaster
Copy link

Hi. Any delivery time estimation for this feature? Need it badly to communicate with WebCodecs's VideoFrame from wasm,

@kainino0x
Copy link
Collaborator Author

Unfortunately I don't have an estimate right now. If you are interested in proposing something I would be happy to take an experimental API in Emscripten. Alternatively, you can use JavaScript / EM_ASM to do what you need in JS. Take a look at JsValStore to get objects between JS and WASM: https://github.com/emscripten-core/emscripten/blob/main/test/webgpu_jsvalstore.cpp

@kainino0x kainino0x mentioned this issue Mar 8, 2024
@kainino0x kainino0x added the !discuss Needs discussion (at meeting or online) label Apr 11, 2024
@kainino0x
Copy link
Collaborator Author

April 25th meeting:

  • What bits are common to wasm and native?

  • How to construct a WGPUExternalTexture in wasm?

  • KN:
    • proposal: no standard entrypoint (in 1.0), each runtime needs its own, e.g. emscripten-specific entry point using Emval or externref?
    • alt: add a wasm-specific descriptor containing __externref_t? is toolchain support for externref sufficiently baked to do this? (Emscripten’s support is a rickety but works; upstream LLVM support seems solid except it’s easy to write invalid code that causes ICEs)
  • CF: No way to do it in pure Emscripten land?
  • KN: If you turn on the right flags, you can get access to extern ref. it’s a builtin
    There’s macros you can use to put JS code in the middle of your C code. If you do this, you can make a block return a JS object, and have it come into C as an extern ref.
  • CF: That’s the only way to do it?
  • CF: The way wasm-bindgen works is it has a JS table of objects, and when it passes to wams it indexes it.
  • KN: Difference is that because extern refs are actual refs to JS objects, they can’t be stored. They can only exist on the stack.
  • CF: Emscripten doesn’t provide table for you?
  • KN: Does not have a generic put-whatever-you-want table; taking it out of the table would return an extern ref, which doesn’t really exist in C.
  • KN: Don’t think we should standardize this part, because it’s entirely specific to the runtime you’re building with (Emscripten / wasm-bindgen)
  • CF: If we do anything in the spec, realistically Emscripten is only going to implement this. We’re not going to do it because we don’t have a C frontend that runs on the web. So I feel like we should have the external texture interface be a generic thing with a pNext pointer. Emscripten can provide whatever they need. And Dawn/wgpu can provide their own native stuff. Let it all require implement-specific details. For WASM that might be forever.
  • KN: That works. So we would put the stuff there with probably no common bits at all? Just an external texture object that has nothing on it, and an entry in the bind group layout/entry.
  • KN: Will think about it further. Thinking:
    • Add WGPUExternalTexture, bind group layout entry, bind group entry; do NOT add any way to create it, or a descriptor. Document that you must use extensions to create it.
    • Avoids having a create function with an empty extensible descriptor that doesn’t do anything.

@kainino0x
Copy link
Collaborator Author

kainino0x commented Apr 25, 2024

Here is what that would look like:

+/**
+ * There is no standard way to create a WGPUExternalTexture.
+ * Use an extension method provided by the implementation you are targeting.
+ */
+typedef struct WGPUExternalTextureImpl* WGPUExternalTexture WGPU_OBJECT_ATTRIBUTE;

+typedef struct WGPUExternalTextureBindingLayout {
+    WGPUChainedStruct const * nextInChain;
+} WGPUExternalTextureBindingLayout WGPU_STRUCTURE_ATTRIBUTE;

 typedef struct WGPUBindGroupLayoutEntry {
     WGPUChainedStruct const * nextInChain;
     uint32_t binding;
     WGPUShaderStageFlags visibility;
     WGPUBufferBindingLayout buffer;
     WGPUSamplerBindingLayout sampler;
     WGPUTextureBindingLayout texture;
     WGPUStorageTextureBindingLayout storageTexture;
+    // TODO: Open question of how you can tell that this one is selected.
+    // Make it a pointer? Add a boolean to it?
+    WGPUExternalTextureBindingLayout externalTexture;
 } WGPUBindGroupLayoutEntry WGPU_STRUCTURE_ATTRIBUTE;

 typedef struct WGPUBindGroupEntry {
     WGPUChainedStruct const * nextInChain;
     uint32_t binding;
     WGPU_NULLABLE WGPUBuffer buffer;
     uint64_t offset;
     uint64_t size;
     WGPU_NULLABLE WGPUSampler sampler;
     WGPU_NULLABLE WGPUTextureView textureView;
+    WGPU_NULLABLE WGPUExternalTexture externalTexture;
 } WGPUBindGroupEntry WGPU_STRUCTURE_ATTRIBUTE;

@Kangz
Copy link
Collaborator

Kangz commented Apr 26, 2024

IMHO the external texture is niche enough that we can use an extension struct on the binding layout. If it is present, then the entry is for an external texture.

@kainino0x
Copy link
Collaborator Author

That's true, it would solve that open question (which we hadn't realized yet during the meeting).

Would this be the first time the presence of an extension struct does something on its own, with all of its contents set to the defaults? I can imagine that some bindings might want to do things like, allocate all of the known extension structs and wire up the full chain, regardless of whether they're used...? (Though this is kind of inefficient. Also it would not actually work well though, because I think we said we are generating an error for any chained struct associated with a feature that's not enabled? Or was that only for out-structs?)

@austinEng
Copy link
Collaborator

I think we said we are generating an error for any chained struct associated with a feature that's not enabled? Or was that only for out-structs?

All structs I believe.

We could also pass WGPUExternalTextureBindingLayout by pointer? It's effectively the same as the chained struct, but looks more like an optional value. I do think it's slightly odd to use the chained struct for something that is not actually an extension/feature. If the concept of WGPUExternalTexture exists in the core API (even if there is no way to actually create it in core), it would be better IMO if the layout could be passed without an extension struct.

Alternatively, if we think that there is no actual way to specify the layout without extension chains, we could also make it so the only thing that exists is:
typedef struct WGPUExternalTextureImpl* WGPUExternalTexture WGPU_OBJECT_ATTRIBUTE
and the binding and layout are all extensions as well

@kainino0x
Copy link
Collaborator Author

kainino0x commented May 15, 2024

May 9 meeting:

  • KN: (explanation of progress since last time)
  • CF: Could it be a top-level boolean?
  • CF: Sounds like a tagged union. Could we do that? (tag + union)
  • KN: Think we talked about this, but not sure what we said. Would be kinda scary/unsafe. Also wouldn’t be able to add new things to the union…
    • CF: Would be able to if it’s smaller than the other things.
    • KN: Could guarantee the union is at least 8 bytes, then you can always add more pointers to the union.
  • Discussed Oct 26, 2023
    • Problem was JS-on-C needs to be able to represent more than one member of the union at once (it’s invalid).
    • CF: OK, let’s just do a boolean?
  • AE: … can make it all extension, no core API.
  • KN: > the presence of an extension struct does something on its own
  • discussion: … this is fine.
  • Make the whole thing an extension, [it's OK if we have] no members in the chained struct for bind group layout entry.

@kainino0x kainino0x added has resolution Issue is resolved, just needs to be done and removed !discuss Needs discussion (at meeting or online) labels May 15, 2024
@kainino0x kainino0x added non-breaking Does not require a breaking change (that would block V1.0) and removed has resolution Issue is resolved, just needs to be done labels Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
non-breaking Does not require a breaking change (that would block V1.0) wasm Issues with WebAssembly targets
Projects
None yet
Development

No branches or pull requests

4 participants