Skip to content
This repository has been archived by the owner on Jul 10, 2023. It is now read-only.

Add glwindow implementation, that renders to a GlWindow #66

Merged
merged 2 commits into from
Mar 5, 2019

Conversation

asajeffrey
Copy link
Member

Add glwindow implementation, that renders to a GlWindow, mainly for testing, but also this forms a basis for VRDisplays that have code that needs to run in the main thread.

Fixes #52 and #65.

@asajeffrey
Copy link
Member Author

The matching servo branch is at https://github.com/asajeffrey/servo/tree/webvr-glwindow

@asajeffrey
Copy link
Member Author

cc @paulrouget @Manishearth and @MortimerGoro

@paulrouget
Copy link
Contributor

This is great! I'll review this soon.

@paulrouget
Copy link
Contributor

This looks good to me. r=me for most of the changes.

For the blit part, I'd like someone more experienced with GL to look at it.

@MortimerGoro can you look at the StopFrame part?

@asajeffrey
Copy link
Member Author

It would be good to get someone used to GL to have a look at this.

let num_bytes = (width as usize) * (height as usize) * 4;
let mut buffer = self.pool.remove().unwrap_or_else(Vec::new);
buffer.resize(num_bytes, 0);
gl.read_pixels_into_buffer(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we share the data in a more performant way?

Copy link
Member Author

Choose a reason for hiding this comment

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

We'd need to have GL context sharing, which got added to glutin after this PR was submitted (rust-windowing/glutin#899 (comment)). Context sharing is still not implemented on macOS. This implementation makes all platforms use a slow path, to get other platforms onto the fast path, we'd have to recreate the interface that webrender has for images that are either implemented using shared textures or using readback. I'm not sure how to do that without making rust-webvr depend on webrender_api.

Copy link
Member Author

Choose a reason for hiding this comment

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

The spec issue for this is immersive-web/webxr#528. The XR spec pretty much requires textures to go via main memory in the case that the browser doesn't share GL textures with the VR display.

@asajeffrey
Copy link
Member Author

@MortimerGoro Are you okay landing this and filing an issue about supporting texture sharing?

Another issue is that running this on Linux the frame rate is half what you'd expect, my suspicion is that on Linux vsync is always enabled, so both swap_buffers (the VR window and the main browser window) are blocking. Sigh.

@MortimerGoro
Copy link
Contributor

@asajeffrey yes, we can work on the texture sharing as a follow-up

@asajeffrey
Copy link
Member Author

@bors-servo r=paulrouget

@bors-servo
Copy link
Contributor

📌 Commit b61b1e5 has been approved by paulrouget

@bors-servo
Copy link
Contributor

⌛ Testing commit b61b1e5 with merge 19fb68b...

bors-servo pushed a commit that referenced this pull request Mar 5, 2019
 Add glwindow implementation, that renders to a GlWindow

Add glwindow implementation, that renders to a GlWindow, mainly for testing, but also this forms a basis for VRDisplays that have code that needs to run in the main thread.

Fixes #52 and #65.
@bors-servo
Copy link
Contributor

☀️ Test successful - checks-travis
Approved by: paulrouget
Pushing 19fb68b to master...

@bors-servo bors-servo merged commit b61b1e5 into servo:master Mar 5, 2019
bors-servo pushed a commit to servo/servo that referenced this pull request Mar 5, 2019
Use a test VRDisplay that renders to a GL window

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

Add a `dom.webvr.test` pref that registers a new VR display, which registers to a GL window.

The matching webvr PR is servo/rust-webvr#66.

---
<!-- 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 fix #22795
- [X] These changes do not require tests because we need a followup PR to support reftests which use the VR display

<!-- 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. -->

<!-- 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/22953)
<!-- Reviewable:end -->
bors-servo pushed a commit to servo/servo that referenced this pull request Mar 5, 2019
Use a test VRDisplay that renders to a GL window

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

Add a `dom.webvr.test` pref that registers a new VR display, which registers to a GL window.

The matching webvr PR is servo/rust-webvr#66.

---
<!-- 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 fix #22795
- [X] These changes do not require tests because we need a followup PR to support reftests which use the VR display

<!-- 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. -->

<!-- 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/22953)
<!-- Reviewable:end -->
bors-servo pushed a commit to servo/servo that referenced this pull request Mar 5, 2019
Use a test VRDisplay that renders to a GL window

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

Add a `dom.webvr.test` pref that registers a new VR display, which registers to a GL window.

The matching webvr PR is servo/rust-webvr#66.

---
<!-- 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 fix #22795
- [X] These changes do not require tests because we need a followup PR to support reftests which use the VR display

<!-- 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. -->

<!-- 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/22953)
<!-- Reviewable:end -->
bors-servo pushed a commit to servo/servo that referenced this pull request Mar 5, 2019
Use a test VRDisplay that renders to a GL window

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

Add a `dom.webvr.test` pref that registers a new VR display, which registers to a GL window.

The matching webvr PR is servo/rust-webvr#66.

---
<!-- 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 fix #22795
- [X] These changes do not require tests because we need a followup PR to support reftests which use the VR display

<!-- 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. -->

<!-- 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/22953)
<!-- Reviewable:end -->
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants