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

Use a test VRDisplay that renders to a GL window #22953

Merged
merged 1 commit into from Mar 5, 2019

Conversation

Projects
None yet
6 participants
@asajeffrey
Copy link
Member

asajeffrey commented Feb 28, 2019

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.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #22795
  • These changes do not require tests because we need a followup PR to support reftests which use the VR display

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

highfive commented Feb 28, 2019

Heads up! This PR modifies the following files:

  • @paulrouget: components/compositing/compositor.rs, components/compositing/Cargo.toml, ports/servo/glutin_app/window.rs, components/compositing/windowing.rs, components/compositing/compositor_thread.rs and 2 more
@asajeffrey

This comment has been minimized.

Copy link
Member Author

asajeffrey commented Feb 28, 2019

@highfive highfive assigned paulrouget and unassigned emilio Feb 28, 2019

if self.window.get_animation_state() == windowing::AnimationState::Animating {
self.composite_if_necessary(CompositingReason::Animation);
}

This comment has been minimized.

@paulrouget

paulrouget Mar 1, 2019

Contributor

Maybe skip this change, and call hearbeat in perform_updates? perform_updates is the "heartbeart" of the compositor.

This comment has been minimized.

@asajeffrey

asajeffrey Mar 1, 2019

Author Member

The reason for doing this is that the main GL window is the one with vsync enabled, so the VR heartbeat needs to be called immediately after swapping the main window buffers. Hence having composite_specific_target calling the hearbeats between self.window.present() (which swaps the main window buffers) and self.process_animations(). If we want to prioritize the VR heartbeats over the main page's animations (which I think we do) then the heartbeats need to be in composite_specific_target.

This comment has been minimized.

@paulrouget

paulrouget Mar 4, 2019

Contributor

Right, I agree about doing the work after the vsync-swap-buffer.

But I'm not sure why process_animations is something we should care about? Isn't just a tick sent to the constellation?

This is the current tasks performed by Servo:

  1. WR render
  2. swap buffer (blocking, vsync)
  3. tick animations

We are adding:

  1. VR heartbeat
  2. VR swap buffer (which is part of heartbeat)

You're suggesting:

  1. WR render
  2. Servo swap_buffer (blocking, vsync)
  3. VR heartbeat
  4. VR swap
  5. Servo tick animations

Is that right?

Would it be basically equivalent to:

  1. WR render
  2. Servo Swap_buffer (blocking, vsync)
  3. Servo tick animations
  4. VR heartbeat
  5. VR swap

If so, we can move the call to heartbeat at the end of perform_updates.

But it would be better to move the heartbeat work before the blocking swap_buffer;

  1. VR heartbeat
  2. WR render
  3. Servo Swap_buffer (blocking, vsync)
  4. VR swap
  5. Servo tick animations

But that would require extracting the swap_buffer call from VR heartbeat.

Does it make sense?

This comment has been minimized.

@asajeffrey

asajeffrey Mar 4, 2019

Author Member

Hmm, I see what you mean, process_animations() isn't doing that much (a bunch of hash table lookups and channel sends) so we can probably afford to run it before the heartbeat.

Not sure why extracting the VR swap buffer from the VR heartbeat would help, moving the heartbeat into perform_updates would simplify things though.

@@ -146,8 +147,15 @@ pub trait WindowMethods {
/// will want to avoid blocking on UI events, and just
/// run the event loop at the vsync interval.
fn set_animation_state(&self, _state: AnimationState);
/// Get the animation state
fn get_animation_state(&self) -> AnimationState;

This comment has been minimized.

@paulrouget

paulrouget Mar 1, 2019

Contributor

I don't think this should be needed. The compositor doesn't need to know the animation state. See previous comment.

This comment has been minimized.

@asajeffrey

asajeffrey Mar 4, 2019

Author Member

If we move the heartbeat call, I think we can drop this.

@asajeffrey
Copy link
Member Author

asajeffrey left a comment

I thiiiiiink we need the hearbeats to be called when compositing, to get the vsync right.

if self.window.get_animation_state() == windowing::AnimationState::Animating {
self.composite_if_necessary(CompositingReason::Animation);
}

This comment has been minimized.

@asajeffrey

asajeffrey Mar 1, 2019

Author Member

The reason for doing this is that the main GL window is the one with vsync enabled, so the VR heartbeat needs to be called immediately after swapping the main window buffers. Hence having composite_specific_target calling the hearbeats between self.window.present() (which swaps the main window buffers) and self.process_animations(). If we want to prioritize the VR heartbeats over the main page's animations (which I think we do) then the heartbeats need to be in composite_specific_target.

@asajeffrey

This comment has been minimized.

Copy link
Member Author

asajeffrey commented Mar 4, 2019

@paulrouget OK, I moved the heartbeat calls and got rid of get_animation_state().

@paulrouget

This comment has been minimized.

Copy link
Contributor

paulrouget commented Mar 5, 2019

r=me with Cargo.toml updated.

@asajeffrey asajeffrey force-pushed the asajeffrey:webvr-glwindow branch 2 times, most recently from a6651ac to eec3e8c Mar 5, 2019

@asajeffrey

This comment has been minimized.

Copy link
Member Author

asajeffrey commented Mar 5, 2019

@bors-servo r=paulrouget

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 5, 2019

📌 Commit eec3e8c has been approved by paulrouget

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 5, 2019

⌛️ Testing commit eec3e8c with merge 66de1e5...

bors-servo added a commit that referenced this pull request Mar 5, 2019

Auto merge of #22953 - asajeffrey:webvr-glwindow, r=paulrouget
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

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 5, 2019

💔 Test failed - magicleap

@asajeffrey

This comment has been minimized.

Copy link
Member Author

asajeffrey commented Mar 5, 2019

Oh how ironic, it's the magicleap build that is broken...

error[E0432]: unresolved import `servo::webvr::VRMainThreadHeartbeat`
  --> ports/libsimpleservo/api/src/lib.rs:24:40
   |
24 | use servo::webvr::{VRExternalShmemPtr, VRMainThreadHeartbeat, VRServiceManager};
   |                                        ^^^^^^^^^^^^^^^^^^^^^ no `VRMainThreadHeartbeat` in `webvr`

error: aborting due to previous error

@asajeffrey asajeffrey force-pushed the asajeffrey:webvr-glwindow branch from eec3e8c to 6ee951a Mar 5, 2019

@asajeffrey

This comment has been minimized.

Copy link
Member Author

asajeffrey commented Mar 5, 2019

@bors-servo r=paulrouget

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 5, 2019

📌 Commit 6ee951a has been approved by paulrouget

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 5, 2019

⌛️ Testing commit 6ee951a with merge b4366e0...

bors-servo added a commit that referenced this pull request Mar 5, 2019

Auto merge of #22953 - asajeffrey:webvr-glwindow, r=paulrouget
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

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 5, 2019

💔 Test failed - status-taskcluster

@asajeffrey asajeffrey force-pushed the asajeffrey:webvr-glwindow branch from 6ee951a to cc2d203 Mar 5, 2019

@asajeffrey

This comment has been minimized.

Copy link
Member Author

asajeffrey commented Mar 5, 2019

Sigh, test-tidy. @bors-servo r=paulrouget

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 5, 2019

📌 Commit cc2d203 has been approved by paulrouget

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 5, 2019

⌛️ Testing commit cc2d203 with merge ac913a6...

bors-servo added a commit that referenced this pull request Mar 5, 2019

Auto merge of #22953 - asajeffrey:webvr-glwindow, r=paulrouget
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

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 5, 2019

💔 Test failed - linux-rel-css

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Mar 5, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 5, 2019

⌛️ Testing commit cc2d203 with merge 55347aa...

bors-servo added a commit that referenced this pull request Mar 5, 2019

Auto merge of #22953 - asajeffrey:webvr-glwindow, r=paulrouget
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

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 5, 2019

@bors-servo bors-servo merged commit cc2d203 into servo:master Mar 5, 2019

3 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.