Add a function for retrieving the window contents #104

notgull · 2023-05-13T03:18:44Z

Closes #36 by adding a fetch() method for the Buffer method. Might need a rework in the face of #98 or #99.

WIP for now, with only implementations for X11 and Win32. I would appreciate guidance from the platform maintainers on how this would be implemented on other platforms.

ids1024 · 2023-05-13T05:53:23Z

On Wayland, if we want to read from the back buffer we can just do that. If we want to get window contents that this softbuffer Surface didn't present, there's no way to do so. For macOS and WASM presenting currently still copies, so this could just be a no-op. On Redox, presumably mapping the window and reading will work.

I'm not sure exactly how this API would be used though. It doesn't seem ideal to use every frame. If we have make a request to the display server and copy all the pixels to do this, would to be better for the application that wants to retain graphics to just have it's own persistent buffer and copy into the softbuffer::Buffer when it draws?

daxpedda · 2023-05-13T10:40:58Z

Web implementation: #105.

If we want to get window contents that this softbuffer Surface didn't present, there's no way to do so.

I assumed that this is the purpose of this API. I implemented the Web implementation accordingly, otherwise it would be no-op like you suggested.

daxpedda · 2023-05-13T10:55:37Z

Additionally, I'm unsure how other platforms then Web handle this, but what if the surface was resized, what does fetch do in this case, error? Or just get part of the contents that fit into the buffer? Does it zero out the rest if the fetched content is smaller then the current size?

ids1024 · 2023-05-13T19:09:22Z

I assumed that this is the purpose of this API.

If so, this isn't possible on Wayland. I'm guessing it's not possible on macOS given how the APIs work, but I would be less confident there.

It's not necessarily bad to expose features that only work on some backends, but I'd like to see more clearly what the actual application for this would be. And whether or not its the best way to achieve that.

notgull · 2023-05-13T19:48:26Z

The goal of this API is to get the currently presented contents of the buffer and download it to the client. This is nice for testing, since it allows you to get the image created by the user. It would be nice for testing not only softbuffer but also drawing frameworks like theo.

ids1024 · 2023-05-13T21:31:32Z

Ah. For debugging uses it makes more sense. Then the performance concerns that would otherwise apply here aren't so much of a problem.

It should be clearly documented what this is and is not suitable for though.

I also wonder whether a method on Buffer like this is best. For debugging purposes, a Surface method returning Result<Vec<u32>, _> could be more suitable. If it's updating the contents of the Buffer that would clash with incremental rendering using buffer age when #99 is merged.

notgull · 2023-05-14T04:18:11Z

I also wonder whether a method on Buffer like this is best. For debugging purposes, a Surface method returning Result<Vec<u32>, _> could be more suitable. If it's updating the contents of the Buffer that would clash with incremental rendering using buffer age when #99 is merged.

That's probably a good idea. I've changed this PR to use this design.

daxpedda · 2023-05-14T09:28:08Z

tests/present_and_fetch.rs

+
+    // Set all pixels to red.
+    let mut buffer = surface.buffer_mut().unwrap();
+    buffer.fill(0xFF0000FF);


That looks like blue to me.

softbuffer/src/lib.rs

Line 340 in fcda747

/// 00000000RRRRRRRRGGGGGGGGBBBBBBBB

Suggested change

buffer.fill(0xFF0000FF);

buffer.fill(0x00FF0000);

0x00FF0000 won't be the same color on big-endian systems, right? If we care about those.

Something like u32::from_ne_bytes could be used for consistent behavior.

I don't think so? I went down a rabbit hole for a bit there and confirmed two things:

Literals are always written in big-endian in Rust. I couldn't find any documentation on that, so I tried it out: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=c1a0820eba008226bae7771c1f510fe8.

Bitshift operators always act in big-endian (except on some old PowerPC architectures I read somewhere?).

So I think what we are doing in Rust right now, seems to be pretty much cross-endianness compatible.
What I don't know is if the APIs expect us to turn things around on big-endian targets.
I also assume nobody here actually has a big-endian system lying around to try it out?

That said, on Web, my purview, this isn't a concern.

I think the use of bitshifts in the examples would also be wrong on big endian. We represent color with u32, but it's really interpreted more like [u8; 4] but with 4 byte alignment. Which isn't dependent on endianess.

I don't have a big endian system to test on, though I fixed a similar issue in tiny-skia that someone reported: linebender/tiny-skia#69. That one is easy to test with virtualization, but this is a bit more complicated since one may want a full OS with a display server. Maybe X11 backend over TCP could work...

Maybe X11 backend over TCP could work...

Sounds like fun, please share the results when you get them 🤣.

