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

Context restoration #7662

Merged
merged 42 commits into from
Sep 14, 2023
Merged

Context restoration #7662

merged 42 commits into from
Sep 14, 2023

Conversation

kazcw
Copy link
Contributor

@kazcw kazcw commented Aug 25, 2023

Pull Request Description

Add support for recovering from GL context loss. When the context is restored, the loading spinner is shown until shaders finish recompiling.

vokoscreenNG-2023-08-25_09-39-11.webm

While the context is missing, the loading spinner is rendered in the 0% state. (This condition will not normally be observed, except momentarily, as the browser should restore the context immediately if it is lost while the page is visible.) When we receive a new context, the spinner switches to the 90% state until restoration completes. Restoration is fast, as we don't need to do much work except recompiling shaders.

Important Notes

  • A new debug hotkey, Ctrl+Alt+Shift+X, causes context loss for testing. Pressing it a second time causes context restoration.
  • Texture is still a CPU-bound texture. It now uses the "immutable" texStorage/texSubImage API, which is a "preferred alternative" to the texImage API because it can be more efficient.
  • The type for texture uniforms is now Uniform<Option<Texture>>. Texture uniforms are decoupled from the context.
  • A new ContextLost error type can be returned by functions that cannot complete if the context is lost.
  • Fix some crashes that could occur when context was lost.
  • Clarify ownership of some rendering-related types: Externalize, and where possible eliminate, Rc/RefCells.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

@MichaelMauderer
Copy link
Contributor

QA: 🟢
Looks good to me!

@farmaazon farmaazon added the CI: Ready to merge This PR is eligible for automatic merge label Sep 11, 2023
@hubertp
Copy link
Contributor

hubertp commented Sep 11, 2023

Tried this PR after waking up from suspend but it appears that IDE still has a problem with restoring the context:
Screenshot from 2023-09-11 17-54-15

@kazcw
Copy link
Contributor Author

kazcw commented Sep 11, 2023

Tried this PR after waking up from suspend but it appears that IDE still has a problem with restoring the context: Screenshot from 2023-09-11 17-54-15

That log is quite strange. @hubertp, you pressed Ctrl+Alt+Shift+X once after the machine awoke from suspend, right? The result looks unlike anything I've seen on the machines I've tested.

Would you mind testing synthetic context loss and restoration by pressing Ctrl+Alt+Shift+X twice, without any suspend? I'd like to compare the logs.

@hubertp
Copy link
Contributor

hubertp commented Sep 12, 2023

That log is quite strange. @hubertp, you pressed Ctrl+Alt+Shift+X once after the machine awoke from suspend, right? The result looks unlike anything I've seen on the machines I've tested.

No, I didn't press anything. I just suspended and woke it up.

@kazcw Same manifestation without suspend:
Kazam_screencast_00018.webm

@kazcw kazcw removed the CI: Ready to merge This PR is eligible for automatic merge label Sep 12, 2023
@kazcw
Copy link
Contributor Author

kazcw commented Sep 14, 2023

That log is quite strange. @hubertp, you pressed Ctrl+Alt+Shift+X once after the machine awoke from suspend, right? The result looks unlike anything I've seen on the machines I've tested.

No, I didn't press anything. I just suspended and woke it up.

@kazcw Same manifestation without suspend: Kazam_screencast_00018.webm

Ah, ok. The error (loseContext: context loss) indicates that the context loss error occurred during loseContext, which is the function that the new hotkey calls--however, that is a coincidence: I think the browser is attributing the context loss error to loseContext (despite that function not actually being called) because it doesn't keep track of what function actually failed in the case of context loss.


I've tested this PR on several devices in order to try to reproduce this issue, and to check for any other OS/driver-specific problems. My findings:

  • On devices tested (Windows 11, Windows 10, OS X; all tested in Electron), suspend (as triggered manually or by closing the lid) does not cause WebGL context loss. I have even tried explicitly triggering S4 hibernation via the command line, and have not been able to produce "natural" context loss on any test machine--although I expect entering hibernation, removing/running out the battery, and waiting until the RAM contents are lost would do it.
  • With this PR, all the devices tested recover from context loss successfully: I have tested both synthetic context loss triggered by loseContext, and "real" context loss triggered by killing Electron's GPU process (the latter on Windows and Linux).

As pertains to this PR, there are no observed cases of receiving the restoreContext event and not successfully restoring the context, so I think this is RTM.

@hubertp, I'm not sure what to make of the issue you've observed. It looks like on your device the browser isn't giving us a restoreContext event; that seems out of our hands and possibly a browser bug. In any case, from an implementation perspective it is a separate issue from this one--to resolve it we need to either fix whatever is preventing the browser from restoring the context(?), or deal with not receiving the event, either of which is orthogonal to handling the event when it arrives.

@kazcw kazcw added the CI: Ready to merge This PR is eligible for automatic merge label Sep 14, 2023
@vitvakatu
Copy link
Contributor

I believe we only need it to work in the Electron app. The fact that it does or does not work in any other browser does not matter much.

@farmaazon
Copy link
Contributor

This is a big change, and it should not make any regressions (before it, we haven't handled context losses anyway), so I propose to merge it ASAP, and make another issue about losing context on some machines.

@mergify mergify bot merged commit b9ec6d4 into develop Sep 14, 2023
24 checks passed
@mergify mergify bot deleted the wip/kw/restore-context branch September 14, 2023 14:40
jdunkerley pushed a commit that referenced this pull request Sep 15, 2023
Add support for recovering from GL context loss. When the context is restored, the loading spinner is shown until shaders finish recompiling.

[vokoscreenNG-2023-08-25_09-39-11.webm](https://github.com/enso-org/enso/assets/1047859/cfa90ec5-72a1-41e6-bafa-177fa5e85fb2)

*While the context is missing, the loading spinner is rendered in the 0% state. (This condition will not normally be observed, except momentarily, as the browser should restore the context immediately if it is lost while the page is visible.) When we receive a new context, the spinner switches to the 90% state until restoration completes. Restoration is fast, as we don't need to do much work except recompiling shaders.*

# Important Notes
- A new debug hotkey, Ctrl+Alt+Shift+X, causes context loss for testing. Pressing it a second time causes context restoration.
- `Texture` is still a CPU-bound texture. It now uses the "immutable" `texStorage/texSubImage` API, which is a ["preferred alternative"](https://registry.khronos.org/webgl/specs/latest/2.0/#3.7.6) to the `texImage` API because it can be more efficient.
- The type for texture uniforms is now `Uniform<Option<Texture>>`. Texture uniforms are decoupled from the context.
- A new `ContextLost` error type can be returned by functions that cannot complete if the context is lost.
- Fix some crashes that could occur when context was lost.
- Clarify ownership of some rendering-related types: Externalize, and where possible eliminate, `Rc/RefCell`s.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IDE does not recreate WebGL context after waking up from hibernation
5 participants