vello_hybrid: Streamline error handling in the WebGL backend#1588
vello_hybrid: Streamline error handling in the WebGL backend#1588LaurenzV wants to merge 1 commit into
Conversation
| AtlasError(#[from] vello_common::multi_atlas::AtlasError), | ||
| // TODO: Consider expanding `RenderError` to replace some `.unwrap` and `.expect`. | ||
| /// A WebGL operation failed during rendering. | ||
| #[cfg(all(target_arch = "wasm32", feature = "webgl"))] |
There was a problem hiding this comment.
Not sure if it's a good idea to feature gate an error variant, open for other suggestions! Maybe WebGL errors should just always be available? Or I guess there could be a generic error type where WebGL and wgpu can insert their own internal error type.
|
Hmm, I think if we want to have something like this we need to think a bit more about what a user is supposed to do in case they encounter an error and document that. I think ideally, once any kind of error is encountered the user is expected to not do anything with the renderer anymore and completely discard it. However, if we switch to this result-based API, users might be inclined to just try reusing the context, which is bad because at that point the whole renderer might be in an inconsistent state (for example, pending bitmap uploads will be completely drained even if not all bitmap glyphs have been uploaded. I think just documenting that "error encountered = don't perform any further operations with the renderer" might be enough, but perhaps this warrants some more discussion. |
| let context_attributes = gl.get_context_attributes().unwrap(); | ||
| let context_attributes = gl | ||
| .get_context_attributes() | ||
| .expect("WebGL context attributes should be available"); |
There was a problem hiding this comment.
Note to self: This can probably fail if the WebGL context has been lost.
We could poison the renderer similar to how a mutex is poisoned, but that doesn't solve the resources problem 🤔 . I'll raise it in the OH tomorrow |
taj-p
left a comment
taj-p
left a comment
There was a problem hiding this comment.
Re-request review once you're ready 🙏
3 participants
Right now, we have a relatively inconsistent approach of error handling, returning errors in some cases but just unwrapping in other cases. This PR attempts to improve the situation by adding more robust error handling to the WebGL backend. Something similar should be done in the wgpu backend, but that is left for a future PR.
In a few cases, it was often not straightforward to decide whether something should become an error or be unwrapped. I've tried to follow the principle of unwrapping in all cases where it should be impossible to fail (i.e.
max_texture_dimensionreturning a non-numeric value), while using errors anywhere else where it could reasonably fail. Alternatively, we could add a finalUnknown(String)error variant where we combine all errors that should in theory never happen, and thus ensure that the backend never crashes, even if something really unexpected happens.