It can be faster to work with 32-bit values than individual bytes. And things like tiny-skia work on &[u32]. And you can safely cast &[u32] to &[u8] but not the other way around due to alignment.

I'm not sure what to think of this, there isn't actually a safe method to cast &[u32] to &[u8] I know of. I would argue that u32 is definitely a worse user interface and the improved speed is speculation (I actually agree with that speculation).

Just an idea: what do you think of &[[u8; 4]]?

[u32] has alignment 4 (on typical architectures), while [u8; 4] has alignment 1. So there's not currently a standard library method to cast &[u32] to &[u8], but it's always sound while the reverse is not true. Likewise it is potentially unsound to cast &[[u8; 4]] to &[u32].

bytemuck is popular for this sort of thing.

while [u8; 4] has alignment 1.

Ah, apologies, was thinking that this does have an alignment of 4 actually, for some reason (I think I played around with too much shaders lately ...).

Alright, w/e, I think I'm still in favor of &[u8] until we can show a reasonable performance degradation, but this is an issue for another time.

In the meantime, your original problem still needs to be addressed. I think we should also document this somewhere.

softbuffer/src/lib.rs

Line 340 in fcda747

/// 00000000RRRRRRRRGGGGGGGGBBBBBBBB

This could be a bit confusing, because it's showing big-endian order, which most platforms aren't, but it's also representative of how literals are written in Rust.

I say we handle it afterwards: #109.

daxpedda · 2023-05-14T09:30:16Z

The goal of this API is to get the currently presented contents of the buffer and download it to the client.

So should this account for stuff not presented by softbuffer? Because if not, backends that don't have a buffer age can just return their buffer (if I understand this correctly).

notgull · 2023-05-14T14:46:13Z

The goal of this API is to get the currently presented contents of the buffer and download it to the client.

So should this account for stuff not presented by softbuffer? Because if not, backends that don't have a buffer age can just return their buffer (if I understand this correctly).

Yes, this is what I wanted. Right now, my goal is to have a reproducible test harness; not only in softbuffer, but in other crates too. For theo, what I want to do is:

Render an image to the window.
Use softbuffer to fetch that image.
Compare that image against an existing image to detect regressions.

ids1024 · 2023-05-14T16:43:37Z

We'll need to consider how this interacts with #98. Does the method fetch accept a format argument? Would the supported formats be the same as for presenting? Or does it return a value tagged with the format the surface contains? I'm not sure exactly how it works in the backends here.

Conversion is not necessarily lossless, if we support pixel formats that aren't just orderings RGB (though again, things like 10 bit color won't work with u32 pixels).

#98 is probably the most important feature missing in softbuffer currently, though I'm still not sure how best to design that, including conversion functionality.

daxpedda · 2023-05-14T18:16:33Z

Render an image to the window.

Use softbuffer to fetch that image.

On Web, the Context created has to be the same, if the original drawer used "webgl2", softbuffer will fail, because it's using "2d". Not sure if we care about this case, considering we are "softbuffer" and all.

Same if transferControlToOffscreen() was used, though this can be worked around with #103.

notgull · 2023-05-19T18:56:45Z

I think this is ready now. macOS, Wayland and Redox are unimplemented for now; I'm hoping that the platform maintainers will be able to take care of that.

Fix X11 bugs - Vec not being filled all the way - Tests need to be run with Xvfb - Add note about visibility

Use winit-test as a test harness Redo testing harness for WASM Make sure the wayland runner doesn't use xvfb

Adjust Web code style

Adjust `fetch()` test for Web Test fixes

Add line to docs about unsupported platforms Test fixes

notgull · 2023-05-19T19:38:56Z

I'm unsure why WASM is failing by returning an empty buffer, nor am I sure why Windows seems to spurious fail in the same way.

daxpedda · 2023-05-19T19:48:24Z

Web fails because #104 (comment) wasn't applied. Currently fails on X11 because of that too.

I confirmed that both succeed when applying #104 (comment).

src/lib.rs

notgull · 2023-06-02T02:56:03Z

Looks like adding GdiFlush fixed the Win32 backend. Hopefully this means that all of the outstanding issues are resolved.

