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

Make softbuffer window contexts own the pixel buffers. #40

Closed
i509VCB opened this issue Dec 21, 2022 · 17 comments
Closed

Make softbuffer window contexts own the pixel buffers. #40

i509VCB opened this issue Dec 21, 2022 · 17 comments
Labels
enhancement New feature or request Needs Design Work
Milestone

Comments

@i509VCB
Copy link
Contributor

i509VCB commented Dec 21, 2022

At the moment all backends need to perform a copy to present. Some backends such as Wayland (always) and X11 (with the SHM extensions) can simply share the pixel memory with the server. On Windows and Mac this saving wouldn't be possible, but it would help on other platforms.

Not sure what redox does in this case.

@ids1024
Copy link
Member

ids1024 commented Dec 21, 2022

Should be possible with dumb/gbm buffers for #42 too, right? Though the implications of working with references into mmaped gbm buffers may be subtler than just shared memory.

Also to provide a slice into an shm buffer, we technically trust the compositor to not modify it for soundness. This is probably fine (and less a concern than the reverse) but it's something to be aware of.

@i509VCB
Copy link
Contributor Author

i509VCB commented Dec 22, 2022

Should be possible with dumb/gbm buffers for #42 too, right? Though the implications of working with references into mmaped gbm buffers may be subtler than just shared memory.

Also to provide a slice into an shm buffer, we technically trust the compositor to not modify it for soundness. This is probably fine (and less a concern than the reverse) but it's something to be aware of.

With dumbuffers I believe that would be possible. GBM I guess mapping memory is weird there.

@jackpot51
Copy link
Member

Redox supports memory mapping window buffers. In fact it is the only way to draw anything. The only problem is the memory needs to be remapped on window resizes, which may complicate the API.

@i509VCB i509VCB closed this as completed Dec 22, 2022
@i509VCB i509VCB reopened this Dec 22, 2022
@i509VCB
Copy link
Contributor Author

i509VCB commented Dec 22, 2022

If we need to ensure that the buffer can't be resized while drawing, we could use a closure in the function to access the buffer which takes an &mut reference.

@notgull
Copy link
Member

notgull commented Dec 22, 2022

Also to provide a slice into an shm buffer, we technically trust the compositor to not modify it for soundness. This is probably fine (and less a concern than the reverse) but it's something to be aware of.

According to the people that I've talked to about this, it may not actually be sound. On the X11 end, it's entirely possible for someone to write an implementation of the X server that maliciously modifies that buffer, and then run that server using our Rust program as a client. While the protocol says that the buffer can't be modified mid-transmission (or at least, not without the appropriate X11 events being delivered), there's nothing really stopping the server from disobeying it. I'm not sure about the Wayland end but I'd imagine that a similar problem exists.

A copy may unfortunately be necessary in the name of safety, in this case.

@i509VCB
Copy link
Contributor Author

i509VCB commented Dec 22, 2022

Yes the server can clobber the entire buffer on Wayland. But every client that has damage relies on the compositor not clobbering said buffer.

No client I've seen however assumes the server will trash the buffer unless given to something like wlr-screencopy which writes to the shm buffer.

@ids1024
Copy link
Member

ids1024 commented Dec 22, 2022

This is the same concern I mentioned earlier. There are a few considerations here:

  • With backends like DRM, presumably this is guaranteed to avoid mutation barring a bug in the kernel or graphics driver. And we rely on a correct and non-malicious kernel for soundness anyway.
  • With Wayland this could be a concern, but as far as malicious modification, the server normally has the same or greater privileges than the client, so trusting the compositor might be reasonable?
    • Similarly with X. Or in remote use cases you wouldn't be able to use shm anyway.
  • I don't think this is likely to lead to exploitable vulnerabilities in practice even with a malicious server, but that's hard to be sure of. And more complicated in a library since the user could potentially be doing things other then merely writing bytes without reading anything.
  • If we don't trust the server for soundness, we could still provide such an API, but it couldn't provide a slice into memory. But it is possible to provide a custom helper type with a custom Index implementation, etc.
    • If so, we'd probably want to provide this no-copy interface in addition to the simpler set_buffer API, rather than only providing it.

@i509VCB
Copy link
Contributor Author

i509VCB commented Dec 22, 2022

There is a significant usability harm if we aren't exposing slices. Many libraries like tiny-skia's PixmapMut takes a mutable slice.

@ids1024
Copy link
Member

ids1024 commented Dec 22, 2022

Yes, for a lot of APIs you would want the buffer or a part of it as a slice, so this could be quite limiting. While for some uses it would mostly work alright.

@john01dav john01dav transferred this issue from rust-windowing/swbuf Dec 22, 2022
@john01dav john01dav added enhancement New feature or request Needs Design Work labels Dec 23, 2022
@notgull
Copy link
Member

notgull commented Dec 26, 2022

Given that we're probably moving in this direction, I figure we should come up with an API for this. Here's some considerations:

  • How do we expose the buffer to the user? Do we have a buffer()/buffer_mut() method set that directly returns a slice? Or, do we want to make access more restrictive (maybe a with_buffer method that takes a closure that operations on a slice).
  • How do we handle resizing the buffer?
  • How should the buffer be pushed to the server?

@ids1024
Copy link
Member

ids1024 commented Dec 27, 2022

For the Wayland backend, at least, we could possibly hand out an owned object that DerefMuts to [u8]. Then the client transfers it back to display the buffer on the screen. That's basically how the current WaylandBuffer type works. But maybe that doesn't make as much sense in other contexts, and isn't actually a more convenient API.

