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

Synchronize WebGL layer creation with underlying GL APIs #24408

Merged
merged 1 commit into from Oct 17, 2019
Merged

Conversation

@jdm
Copy link
Member

jdm commented Oct 9, 2019

I believe the underlying cause of #24373 is that the XR callbacks can end up executing before the XR WebGL layer setup has occurred. This was observed in the following symptom:

  • a DOM exception would be thrown from inside the renderVR function, which is only called from XR's requestAnimationFrame callback
  • following this, the webgl thread would panic with a GL error when executing the GL operations invoked from XRWebGLLayer::Constructor

This indicates that somehow the XR rAF callback that raised an exception would have been operating on GL state that was not actually the state intended.

This change is Reviewable

@highfive
Copy link

highfive commented Oct 9, 2019

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/xrwebgllayer.rs
  • @KiChjang: components/script/dom/xrwebgllayer.rs
@highfive
Copy link

highfive commented Oct 9, 2019

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@jdm jdm changed the title xr: Synchronize WebGL layer creation with underlying GL APIs. Synchronize WebGL layer creation with underlying GL APIs Oct 9, 2019
@jdm
Copy link
Member Author

jdm commented Oct 9, 2019

Given my previous explanation of what's going on, this change feels like a workaround for a deeper problem. The rAF isn't invoked until after the XRWebGLLayer constructor returns, so any webgl commands should still be ordered correctly when the first rAF callback occurs.

@jdm jdm changed the title Synchronize WebGL layer creation with underlying GL APIs [WIP] Synchronize WebGL layer creation with underlying GL APIs Oct 9, 2019
@jdm
Copy link
Member Author

jdm commented Oct 9, 2019

@Manishearth see if this makes the gl errors go away for you?

@jdm
Copy link
Member Author

jdm commented Oct 9, 2019

Oh wait, there's no ordering problem - the DOM exception and the GL errors happen in different threads. Any GL operations invoked from the script thread would be invoked after the XRWebGLLayer's operations are complete.

@Manishearth
Copy link
Member

Manishearth commented Oct 9, 2019

Confirmed that webxr-broken.html (which I've updated to use .inputSources) does not crash with this patch, instead throwing a stream of errors every time pose tracking is lost (which it is supposed to)

@Manishearth
Copy link
Member

Manishearth commented Oct 10, 2019

r=me, idk if the WIP is "still need to test" or "unsure if this is the right approach"

@Manishearth
Copy link
Member

Manishearth commented Oct 15, 2019

Is there something blocking merging this?

@jdm
Copy link
Member Author

jdm commented Oct 15, 2019

The WIP was "unsure if this is the right approach". I'm still not convinced that it's not papering over some other issue.

@jdm
Copy link
Member Author

jdm commented Oct 17, 2019

Based on my analysis in #24373 I think this is a sensible way to deal with this issue.

@jdm jdm changed the title [WIP] Synchronize WebGL layer creation with underlying GL APIs Synchronize WebGL layer creation with underlying GL APIs Oct 17, 2019
@jdm
Copy link
Member Author

jdm commented Oct 17, 2019

@bors-servo r=Manishearth

@bors-servo
Copy link
Contributor

bors-servo commented Oct 17, 2019

📌 Commit 6ce5c25 has been approved by Manishearth

@bors-servo
Copy link
Contributor

bors-servo commented Oct 17, 2019

Testing commit 6ce5c25 with merge a918202...

bors-servo added a commit that referenced this pull request Oct 17, 2019
Synchronize WebGL layer creation with underlying GL APIs

I believe the underlying cause of #24373 is that the XR callbacks can end up executing before the XR WebGL layer setup has occurred. This was observed in the following symptom:
* a DOM exception would be thrown from inside the renderVR function, which is only called from XR's requestAnimationFrame callback
* following this, the webgl thread would panic with a GL error when executing the GL operations invoked from XRWebGLLayer::Constructor

This indicates that _somehow_ the XR rAF callback that raised an exception would have been operating on GL state that was not actually the state intended.

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24408)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 17, 2019

💔 Test failed - status-taskcluster

@CYBAI
Copy link
Collaborator

CYBAI commented Oct 17, 2019

@bors-servo
Copy link
Contributor

bors-servo commented Oct 17, 2019

💣 Failed to start rebuilding: Unknown error

@bors-servo
Copy link
Contributor

bors-servo commented Oct 17, 2019

Testing commit 6ce5c25 with merge 9b59a11...

bors-servo added a commit that referenced this pull request Oct 17, 2019
Synchronize WebGL layer creation with underlying GL APIs

I believe the underlying cause of #24373 is that the XR callbacks can end up executing before the XR WebGL layer setup has occurred. This was observed in the following symptom:
* a DOM exception would be thrown from inside the renderVR function, which is only called from XR's requestAnimationFrame callback
* following this, the webgl thread would panic with a GL error when executing the GL operations invoked from XRWebGLLayer::Constructor

This indicates that _somehow_ the XR rAF callback that raised an exception would have been operating on GL state that was not actually the state intended.

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24408)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 17, 2019

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
Approved by: Manishearth
Pushing 9b59a11 to master...

@bors-servo bors-servo merged commit 6ce5c25 into master Oct 17, 2019
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
Taskcluster (pull_request) TaskGroup: success
Details
homu Test successful
Details
@bors-servo bors-servo deleted the jdm-patch-31 branch Oct 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.