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

Port to upstream Glutin #2385

Merged
merged 4 commits into from Feb 8, 2018
Merged

Port to upstream Glutin #2385

merged 4 commits into from Feb 8, 2018

Conversation

@kvark
Copy link
Member

kvark commented Feb 6, 2018

Fixes #2356
cc @tomaka


This change is Reviewable

@kvark kvark force-pushed the kvark:glutin branch from 69bfdb6 to 13521d2 Feb 6, 2018
@kvark kvark changed the title [WIP] Port to upstream glutin Port to upstream Glutin Feb 6, 2018
@kvark
Copy link
Member Author

kvark commented Feb 6, 2018

The code is ready now, although I'm concerned about the input lag. When I just start, say the animation example (cd webrender && cargo run --example animation) it appears to only respond to keys in a few seconds after start, and even then the lag is noticeable. The old code of both wrench and example boilerplate used wait_events, and now I'm changing it to run_forever. Any ideas @tomaka?

@tomaka
Copy link

tomaka commented Feb 6, 2018

Is it on X11? The X11 code has changed considerably recently, so it's not impossible that a bug sneaked in.

@kvark
Copy link
Member Author

kvark commented Feb 6, 2018

@tomaka yes, I'm on X11

@kvark
Copy link
Member Author

kvark commented Feb 6, 2018

Ugh, another problem: https://tools.taskcluster.net/groups/HBUSRLrbSAqmv8kJJnjljg/tasks/HBUSRLrbSAqmv8kJJnjljg/runs/0/logs/public%2Flogs%2Flive.log#L1254

../target/release/build/osmesa-src-31be08b90e5b125b/out/lib/gallium
thread 'main' panicked at 'No backend is available', /home/worker/.cargo/registry/src/github.com-1ecc6299db9ec823/winit-0.10.0/src/platform/linux/mod.rs:346:8

@kvark kvark force-pushed the kvark:glutin branch 3 times, most recently from f8bc1ad to 2f36dfb Feb 6, 2018
@kvark
Copy link
Member Author

kvark commented Feb 7, 2018

cc @staktrace ^
For some reason, the upstream glutin doesn't like running headless on TaskCluster CI. It does work locally on Linux for me though

@staktrace
Copy link
Contributor

staktrace commented Feb 7, 2018

I don't know offhand what the problem here is. It looks like the panic is because winit cannot initialize either wayland or X, but if we're running headless I would have thought we wouldn't need either of those?

@kvark
Copy link
Member Author

kvark commented Feb 7, 2018

I guess one way to proceed would be to avoid creating EventsLoop on headless

@staktrace
Copy link
Contributor

staktrace commented Feb 7, 2018

Also just a note - since you're modifying .travis.yml you should make the same change to .taskcluster.yml or the two CIs will get out of sync.

@kvark
Copy link
Member Author

kvark commented Feb 7, 2018

@staktrace thanks for the heads up! should be good now.
I think we should have a Makefile to have those basic commands called from either of the CI configs.

@staktrace
Copy link
Contributor

staktrace commented Feb 7, 2018

Yeah I think servo has a etc/ci/ folder with automation scripts, something similar would be good to do here

@kvark
Copy link
Member Author

kvark commented Feb 7, 2018

@glennw this should be ready for review (assuming CI passes). Would be great if you could pull this change on your end and see if everything (namely, examples and wrench) still works as expected.

@glennw
Copy link
Member

glennw commented Feb 7, 2018

@kvark Sure, I'll do some testing and review it today.

@glennw
Copy link
Member

glennw commented Feb 7, 2018

Reviewed 9 of 11 files at r1, 5 of 7 files at r2, 2 of 2 files at r3, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


webrender/Cargo.toml, line 46 at r4 (raw file):

env_logger = "0.5"
rand = "0.3"                # for the benchmarks
glutin = "0.12"    		 	# for the example apps

nit: tabs here instead of spaces


Comments from Reviewable

@glennw
Copy link
Member

glennw commented Feb 7, 2018

The tiniest of nits only - still building locally to actually test though.

@kvark kvark force-pushed the kvark:glutin branch from 3196d51 to 0d3b23d Feb 7, 2018
@glennw
Copy link
Member

glennw commented Feb 7, 2018

OK, I tested this a bit - using Wayland on Ubuntu 17.10.

The window decorations are a bit strange (very different to a normal window), and there is no title. Resizing also doesn't work correctly (but I think that was the case previously too, just that you now see uninitialized GPU memory :) ).

I'm fine with landing it like that - I think it's a net win, and it doesn't affect the main functionality.

So, r=me when you're ready to merge this.

@glennw glennw self-requested a review Feb 7, 2018
@glennw
glennw approved these changes Feb 7, 2018
@bors-servo
Copy link
Contributor

bors-servo commented Feb 7, 2018

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

@kvark kvark force-pushed the kvark:glutin branch from 0d3b23d to 884b5e9 Feb 7, 2018
@glennw
Copy link
Member

glennw commented Feb 8, 2018

@bors-servo
Copy link
Contributor

bors-servo commented Feb 8, 2018

📌 Commit 884b5e9 has been approved by glennw

@bors-servo
Copy link
Contributor

bors-servo commented Feb 8, 2018

Testing commit 884b5e9 with merge a492386...

bors-servo added a commit that referenced this pull request Feb 8, 2018
Port to upstream Glutin

Fixes #2356
cc @tomaka

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

bors-servo commented Feb 8, 2018

☀️ Test successful - status-appveyor, status-taskcluster, status-travis
Approved by: glennw
Pushing a492386 to master...

@bors-servo bors-servo merged commit 884b5e9 into servo:master Feb 8, 2018
3 of 4 checks passed
3 of 4 checks passed
code-review/reviewable 5 files, 1 discussion left (glennw, kvark)
Details
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@kvark kvark deleted the kvark:glutin branch Feb 8, 2018
@atouchet
Copy link
Contributor

atouchet commented Feb 10, 2018

Could we get a Servo WR update with this to address servo/servo#19901?

@kvark
Copy link
Member Author

kvark commented Feb 10, 2018

@atouchet If you are talking about updating WR version used in Servo, than it wouldn't help. This PR only ports the examples and wrench, which have nothing to do with Servo.

@atouchet
Copy link
Contributor

atouchet commented Feb 10, 2018

Alright, I was under the impression that a WebRender update caused the breakage but I guess that is not actually the case then.

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.