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: Implement image transfer using the MIT-SHM extension #46

Merged
merged 5 commits into from
Jan 6, 2023

Conversation

notgull
Copy link
Member

@notgull notgull commented Dec 23, 2022

This PR implements image transfer using the MIT-SHM extension, allowing us to avoid image transfers over the wire.

At the moment I can't test this on my machine, as the x11-dl crate has a bug that prevents it from loading the MIT-SHM extension (AltF02/x11-rs#164). Circumvented this by moving to x11rb.

Closes #38

(This is a port of rust-windowing/swbuf#23 to the new repository)

src/x11.rs Outdated Show resolved Hide resolved
src/x11.rs Outdated Show resolved Hide resolved
@notgull
Copy link
Member Author

notgull commented Jan 4, 2023

I'm waiting for #55 to make progress before I continue on this, since I'll probably reuse whatever SHM mechanism that uses here.

@notgull notgull added Xcb enhancement New feature or request labels Jan 4, 2023
@jackpot51
Copy link
Member

I'm waiting for #55 to make progress before I continue on this, since I'll probably reuse whatever SHM mechanism that uses here.

55 is merged

@jackpot51
Copy link
Member

Will this gracefully fall back to the old method if MIT-SHM is not available? Would that be the case when using X11 forwarding, like with SSH?

@notgull
Copy link
Member Author

notgull commented Jan 5, 2023

Yes, this falls back to the wire method if it determines that SHM is not available.

This is still marked as draft because I realized that I left a leak in it before I left for the gym. Let me fix that before I mark it as ready.

src/x11.rs Show resolved Hide resolved
src/x11.rs Show resolved Hide resolved
src/x11.rs Show resolved Hide resolved
src/x11.rs Show resolved Hide resolved
@notgull notgull marked this pull request as ready for review January 5, 2023 17:41
src/x11.rs Outdated Show resolved Hide resolved
src/x11.rs Show resolved Hide resolved
@ids1024
Copy link
Member

ids1024 commented Jan 5, 2023

I guess on vaguely modern versions of X11, we have the option to use https://docs.rs/x11rb/0.11.0/x11rb/protocol/shm/trait.ConnectionExt.html#method.shm_attach_fd instead of using shm_attach with System V shared memory segments? Not sure if there are an non-historical context in which shm would be available but that isn't. Or if there are any other drawbacks.

That could share code for allocating/growing with Wayland.

@notgull
Copy link
Member Author

notgull commented Jan 5, 2023

I guess on vaguely modern versions of X11, we have the option to use https://docs.rs/x11rb/0.11.0/x11rb/protocol/shm/trait.ConnectionExt.html#method.shm_attach_fd instead of using shm_attach with System V shared memory segments? Not sure if there are an non-historical context in which shm would be available but that isn't. Or if there are any other drawbacks.

The main drawback would be not being able to use IPC_PRIVATE, although I think you mentioned something earlier about IPC_PRIVATE causing issues on BSD? If so I can rewrite this to use shm_open et al.

@ids1024
Copy link
Member

ids1024 commented Jan 5, 2023

I think all I said about IPC_PRIVATE previously is that since it's a flag for shmget it can't be used with Wayland, which needs something with an fd.

I guess there's no real need to change it. (Or at least, not that I'm familiar with.) Though needing to deal with the limitations of shm_open isn't too much of a drawback given we have to have code for that anyway.

@notgull
Copy link
Member Author

notgull commented Jan 5, 2023

If it's all the same, then I'd rather keep the idiomatic SHM wrapper I have right now.

@jackpot51
Copy link
Member

I think this can be merged once @ids1024 approves

@ids1024
Copy link
Member

ids1024 commented Jan 6, 2023

The code generally looks good now. Has it been tested that it correctly falls back to not using shm on a remote X server?

With XShm, any other X client could get the shm segment for our window, map it, and read or write to our window buffer, right? This is a bit of a concern for #40. I think(?) that shouldn't be a soundness concern with mem::copy_nonoverlapping as used here.

@notgull
Copy link
Member Author

notgull commented Jan 6, 2023

The code generally looks good now. Has it been tested that it correctly falls back to not using shm on a remote X server?

I've tested it, and it successfully falls back to the wire method for transfer, so we're good there.

With XShm, any other X client could get the shm segment for our window, map it, and read or write to our window buffer, right?

I don't believe that there is a way for an X client to get the SHM ID in use by another X client unless there's a server bug.

I think this can be merged once @ids1024 approves

Okay, I'll merge this PR!

@notgull notgull merged commit a6042f6 into master Jan 6, 2023
@notgull notgull deleted the notgull/x11-shm branch January 6, 2023 03:27
@ids1024
Copy link
Member

ids1024 commented Jan 6, 2023

Ah. I was thinking that a compositor/compositing WM would read directly from the shm buffer, but I guess that's handled in a different way.

@psychon
Copy link
Contributor

psychon commented Jan 6, 2023

I guess on vaguely modern versions of X11, we have the option to use https://docs.rs/x11rb/0.11.0/x11rb/protocol/shm/trait.ConnectionExt.html#method.shm_attach_fd instead of using shm_attach with System V shared memory segments?

Uhm... I have no idea what Xorg does in this case for non-local connections, but I wouldn't be surprised if this fails by breaking the whole X11 connection. At least I would suggest some testing before trying to use this.

I was thinking that a compositor/compositing WM would read directly from the shm buffer, but I guess that's handled in a different way.

I think there is always a memcpy involved. It might be possible to get direct scanout via SHM CreatePixmap and the present extension, but I don't think that's implemented anywhere. (But this is mostly just random guesses)

Also, the true here:

conn.shm_attach(new_id, seg.id(), true)?.ignore_error();

is a parameter called read_only. I would guess this means that the X11 server maps the shared memory read-only, which should make any aliasing a non-issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Xcb Xlib
Development

Successfully merging this pull request may close these issues.

Use a shared memory buffer for the X11 backend
5 participants