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

X11 present #989

Merged
merged 13 commits into from
Jun 30, 2020
Merged

X11 present #989

merged 13 commits into from
Jun 30, 2020

Conversation

jneem
Copy link
Collaborator

@jneem jneem commented May 26, 2020

Fixes #932

This adds support for the X11 Present extension, for scheduling drawing, falling back to the old sleep-based method if Present is absent or causes errors.

The main showstopper for now is a bug in rust-xcb that breaks the present extension. Unfortunately, that project doesn't seem very active -- I see that @xStrom has a PR that's been sitting for a while...

Copy link
Member

@xStrom xStrom left a comment

Choose a reason for hiding this comment

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

I skimmed over this and it looks very promising. Need to carve out some time for a deeper look.

druid-shell/src/platform/x11/application.rs Outdated Show resolved Hide resolved
druid-shell/src/platform/x11/window.rs Outdated Show resolved Hide resolved
@xStrom xStrom added S-needs-review waits for review shell/x11 concerns the X11 backend labels May 26, 2020
@jneem
Copy link
Collaborator Author

jneem commented May 27, 2020 via email

@jneem
Copy link
Collaborator Author

jneem commented Jun 5, 2020

I'd like to make progress on this, but I'm blocked by some showstoppers in rust-xcb, which appears to be inactive...

Is there a good way of making progress there?

@luleyleo
Copy link
Collaborator

luleyleo commented Jun 5, 2020

I'm not sure what to do about this...

I suppose we could try to reach out to the maintainer of xcb directly and find out if he intends to continue his work on xcb, and if not either fork it and maintain it ourselves, or use an alternative (x11rb looks interesting).

@xStrom what do you think?

@luleyleo
Copy link
Collaborator

luleyleo commented Jun 5, 2020

This might be interesting:
https://github.com/psychon/x11rb/blob/master/doc/comparison.md

@jneem
Copy link
Collaborator Author

jneem commented Jun 5, 2020

Hm, yeah. I had a quick look at the docs and they look pretty good (well, the auto-generated code for the extensions is missing docs, but that's not too bad). One downside is that it's fairly new and doesn't have any substantial users on crates.io.

It would also add dependencies on gethostname and nix, but I guess those aren't a big deal.

@psychon
Copy link
Contributor

psychon commented Jun 19, 2020

I never saw anything using the present extension, I think... I kind of expected that one just uses a timer and schedules redraws internally while animations are running.

I did not look at anything in here, but what exactly do you need present for? Things usually use gl when they want vsync'd drawing.

@xStrom xStrom mentioned this pull request Jun 19, 2020
16 tasks
let region_id = conn.generate_id();

if let Err(e) =
xcb::xfixes::create_region_checked(&conn, region_id, &[]).request_check()
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem like an error worth handling (i.e.: Why not use the unchecked variant?). Creating an empty region should never fail, I think. Unless something is out of memory and we have more serious problems than Present support.

Copy link
Member

Choose a reason for hiding this comment

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

Creating an empty region should never fail

That's great until it does. Especially given how much diversity there is in Linux-land with implementations. This check isn't really in a hot path, so being more defensive is fine I think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The general error-handling approach I'm using (which I guess should be written in a comment) is to check everything synchronously when initializing, and then after that to collect errors asynchronously through the event loop.

@jneem
Copy link
Collaborator Author

jneem commented Jun 19, 2020

but what exactly do you need present for? Things usually use gl when they want vsync'd drawing.

The main reason is for vsync, yes. The two options for vsync seem to be present and gl, and present is easier 😄 I found some examples of window managers using present -- they seem to usually have a present path and a gl path, and I just decided to try the present one first.

I was also hoping that present might improve latency (and in particular, give us explicit control over latency). This is more speculative, though, and I don't currently have any way to measure it anyway.

@jneem jneem marked this pull request as draft June 20, 2020 14:04
@jneem
Copy link
Collaborator Author

jneem commented Jun 20, 2020

Resurrecting this now that #1025 is in.
Items left to do:

  • Be less eager with pixmap resizing
  • Do the pixmaps regardless of whether we're using present (meaning we'll always have double-buffering)
  • Add a way (environment variables?) to disable present for testing

@xStrom xStrom added S-waiting-on-author waits for changes from the submitter and removed S-needs-review waits for review labels Jun 20, 2020
@jneem jneem changed the title [WIP] X11 present X11 present Jun 21, 2020
@jneem jneem marked this pull request as ready for review June 21, 2020 23:35
@jneem
Copy link
Collaborator Author

jneem commented Jun 21, 2020

Ok, I think this is in better shape now.

In addition to using the present extension, this also improves the non-present path a little, by drawing into a pixmap and then copying out those pixels. (We use the same pixmap-handling code in both paths; the only difference is whether we present or copy.)

I've tested this in several different configurations, and it seems to work without tearing and with very few missed frames. Funny story: I discovered while testing this that my laptop was using generic video drivers, because I never installed the intel ones 😐 . (I did see some tearing with the generic drivers, but they went away with the intel ones.) I also tested on nouveau drivers.

@luleyleo luleyleo added S-needs-review waits for review and removed S-waiting-on-author waits for changes from the submitter labels Jun 22, 2020
Copy link
Collaborator

@luleyleo luleyleo left a comment

Choose a reason for hiding this comment

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

This is looking pretty good already. I've a bunch more nits, but I think we can get this merged soon.

druid-shell/src/platform/x11/application.rs Outdated Show resolved Hide resolved
druid-shell/src/platform/x11/window.rs Outdated Show resolved Hide resolved
druid-shell/src/platform/x11/window.rs Outdated Show resolved Hide resolved
druid-shell/src/platform/x11/window.rs Outdated Show resolved Hide resolved
druid-shell/src/platform/x11/window.rs Outdated Show resolved Hide resolved
druid-shell/src/platform/x11/window.rs Show resolved Hide resolved
druid-shell/src/platform/x11/window.rs Show resolved Hide resolved
druid-shell/src/platform/x11/window.rs Outdated Show resolved Hide resolved
druid-shell/examples/perftest.rs Show resolved Hide resolved
druid-shell/src/platform/x11/application.rs Show resolved Hide resolved
@luleyleo luleyleo added S-waiting-on-author waits for changes from the submitter and removed S-needs-review waits for review labels Jun 28, 2020
@jneem
Copy link
Collaborator Author

jneem commented Jun 28, 2020

Seems to be a problem with piet-coregraphics? I'll just wait a bit and see if it gets better...

@jneem jneem requested a review from luleyleo June 29, 2020 16:40
@jneem jneem added S-needs-review waits for review and removed S-waiting-on-author waits for changes from the submitter labels Jun 29, 2020
Copy link
Collaborator

@luleyleo luleyleo left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@luleyleo luleyleo removed the S-needs-review waits for review label Jun 30, 2020
@jneem jneem merged commit 3b3d401 into linebender:master Jun 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
shell/x11 concerns the X11 backend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

X11 shell uses sleep to sync rendering.
4 participants