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

Determine how to return CompilationInfo #82

Closed
toji opened this issue Mar 31, 2021 · 3 comments · Fixed by #114
Closed

Determine how to return CompilationInfo #82

toji opened this issue Mar 31, 2021 · 3 comments · Fixed by #114

Comments

@toji
Copy link

toji commented Mar 31, 2021

CompilationInfo is a little unique because it's one of the only places in the API that a structure of data is being returned out of the API. AdapterProperties seems to be the only close analog, but because CompilationInfo is queried with a promise in the JS API it would need to be supplied by a callback here.

So a first stab at this would suggest we'd want an API surface like that following?

typedef enum WGPUCompilationMessageType {
    WGPUCompilationMessageType_Undefined = 0x00000000,
    WGPUCompilationMessageType_Error = 0x00000001,
    WGPUCompilationMessageType_Warning = 0x00000002,
    WGPUCompilationMessageType_Info = 0x00000003,
} WGPUCompilationMessageType;

typedef struct WGPUCompilationMessage {
    WGPUChainedStruct const * nextInChain; // Are these extensible?
    char const * message;
    WGPUCompilationMessageType type;
    uint64_t lineNum;
    uint64_t linePos;
} WGPUCompilationMessage;

typedef struct WGPUCompilationInfo {
    WGPUChainedStruct const * nextInChain; // Are these extensible?
    WGPUCompilationMessage * messages;
    uint32_t messageCount;
} WGPUCompilationInfo ;

typedef void (*WGPUCompilationInfoCallback )(WGPUCompilationInfo* compilationInfo, void * userdata);

WGPU_EXPORT void wgpuShaderModuleGetCompilationInfo(WGPUShaderModule shaderModule, WGPUCompilationInfoCallback callback, void * userdata);

Or, recognizing that WGPUCompilationInfo is just a list of messages, if we weren't worried about extensibility of that particular structure the callback signature could simply be:

typedef void (*WGPUCompilationInfoCallback )(WGPUCompilationMessage* messages, uint32_t messageCount, void * userdata);

I'm also not sure what sort of guarantees the library gives about lifetime of objects like this, if it's expected that it would only be valid for the duration of the callback or if the library would be expected to keep it alive and owned by the shader module after being queried.

@austinEng
Copy link
Collaborator

Overall, I think we should keep the extensibility you have proposed. I could definitely imagine extensions like WEBGL_debug_shaders.getTranslatedShaderSource() that might fit nicely here.

However, I think the way Vulkan usually handles extensibility like this for output structs is that you pass in a chained struct which the Vulkan implementation populates. Maybe one way we could do that is to also pass in WGPUCompilationInfo into wgpuShaderModuleGetCompilationInfo. So, if you chain WGPUCompilationInfoGetTranslatedShaderSource onto that, then the implementation will also populate that struct. In that world we could:

  1. continue passing the same WGPUCompilationInfo * in the callback for consistency with other async functions like createRenderPipelineAsync
  2. OR have a different WGPUCompilationInfoDoneCallback which just tells you when the implementation has finished populating the struct.

P.S. Okay, maybe not WGPUCompilationInfoGetTranslatedShaderSource since we can't always translate at this stage. But maybe we'll have other extensions for certain kinds of reflection, SPIR-V specific compilation info, AST inspection?, etc.

@kainino0x
Copy link
Collaborator

I just noticed that in addition to the output extension chain, there's also an output string here. So unless we do the Vulkan thing consistently (so for strings: call with null to get the string length, then call again with preallocated storage - for every message! awful)...
I think we should have the implementation own the allocations of both the chain and the strings, and choose one of the two lifetimes Brandon suggested (either the duration of the callback, or the remaining lifetime of the WGPUShaderModule).

@kainino0x
Copy link
Collaborator

#96 adds nextInChain to both objects. I think it's very good to have it for both.
It doesn't add GetCompilationInfo yet though.

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 a pull request may close this issue.

3 participants