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.

Sign up for GitHub

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

Jump to bottom

Support for webxr layer management #26927

Merged
merged 1 commit into from
Jun 28, 2020
Merged

Support for webxr layer management #26927

merged 1 commit into from
Jun 28, 2020

Conversation

asajeffrey
Copy link
Member

This adds layer management to servo's webxr implementation, and removes all the special-cased code for openxr surface creation and opaque framebuffers.

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes do not require tests because it's all back-end plumbing

@highfive
Copy link

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/dom/xrrenderstate.rs, components/script/dom/webglframebuffer.rs, components/script/dom/document.rs, components/script/dom/xrwebglsubimage.rs, components/script/dom/xrwebglbinding.rs and 9 more

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jun 15, 2020
@highfive
Copy link

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify script code, but no tests are modified. Please consider adding a test!

@asajeffrey
Copy link
Member Author

Note: this is still a draft because it depends on servo/webxr#176

@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #26872) made this pull request unmergeable. Please resolve the merge conflicts.

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Jun 15, 2020
@nox
Copy link
Contributor

nox commented Jun 25, 2020

r? @Manishearth

@highfive highfive assigned Manishearth and unassigned nox Jun 25, 2020
Manishearth
Manishearth approved these changes Jun 26, 2020
View reviewed changes
Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

Script side seems okay, i'm not so clear on the webgl side though

@asajeffrey asajeffrey marked this pull request as ready for review June 27, 2020 14:47
@asajeffrey
Copy link
Member Author

@jdm ok'd landing this at https://chat.mozilla.org/#/room/#servo:mozilla.org/$hp8mucO869VdIFPbYkZQDhwkcYkqBXbYKhwktno1XUw

@bors-servo r=Manishearth

@bors-servo
Copy link
Contributor

📌 Commit 947b53a has been approved by Manishearth

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors. labels Jun 27, 2020
@bors-servo
Copy link
Contributor

⌛ Testing commit 947b53a with merge 012630b...

bors-servo added a commit that referenced this pull request Jun 27, 2020
@bors-servo
Auto merge of #26927 - asajeffrey:webxr-layer-management, r=Manishearth
012630b 
Support for webxr layer management

<!-- Please describe your changes on the following line: -->

This adds layer management to servo's webxr implementation, and removes all the special-cased code for openxr surface creation and opaque framebuffers.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes do not require tests because it's all back-end plumbing

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

💔 Test failed - status-taskcluster

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Jun 27, 2020
@asajeffrey
Copy link
Member Author

Hmm, so the windows failure is #26895 but the macOS failures look like genuine errors with WebGL in WebXR, e.g. https://community-tc.services.mozilla.com/tasks/bIxuL7ubTYS69siWmOpkmQ/runs/0/logs/https%3A%2F%2Fcommunity-tc.services.mozilla.com%2Fapi%2Fqueue%2Fv1%2Ftask%2FbIxuL7ubTYS69siWmOpkmQ%2Fruns%2F0%2Fartifacts%2Fpublic%2Ffiltered-wpt-errorsummary.log is

  ▶ CRASH [expected ERROR] /webxr/gamepads-module/xrInputSource_gamepad_input_registered.https.html
  │ 
  │ _RegisterApplication(), FAILED TO establish the default connection to the WindowServer, _CGSDefaultConnection() is NULL.
  │ [2020-06-27T16:48:41Z ERROR canvas::webgl_thread] Last GL operation failed: BindTexture(3553, Some(WebGLTextureId(3)))
  │ Unexpected WebGL error: 0x502 (1282) [BindTexture(3553, Some(WebGLTextureId(3)))] (thread WebGL thread, at components/canvas/webgl_thread.rs:2229)
  │    0: <servo::backtrace::Print as core::fmt::Debug>::fmt
  │    1: core::fmt::write
  │    2: std::io::Write::write_fmt
  │    3: servo::backtrace::print
  │    4: servo::main::{{closure}}
  │    5: std::panicking::rust_panic_with_hook
  │    6: _rust_begin_unwind
  │    7: std::panicking::begin_panic_fmt
  │    8: canvas::webgl_thread::WebGLImpl::apply
  │    9: canvas::webgl_thread::WebGLThread::handle_msg
  │   10: canvas::webgl_thread::WebGLThread::process
  │   11: std::sys_common::backtrace::__rust_begin_short_backtrace
  │   12: core::ops::function::FnOnce::call_once{{vtable.shim}}
  │   13: std::sys::unix::thread::Thread::new::thread_start
  │   14: __pthread_body
  │   15: __pthread_start