* On MacOS, the contents scale is updated when set_buffer() is called, to adapt when the window is on a new screen (#68). * **Breaking:** Split the `GraphicsContext` type into `Context` and `Surface` (#64). * On Web, cache the document in the `Context` type (#66). * **Breaking:** Introduce a new "owned buffer" for no-copy presentation (#65). * Enable support for multi-threaded WASM (#77). * Fix buffer resizing on X11 (#69). * Add a set of functions for handling buffer damage (#99). * Add a `fetch()` function for getting the window contents (#104). * Bump MSRV to 1.64 (#81).

Add API for fetch()

2ea54c1

daxpedda mentioned this pull request May 13, 2023

Add fetch() Web Implementation #105

Merged

daxpedda mentioned this pull request May 13, 2023

Initial Window Size #106

Open

daxpedda reviewed May 14, 2023

View reviewed changes

This was referenced May 19, 2023

Add OffscreenCanvas support #103

Merged

Endianness Considerations #109

Open

notgull marked this pull request as ready for review May 19, 2023 18:53

notgull requested review from jackpot51, madsmtm and john01dav as code owners May 19, 2023 18:53

notgull and others added 3 commits May 19, 2023 12:00

Add an impl for X11

35c4276

Fix X11 bugs - Vec not being filled all the way - Tests need to be run with Xvfb - Add note about visibility

Add a test harness for fetch/present

d721a36

Use winit-test as a test harness Redo testing harness for WASM Make sure the wayland runner doesn't use xvfb

Add Web implementation

a449875

Adjust Web code style

notgull force-pushed the notgull/fetch branch from 711523a to 5501cef Compare May 19, 2023 19:04

Move fetch() to the surface

d5ca690

Adjust `fetch()` test for Web Test fixes

notgull force-pushed the notgull/fetch branch from 5501cef to 5b80dae Compare May 19, 2023 19:23

Return Unimplemented error on other platforms

c92a99c

Add line to docs about unsupported platforms Test fixes

notgull force-pushed the notgull/fetch branch from 5b80dae to c92a99c Compare May 19, 2023 19:29

Try blue

e4ea2d5

ids1024 reviewed May 29, 2023

View reviewed changes

src/lib.rs Show resolved Hide resolved

daxpedda mentioned this pull request May 29, 2023

Document possible errors for Web #113

Merged

daxpedda and others added 2 commits May 29, 2023 13:44

Document possible errors for Web (#113)

0eb4eaf

Try adding GdiFlush to Win32

65b5d48

ids1024 approved these changes Jun 2, 2023

View reviewed changes

notgull merged commit 4424847 into master Jun 2, 2023

notgull deleted the notgull/fetch branch June 2, 2023 03:09

daxpedda mentioned this pull request Jun 2, 2023

Update criterion requirement from 0.4.0 to 0.5.1 #112

Closed

ids1024 mentioned this pull request Jun 2, 2023

Add Buffer::present_with_damage() and Buffer::age() #99

Merged

notgull mentioned this pull request Jun 4, 2023

v0.3.0 #118

Merged

daxpedda mentioned this pull request Jun 4, 2023

Windows CI flaky test #119

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a function for retrieving the window contents #104

Add a function for retrieving the window contents #104

notgull commented May 13, 2023

ids1024 commented May 13, 2023

daxpedda commented May 13, 2023

daxpedda commented May 13, 2023

ids1024 commented May 13, 2023

notgull commented May 13, 2023

ids1024 commented May 13, 2023

notgull commented May 14, 2023

daxpedda May 14, 2023

ids1024 May 14, 2023

daxpedda May 14, 2023

ids1024 May 14, 2023

daxpedda May 14, 2023 •

edited

Loading

ids1024 May 16, 2023

daxpedda May 16, 2023

ids1024 May 16, 2023

daxpedda May 16, 2023

daxpedda May 19, 2023

daxpedda commented May 14, 2023 •

edited

Loading

notgull commented May 14, 2023

ids1024 commented May 14, 2023

daxpedda commented May 14, 2023

notgull commented May 19, 2023

notgull commented May 19, 2023

daxpedda commented May 19, 2023 •

edited

Loading

notgull commented Jun 2, 2023

Add a function for retrieving the window contents #104

Add a function for retrieving the window contents #104

Conversation

notgull commented May 13, 2023

ids1024 commented May 13, 2023

daxpedda commented May 13, 2023

daxpedda commented May 13, 2023

ids1024 commented May 13, 2023

notgull commented May 13, 2023

ids1024 commented May 13, 2023

notgull commented May 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxpedda May 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxpedda commented May 14, 2023 • edited Loading

notgull commented May 14, 2023

ids1024 commented May 14, 2023

daxpedda commented May 14, 2023

notgull commented May 19, 2023

notgull commented May 19, 2023

daxpedda commented May 19, 2023 • edited Loading

notgull commented Jun 2, 2023

daxpedda May 14, 2023 •

edited

Loading

daxpedda commented May 14, 2023 •

edited

Loading

daxpedda commented May 19, 2023 •

edited

Loading