I think buffer/buffer_mut is fine and with_buffer is unnecessary. As long as the method that submits/swaps the buffer takes &mut self, that will ensure the caller no longer has any references to the buffer.

@ids1024
Copy link
Member

ids1024 commented Dec 29, 2022

Another consideration is that set_buffer takes a width/height, while that's needed to allocate/resize buffers before we can provide a slice.

One possible API for this, and #37:

Context::new<T: HasRawDisplayHandle>(display: &T) -> Self;
Surface::new<T: HasRawWindowHandle>(context: &Context, window: &T, width: u32, height: u32); 
Surface::resize(&mut self, width: u32, height: u32);
Surface::size(&self) -> (u32, u32);
Surface::buffer(&self) -> &[u8];
Surface::buffer_mut(&mut self) -> &mut [u8];
Surface::swap_buffers(&mut self);

(Perhaps make all of these return a Result.)

Considering the Wayland implementation, which is what I'm familiar with:

  • Context/Surface/resize/swap_buffers generally mirrors Glutin and other libraries for hardware accelerated rendering. Just instead of OpenGL calls to draw, we have .buffer_mut().
  • This could use either triple or double buffering (double requires waiting for the compositor to release the previous buffer at the end of swap_buffers, or during buffer/buffer_mut). This could also be configurable.
  • buffer/buffer_mut create slices into mmapped shared memory, and trusts the compositor not to mutate the memory for soundness.
  • We could still have a set_buffer function like the current one on top of these methods. With double buffering it could perform basically the same way. (Assuming we block like in wayland: Block dispatching if back buffer isn't released #55).
    • But is this really more ergonomic for an uses? Otherwise it's best to leave out if broken API changes are happening anyway.

For other platforms:

  • If we aren't able to provide a slice into the memory that's sent to the server or it isn't sent over shared memory (for example on X without XShm), the backend can simply have a Vec<u8> for the front buffer, and copy the data over on swap in the same way set_buffer currently does
    • The performance and behavior should be comparable to any other solution assuming the application is able to render into the buffer without storing its own copy.
    • "swap buffers" isn't such an accurate description in this case.
  • What are the contents of the buffer initially? Should we document it as being unspecified, since it may or may not be the same buffer used in the last, second to last, etc. draw, but we also don't want to have to zero it each swap?

@notgull
Copy link
Member

notgull commented Dec 30, 2022

My thoughts:

  • I think we should zero out the buffer. Having an &mut [MaybeUninit<u8>] instead of an &mut [u8] probably isn't ergonomic.
  • Should we also incorporate Add a method for getting pixels from a window #36 into this?
  • I see that you use an &[u8] instead of an &[u32] to meet the use case where formats use a quantum that fits into less than 32 bits. How are we representing formats in this case? An enum? A trait object? If the latter, we could (theoretically) have a Quantum associated type that abstracts over this, and we could instead use &mut [Quantum].

@john01dav
Copy link
Collaborator

I disagree with the proposal to use &[u8]. For formats that nicely fit into &[u32], like we exclusively use right now, it's very convenient to have this type — and all associated alignment requirements — specified in the type.

One way that we could nonetheless support different types is to have a Surface trait with multiple implementations, and then a SurfaceU8, SurfaceU32, etc. trait for each type. For example, you can have SurfaceRgbU32 implements Surface, SurfaceU32, and SurfaceU8 (because &[u32] can be bytemuck-casted to &[u8]), and others as applicable. But, SurfaceGreyscaleU8 would just implement SurfaceU8. You could also do something like the image crate where you have Surface or Surface instead of fully separate classes.

@ids1024
Copy link
Member

ids1024 commented Dec 30, 2022

I think we should zero out the buffer.

I mean we'd zero the buffer when it is allocated or grown, but after swapping buffers it would have the contents of a previous draw. But with single/double/triple buffering that isn't guaranteed either.

Oh right, I forgot the API is currently using &[u32]. Is that what drawing libraries like tiny-skia expect? It could be good to have a way to get the buffer as either type, though the caller can just use something like bytemuck. As for formats other that aren't 32-bit per pixel... not sure what sort of API we'd want for that. We'd not want to represent it as a &[u32], but we'd also need some API for negotiating what format the backend supports. Though even with just 32-bit RGB formats, we want some way to choose RGBA vs RGBX (#17).

SurfaceU8/SurfaceU32/ect could be handled with a bytes per pixel const generic and such, though you quickly get into arcane type errors and things Rust may not support yet.

For #36, we want a way to read the pixels that are in the front buffer, essentially? For Wayland this is easy enough. For other backends it might not be available in an easily readable way client-side, especially with a no-copy API. (If the pixel data was sent over a socket, or uploaded to GPU.)

@john01dav
Copy link
Collaborator

It could be good to have a way to get the buffer as either type, though the caller can just use something like bytemuck.

This may seem obvious to some, but I think that it's important to point out that u8 is not required to be aligned to u32 boundaries. Bytemuck does runtime checks for alignment when casting, but there's no guarantee unless we use the larger types. But, of course, the larger types are guaranteed to be aligned compatibly with the smaller types. That's why I want SurfaceU32/SurfaceU8 etc. That way, we can auto-implement the smaller types for the larger ones via bytemuck, thus exposing all relevant alignment guarantees, and being flexible to users.

@notgull
Copy link
Member

notgull commented Apr 6, 2023

Closed by #65

@notgull notgull closed this as completed Apr 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Needs Design Work
Development

No branches or pull requests

5 participants