X11 backend #371

i509VCB · 2021-08-25T04:03:22Z

This pull request is part of series in #293.

The X11 backend is renderer agnostic and imports a dmabuf for presentation to a window. The X11 backend also supplies input events, and events indicating window exposure, close requests and resizing.

This backend requires the X server to have the Xfixes, DRI3 and Present extensions. Namely the DRI3 extension means proprietary nvidia gpus cannot use the backend as the X server does not supply those when running with nvidia gpus.

FreeBSD and Non-Linux

These platforms should work although these were not tested. The only functionality not implemented is resolving another type of DrmNode from an existing DrmNode. This functionality isn't strictly necessary to run the backend but may mean some hardware configurations won't work.

DemiMarie · 2021-08-26T04:54:48Z

libX11 is not thread-safe unless XInitThreads is called.

src/backend/x11/mod.rs

src/backend/x11/buffer.rs

psychon · 2021-08-28T08:07:32Z

src/backend/x11/connection.rs

+            XCBConnection::from_raw_xcb_connection(xcb_connection_t, false)
+        };
+
+        if xcb_connection.is_err() {


Off topic: I was briefly thinking "how can this be an error?!?". Then I looked at the code and saw that x11rb has to parse the .setup() that the X11 server sent here and that can go wrong (e.g. the server sent a too short setup claiming to contain 10000 screens). libxcb just does not handle this possibility at all...

Now I am happy about all the safety guarantees that Rust provides.

No one will get this, but since you already wrote on this issue: CC @DemiMarie x11rb is the Rust library I was talking about elsewhere (in case you even care about this - if not, that's totally fine as well).

I do care! For one, this means that Smithay has a chance to have XWayland support that is robust against malicious X servers, assuming that the X server someday gets sandboxed. That would allow for per-Flatpak X servers that run in the Flatpak sandbox, for example.

Does x11rb use libxcb internally and then validate its output, or does it do everything itself?

x11rb I believe decodes the events and encodes requests itself, libxcb only being used to read and write across the socket when the XCBConnection is used.

Does x11rb use libxcb internally and then validate its output, or does it do everything itself?

There are two options in x11rb. There is RustConnection that is pure rust. If you disable the allow-unsafe-code feature, then you lose support for FD-passing, but get all of Rust's safety guarantees always.

With the allow-unsafe-code feature, there is also a XCBConnection. Just as @i509VCB said, this only uses libxcb to send & receive raw bytes (This uses C APIs like xcb_send_request and xcb_wait_for_event / xcb_wait_for_reply from libxcb). Since libxcb does not provide an extra length return, the code has to look at something like three bytes of an X11 packet and figure out the length of the packet, then call the unsafe function std::slice::from_raw_parts. Afterwards, the same safe code is used as with RustConnection. I am pretty sure that these length calculations are exactly the same math as libxcb does internally, but obviously this is a bit less safe than RustConnection.

XCBConnection allows sharing the connection with C code that expects a libxcb xcb_connection_t* (e.g. cairo or mesa). RustConnection is a Rust-only thing that cannot be used in this way.

I bet no one cares, but x11rb even works on Windows. Obviously, there is no FD passing there.

src/backend/x11/connection.rs

src/backend/x11/window.rs

src/backend/x11/mod.rs

psychon · 2021-08-28T08:33:55Z

src/backend/x11/mod.rs

+                    x11::Event::Expose(expose) => {
+                        // TODO: We would ideally use this to determine damage and render more efficiently that way.
+                        //
+                        // Although there is an Expose with damage event somewhere?


I don't understand the question, but I'll try to answer it anyway:

A expose event contains a rectangle that was exposed (x, y, width, height). If a non-rectangular area is exposed (e.g. Two other windows partially cover your window and now your window is brought to front. Thus, these two rectangles are exposed), then two Expose events are sent. The first one contains .count == 1 to indicate that another event will follow.

(And if there were four rectangles, then the first one would have count = 3, the next one count = 2 etc...)

src/backend/x11/buffer.rs

i509VCB · 2021-09-24T20:13:58Z

libX11 is not thread-safe unless XInitThreads is called.

XCB FFI is no longer necessary and is now no longer included in the backend.

anvil/src/x11.rs

psychon · 2021-09-26T05:25:55Z

anvil/src/x11.rs

+
+                        Err(err) => {
+                            if let SwapBuffersError::ContextLost(err) = err {
+                                error!(log, "Critical Rendering Error: {}", err);


Errors other than ContextLost are just silently ignored? Is that right?

Copied from winit is the excuse here, probably not a great thing to copy either.

build.rs

src/backend/x11/buffer.rs

src/backend/x11/mod.rs

src/backend/x11/window_inner.rs

anvil/src/x11.rs

i509VCB

Reviewing my own code 🤔

src/backend/x11/buffer.rs

src/backend/x11/mod.rs

src/backend/x11/window_inner.rs

src/backend/x11/drm.rs

src/backend/x11/mod.rs

src/backend/x11/buffer.rs

Cargo.toml

anvil/src/main.rs

elinorbgr · 2021-10-16T11:41:13Z

anvil/src/input_handler.rs

+impl AnvilState<X11Data> {
+    pub fn process_input_event<B: InputBackend>(&mut self, event: InputEvent<B>) {
+        match event {
+            InputEvent::Keyboard { event } => match self.keyboard_key_to_action::<B>(event) {


I feel like this could have some deduplication wrt other backends

elinorbgr · 2021-10-16T11:47:04Z

Also it looks like this needs a rebase now.

Drakulix

Some minor nitpicks, but wow this PR is clean!
I really like it, thanks a lot.

Also there is some stuff, that should be de-duplicated in the future. E.g. X11's buffers share a lot of code with the GbmBufferedSurface, but we can do that later. This PR has gotten big enough.

anvil/Cargo.toml

anvil/src/x11.rs

src/reexports.rs

Cargo.toml

psychon reviewed Aug 28, 2021

View reviewed changes

src/backend/x11/mod.rs Outdated Show resolved Hide resolved

psychon reviewed Aug 28, 2021

View reviewed changes

src/backend/x11/mod.rs Outdated Show resolved Hide resolved

psychon reviewed Aug 28, 2021

View reviewed changes

psychon reviewed Aug 29, 2021

View reviewed changes

src/backend/x11/buffer.rs Outdated Show resolved Hide resolved

i509VCB force-pushed the x-client branch from a2eb2b6 to 3f3ccbf Compare September 9, 2021 22:51

i509VCB changed the title ~~Direct X11 backend~~ X11 backend Sep 9, 2021

i509VCB force-pushed the x-client branch from 6046e3d to 0b8d5bf Compare September 25, 2021 08:16

i509VCB marked this pull request as ready for review September 26, 2021 04:38

psychon reviewed Sep 26, 2021

View reviewed changes

i509VCB force-pushed the x-client branch from de97a47 to 330b5d9 Compare September 30, 2021 22:06

i509VCB mentioned this pull request Oct 1, 2021

Segfaults when OpenGL application exits when the dmabuf global is not registered. #386

Open

i509VCB commented Oct 2, 2021

View reviewed changes

anvil/src/x11.rs Outdated Show resolved Hide resolved

i509VCB commented Oct 2, 2021

View reviewed changes

psychon reviewed Oct 3, 2021

View reviewed changes

src/backend/x11/drm.rs Outdated Show resolved Hide resolved

DemiMarie suggested changes Oct 7, 2021

View reviewed changes

src/backend/x11/mod.rs Outdated Show resolved Hide resolved

src/backend/x11/mod.rs Outdated Show resolved Hide resolved

psychon reviewed Oct 13, 2021

View reviewed changes

src/backend/x11/buffer.rs Outdated Show resolved Hide resolved

i509VCB commented Oct 14, 2021

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

anvil/src/main.rs Outdated Show resolved Hide resolved

elinorbgr reviewed Oct 16, 2021

View reviewed changes

i509VCB force-pushed the x-client branch 3 times, most recently from aed87a4 to 431fdce Compare October 17, 2021 07:31

Drakulix requested changes Oct 17, 2021

View reviewed changes

anvil/Cargo.toml Outdated Show resolved Hide resolved

anvil/src/x11.rs Show resolved Hide resolved

src/reexports.rs Outdated Show resolved Hide resolved

Cargo.toml Outdated Show resolved Hide resolved

i509VCB force-pushed the x-client branch from 431fdce to 029a912 Compare October 17, 2021 18:44

i509VCB added 4 commits October 17, 2021 13:46

anvil: Derive missing fmt::Debug

d041e5c

Add DrmNode to backend::drm.

4d93805

Move X11Source to smithay underx11rb_event_source feature

c8d56f1

Implement Hash on some enums in backend::input

953d9ce

X11 backend

2e68cfd

i509VCB force-pushed the x-client branch from 029a912 to 2e68cfd Compare October 17, 2021 18:46

Drakulix approved these changes Oct 17, 2021

View reviewed changes

Drakulix merged commit 7ad40d3 into Smithay:master Oct 18, 2021

i509VCB mentioned this pull request Oct 28, 2021

Consider having direct Wayland/X11 backends instead of winit #293

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

X11 backend #371

X11 backend #371

i509VCB commented Aug 25, 2021 •

edited

Loading

DemiMarie commented Aug 26, 2021

psychon Aug 28, 2021

DemiMarie Sep 9, 2021

i509VCB Sep 9, 2021 •

edited

Loading

psychon Sep 13, 2021 •

edited

Loading

psychon Aug 28, 2021

i509VCB commented Sep 24, 2021

psychon Sep 26, 2021

i509VCB Sep 27, 2021

i509VCB left a comment

elinorbgr Oct 16, 2021

elinorbgr commented Oct 16, 2021

Drakulix left a comment •

edited

Loading

X11 backend #371

X11 backend #371

Conversation

i509VCB commented Aug 25, 2021 • edited Loading

FreeBSD and Non-Linux

DemiMarie commented Aug 26, 2021

psychon Aug 28, 2021

Choose a reason for hiding this comment

DemiMarie Sep 9, 2021

Choose a reason for hiding this comment

i509VCB Sep 9, 2021 • edited Loading

Choose a reason for hiding this comment

psychon Sep 13, 2021 • edited Loading

Choose a reason for hiding this comment

psychon Aug 28, 2021

Choose a reason for hiding this comment

i509VCB commented Sep 24, 2021

psychon Sep 26, 2021

Choose a reason for hiding this comment

i509VCB Sep 27, 2021

Choose a reason for hiding this comment

i509VCB left a comment

Choose a reason for hiding this comment

elinorbgr Oct 16, 2021

Choose a reason for hiding this comment

elinorbgr commented Oct 16, 2021

Drakulix left a comment • edited Loading

Choose a reason for hiding this comment

i509VCB commented Aug 25, 2021 •

edited

Loading

i509VCB Sep 9, 2021 •

edited

Loading

psychon Sep 13, 2021 •

edited

Loading

Drakulix left a comment •

edited

Loading