@asajeffrey
Copy link
Member Author

Running that test 100 times on linux it doesn't fail, so it is a macOS-specific issue. Sigh.

@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Jun 27, 2020
@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Jun 27, 2020
@asajeffrey
Copy link
Member Author

Hmm, another macOS WebGL error...

 ▶ CRASH [expected OK] /webxr/xrWebGLLayer_framebuffer_draw.https.html
  │ 
  │ _RegisterApplication(), FAILED TO establish the default connection to the WindowServer, _CGSDefaultConnection() is NULL.
  │ [2020-06-27T21:42:09Z ERROR canvas::webgl_thread] Last GL operation failed: Clear(16384)
  │ Unexpected WebGL error: 0x506 (1286) [Clear(16384)] (thread WebGL thread, at components/canvas/webgl_thread.rs:2229)
  │    0: <servo::backtrace::Print as core::fmt::Debug>::fmt
  │    1: core::fmt::write
  │    2: std::io::Write::write_fmt
  │    3: servo::backtrace::print
  │    4: servo::main::{{closure}}
  │    5: std::panicking::rust_panic_with_hook
  │    6: _rust_begin_unwind
  │    7: std::panicking::begin_panic_fmt
  │    8: canvas::webgl_thread::WebGLImpl::apply
  │    9: canvas::webgl_thread::WebGLThread::handle_msg
  │   10: canvas::webgl_thread::WebGLThread::process
  │   11: std::sys_common::backtrace::__rust_begin_short_backtrace
  │   12: core::ops::function::FnOnce::call_once{{vtable.shim}}
  │   13: std::sys::unix::thread::Thread::new::thread_start
  │   14: __pthread_body
  │   15: __pthread_start

@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Jun 28, 2020
@asajeffrey
Copy link
Member Author

So that one was caused by WebGL not supporting TEXTURE_RECTANGLE so we need to do bind the texture to the opaque framebuffer without error checking.

@bors-servo r=Manishearth

@bors-servo
Copy link
Contributor

📌 Commit 904f94d has been approved by Manishearth

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jun 28, 2020
@bors-servo
Copy link
Contributor

⌛ Testing commit 904f94d with merge 9d52d7a...

bors-servo added a commit that referenced this pull request Jun 28, 2020
@bors-servo
Auto merge of #26927 - asajeffrey:webxr-layer-management, r=Manishearth
9d52d7a 
Support for webxr layer management

<!-- Please describe your changes on the following line: -->

This adds layer management to servo's webxr implementation, and removes all the special-cased code for openxr surface creation and opaque framebuffers.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes do not require tests because it's all back-end plumbing

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

💔 Test failed - status-taskcluster

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Jun 28, 2020
@jdm
Copy link
Member

jdm commented Jun 28, 2020

Looks like it's just missing a mach fmt now; the reftests failures are just #23290.

@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Jun 28, 2020
@asajeffrey
Copy link
Member Author

Ran mach fmt. @bors-servo r=Manishearth

@bors-servo
Copy link
Contributor

📌 Commit 349619e has been approved by Manishearth

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jun 28, 2020
@bors-servo
Copy link
Contributor

⌛ Testing commit 349619e with merge 345238a...

@bors-servo
Copy link
Contributor

☀️ Test successful - status-taskcluster
Approved by: Manishearth
Pushing 345238a to master...

@bors-servo bors-servo merged commit 345238a into servo:master Jun 28, 2020
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jun 28, 2020
This was referenced Jul 17, 2020
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Manishearth Manishearth Manishearth approved these changes

Assignees

@Manishearth Manishearth

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@asajeffrey @highfive @bors-servo @nox @jdm @Manishearth @thtrummer