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

WebGLObject creation is now infallible, and starts Lost iff context Lost. #3642

Merged
merged 5 commits into from
Jul 29, 2024

Conversation

kdashg
Copy link
Contributor

@kdashg kdashg commented Apr 16, 2024

A general design pattern for WebGL is that it should generally be possible for rare failures (like context lost) to be ignorable when possible, such that code Just Works, and doesn't have to be rigorously robust to unusual failure cases. This is particularly important for context loss, since it is absolutely random, and could happen between any two invocations in JS.

For example, getUniformLocation might fail if a program doesn't have an expected uniform name, or fails to link. (or if context is lost!) By making e.g. uniform1i take WebGLUniformLocation? location and not WebGLUniformLocation location, we can paper over certain (rare-ish?) partial failures, rather than snowballing failures into black screens or runtime exceptions.

It would be helpful to developers if object creation were infallible, since returning null here can cause hard-to-isolate bugs.
Infallible creation means that the following pattern is safe:

const fb = gl.createFramebuffer();
fb.gl = gl;

Object creation can still always fail due to true OOM situations, just as creating other dom and js objects can fail if you run out of memory. (it generates an exception)

Additionally, this makes it easier to add labels robustly in #3637, though there is no strict dependency between these PRs.

This PR does not provide a way to detect whether creation is infallible when running on a particular (version of a) user agent/browser, but I think that's ok if we have group consensus to add this, since there are not many implementations. (and most user code probably assumes this is the case anyways)

@kdashg
Copy link
Contributor Author

kdashg commented Apr 16, 2024

For example, this code that I wrote for Best Practices is not robust against context-loss: https://developer.mozilla.org/en-US/docs/Web/API/WebGL_API/WebGL_best_practices#use_non-blocking_async_data_readback

Copy link
Member

@kenrussell kenrussell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for thinking about this so deeply and putting together this comprehensive change. I agree that apps will likely be more resilient to context losses with this spec update.

Have you had a chance to prototype this change in Firefox? Do the revised tests pass with the new context loss semantics?

I'd appreciate it if we could coordinate landing of this change so that we can minimize the time duration these top-of-tree tests are failing in various implementations, since these tests cover important functionality (context loss and restoration).

sdk/tests/conformance/context/context-lost-restored.html Outdated Show resolved Hide resolved
sdk/tests/conformance/context/context-lost-restored.html Outdated Show resolved Hide resolved
sdk/tests/conformance/context/context-lost.html Outdated Show resolved Hide resolved
sdk/tests/conformance/context/context-lost.html Outdated Show resolved Hide resolved
@kdashg
Copy link
Contributor Author

kdashg commented Jul 2, 2024

Seems to be working properly in a prototype of Firefox:
https://treeherder.mozilla.org/jobs?repo=try&revision=63382bf6daa06fb9bd5a23e9217d3a18ec7bdace

@kdashg
Copy link
Contributor Author

kdashg commented Jul 22, 2024

This landed in Firefox Nightly 130. Bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1892542

@kdashg
Copy link
Contributor Author

kdashg commented Jul 29, 2024

Thanks all!

@kdashg kdashg merged commit 97b78aa into KhronosGroup:main Jul 29, 2024
2 checks passed
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 this pull request may close these issues.

3 participants