eframe: capture a screenshot using `Frame::request_screenshot` #2676

amfaber · 2023-02-05T08:58:50Z

Hi!

As per #2642, I would like to capture the frames produced by egui.
I decided to take a stab at implementing the feature myself, and hopefully contributing back to this awesome repo. The implementation concerns two distinct areas.

Exposing the possibility for the app the communicate to the backend that it would like to capture the current frame, as well as allowing the backend to hand the data back to the app. This is all done in eframe.
Implementing the actual screen capture for the wgpu backend.

As I am only familiar with wgpu, I have not attempted a glow implementation.

Description of the changes

is fairly straightforward. It adds the boolean field "pixels_requested" to AppOutput, allowing the app to communicate to the backend that it wants a screencapture, as well as the field "pixel_data" to Frame, in order to be able to pass the data back to the app during App::post_rendering.
is a little bit heavier, but localized to the wgpu backend. The fundamental strategy is that when a screencapture is requested, instead of rendering to the surface texture, we do all the rendering of the frame to a texture that is wgpu::TextureUsages::COPY_SRC, which allows us to copy the frame's data to both the surface texture as well as a wgpu::Buffer which is wgpu::BufferUsages::MAP_READ, from which the data can be loaded back onto the cpu. This new pair of texture and buffer need to have the same dimensions as the surface texture, and so they are updated if they need to be used for a frame capture but their dimensions no longer match up with the surface. They are never initialized if a screen capture is never requested.
As the intention is to deliver back a Vec of rgba values for the screen, the bytes need to be reshuffled in the case that the surface texture is wgpu::TextureFormat::Bgra8Unorm. Currently, screen capturing is only supported if the surface is either wgpu::TextureFormat::Rgba8Unorm or wgpu::TextureFormat::Bgra8Unorm, panic'ing if the screen is another format AND a capture is requested.

Concerns

I have tried to make 1. as non-intrusive as possible, still only exposing a &Frame to the app's post_rendering, while allowing the app to take the generated screenshot from Frame via compile-time interior mutability with std::cell::Cell.
One thing that bugs me is that there is now another flag on AppOutput that isn't handled by either handle_app_output or handle_platform_output, but since the nature of the feature is so tightly coupled with the rendering backend, I don't see a way around that.

As for 2., it ended up being very coupled to Painter::paint_and_update_textures. Again I don't see a way around this, as during testing on my Macbook, I found that wgpu::TextureUsages::COPY_SRC is not an allowed usage for the surface texture, necessitating that we instead make the surface wgpu::TextureUsages::COPY_DST and render to a separate texture which is wgpu::TextureUsages::COPY_SRC. The fact that we need to render to a completely different texture when we intend to take a screencapture means that we will have to interact closely with Painter::paint_and_update_textures. My current solution is by adding a boolean argument for whether we are doing a screen capture, and having the function return a Some(Vec) only if a capture was requested and it succeded, and otherwise None.

For now I have handled all the fallible operations (except if the surface format isn't recognized) through Options, but I think would probably make sense to make an Error type for this that can be exposed to the user in Frame::frame_pixels. Failure cases include:
Trying to get the frame pixels without having requested them first
Attempting to use this feature with a backend that doesn't support it yet.
Using it with a surface format that isn't covered by out cases of how to convert to rgba.
A failure during the callback of wgpu::BufferSlice::map_async.

emilk

This is a very nice feature to have!

I suggest looking at examples/screenshot/src/main.rs for how to implement it for glow, and then you could use the new feature in that same example, to make it a lot simpler!

crates/egui-wgpu/src/winit.rs

emilk · 2023-02-08T07:21:25Z

crates/egui-wgpu/src/winit.rs

+        let to_rgba = match tex.format() {
+            wgpu::TextureFormat::Rgba8Unorm => [0, 1, 2, 3],
+            wgpu::TextureFormat::Bgra8Unorm => [2, 1, 0, 3],
+            _ => panic!("Video capture not supported for the used surface format"),


let's use tracing::error instead, and also include the unsupported format in the error message

emilk · 2023-02-08T07:22:28Z

crates/eframe/src/epi.rs

@@ -672,6 +674,28 @@ impl Frame {
        self.storage.as_deref()
    }

+    /// Request the current frame's pixel data. Needs to be retrieved by calling [`eframe::Frame::frame_pixels`]
+    /// during [`eframe::App::post_rendering`].
+    pub fn request_pixels(&mut self) {


Maybe this should be called something like take_screenshot instead?

emilk · 2023-02-08T07:23:23Z

crates/eframe/src/epi.rs

@@ -672,6 +674,28 @@ impl Frame {
        self.storage.as_deref()
    }

+    /// Request the current frame's pixel data. Needs to be retrieved by calling [`eframe::Frame::frame_pixels`]
+    /// during [`eframe::App::post_rendering`].


This feature would be a lot more useful if the user could call frame_pixels at any time. Basically request_pixels would set some values, and they would remain until the user retrieves them

With this setup, that behaviour can still be achieved by simply retrieving the frame's data and manually saving it to the apps state in whatever way the user sees fit. I think that by clearing the data out after each frame there we achieve a small amount of clarity as to where a frame originated from, as it can only be retrieved from the post_rendering stage that created it. That being said I'm not against just letting stick around on the eframe::Frame if you think its more ergonomic.

emilk · 2023-02-08T07:25:36Z

crates/eframe/src/epi.rs

+    /// Returns None if
+    ///     Called in [`eframe::App::update`]
+    ///     [`eframe::Frame::request_pixels`] wasn't called on this frame during [`eframe::App::update`]
+    ///     The rendering backend doesn't support this feature (yet). Currently only implemented for the wgpu backend.


Implementing it for glow shouldn't be much work at all. You can read from the framebuffer in OpenGL. See examples/screenshot/src/main.rs for an example!

emilk · 2023-02-08T07:26:58Z

crates/eframe/src/epi.rs

+    ///     [`eframe::Frame::request_pixels`] wasn't called on this frame during [`eframe::App::update`]
+    ///     The rendering backend doesn't support this feature (yet). Currently only implemented for the wgpu backend.
+    ///     Retrieving the data was unsuccesful in some way.
+    pub fn frame_pixels(&self) -> Option<Vec<u8>> {


This needs to document the format (premultiplied RGBA), or switch it to using egui::Color32.

Does the pixel data start from the top of the screen, or the bottom?

Great points! I ended up refactoring to emit an egui::ColorImage from both backends to avoid ambiguity

emilk · 2023-02-08T07:28:54Z

crates/eframe/src/epi.rs

+    /// Returns None if
+    ///     Called in [`eframe::App::update`]
+    ///     [`eframe::Frame::request_pixels`] wasn't called on this frame during [`eframe::App::update`]


You should use * when making lists. You can check how the docs look with cargo doc -p eframe --no-deps --open

amfaber · 2023-02-08T17:05:06Z

From the example I figured out how to implement it for glow, and refactored example to use the new feature. I ended up returning an egui::ColorImage from the method as well instead of Vec, as that seems a lot more ergonomic to me. Because of that I also added some utility functions to egui::ColorImage.
ColorImage::region takes an egui::Rect out of an existing image, which covers my use-case. The rest of the new methods make it easier to access the image's data in raw form in the interest of interoperating with other crates such as image, or doing additional rendering on the gpu.

I know that ColorImage::into_raw is a little spicy, since it's using unsafe to return the data as a Vec without a copy. In my mind it should be safe since the underlying data is a Vec which is essentially just a Vec<[u8; 4]>, meaning we don't have any issues with endianness since the order of RGBA is always well-defined. Admittedly I have only tested it on my own setup.
To me, there is a case for avoiding the copy as a lot of data is potentially passed through this pipeline when recording.
Let me know if you think it should be included and if it should be annotated as unsafe if it is. For now I've left it as safe as I believe it to be so.

emilk · 2023-02-08T17:27:45Z

crates/epaint/src/image.rs

+        let capacity = self.pixels.capacity() * ratio;
+        let ptr = self.pixels.as_mut_ptr() as *mut u8;
+        std::mem::forget(self.pixels);
+        unsafe { Vec::from_raw_parts(ptr, length, capacity) }


I believe this is unsound. From reading the bytemuck docs, you can only do this if the alignment of the source and target type is the same, which it isn't

I suggest removing this method. Users can to it themselves if they wish to.

Done. I assume the cast_slice's are still fine, as Color32 is annotated with bytemuck::Pod?

Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>

without bumping version numbe

Massaged to pass tests

amfaber · 2023-03-01T22:33:01Z

I finally figured out how to run the CI for my own fork so that I could fix the failing tests.
However, due to ebarnard/rust-plist@aa38213 updating their dependency on base64 before bumping their version number, we are now using two versions of base64 - 0.13.1 and 0.21.0. For now I've added an ignore flag for base64 to deny.toml, basically ignoring the problem. Would love some thoughts on how to resolve an issue like this.

Wumpf

checked over the wgpu code and looks pretty straight forward! :)
Was hoping for something that is fast enough for streaming out videos, but that's for another time then (see (unactionable) comments)

Wumpf · 2023-03-02T10:42:05Z

crates/egui-wgpu/src/winit.rs

+impl CaptureState {
+    fn new(device: &Arc<wgpu::Device>, surface_texture: &wgpu::Texture) -> Self {
+        let texture = device.create_texture(&wgpu::TextureDescriptor {
+            label: None,


Would be nice to put a label in here that describes what this texture is used for.

Wumpf · 2023-03-02T10:42:14Z

crates/egui-wgpu/src/winit.rs

+        let padding = BufferPadding::new(surface_texture.width());
+
+        let buffer = device.create_buffer(&wgpu::BufferDescriptor {
+            label: None,


same here, please add a label

Wumpf · 2023-03-02T10:44:20Z

crates/egui-wgpu/src/winit.rs

+        let bytes_per_pixel = std::mem::size_of::<u32>() as u32;
+        let unpadded_bytes_per_row = width * bytes_per_pixel;
+        let align = wgpu::COPY_BYTES_PER_ROW_ALIGNMENT;
+        let padded_bytes_per_row_padding = (align - unpadded_bytes_per_row % align) % align;


wgpu has wgpu::util::align_to for this which is a bit nicer to read :)

Wumpf · 2023-03-02T10:53:08Z

crates/egui-wgpu/src/winit.rs

+        buffer_slice.map_async(wgpu::MapMode::Read, move |v| {
+            drop(sender.send(v));
+        });
+        device.poll(wgpu::Maintain::WaitForSubmissionIndex(id));


I suppose this is out of scope of this PR, but it would be nice to make this non-blocking.
That would ofc also mean we need to have several buffers etc., gets more complex quickly

Wumpf · 2023-03-02T10:56:12Z

crates/egui-wgpu/src/winit.rs

+        let to_rgba = match tex.format() {
+            wgpu::TextureFormat::Rgba8Unorm => [0, 1, 2, 3],
+            wgpu::TextureFormat::Bgra8Unorm => [2, 1, 0, 3],
+            _ => {
+                tracing::error!("Screen can't be captured unless the surface format is Rgba8Unorm or Bgra8Unorm. Current surface format is {:?}", tex.format());
+                return None;
+            }
+        };


I was convinced copy_texture_to_texture could do this conversion for us, but looking it up that's wrong :(. Which makes putting this conversion on gpu again out of scope of this PR 😢

emilk · 2023-03-02T15:03:16Z

I finally figured out how to run the CI for my own fork so that I could fix the failing tests. However, due to ebarnard/rust-plist@aa38213 updating their dependency on base64 before bumping their version number, we are now using two versions of base64 - 0.13.1 and 0.21.0. For now I've added an ignore flag for base64 to deny.toml, basically ignoring the problem. Would love some thoughts on how to resolve an issue like this.

Why update Cargo.lock at all in this PR?

git checkout main Cargo.lock deny.toml
cargo check
git add -u
git commit -m 'Revert changes to Cargo.lock and deny.toml`

Pushing upstream

amfaber · 2023-03-02T19:50:00Z

git checkout main Cargo.lock deny.toml
cargo check
git add -u
git commit -m 'Revert changes to Cargo.lock and deny.toml`

Thanks! That was just what I needed :)

Why update Cargo.lock at all in this PR?

It wasn't intentional. I feel like something broke when integrating a bunch of new commits from the master to my fork a few weeks ago and I ended up with some merge conflicts in the Cargo.toml somehow. At the time I think I tried to revert it, but it didn't work out for whatever reason. Then I naively thought "its autogenerated from the Cargo.toml anyways, so I could just delete it and let cargo handle it for me". I now see the error of those ways. Simply reverting just worked now, so I must have messed something up when I tried it initially

hacknus · 2023-03-25T08:59:09Z

any updates on when this can be merged?

crates/eframe/src/epi.rs

emilk · 2023-03-29T08:56:54Z

crates/egui-wgpu/CHANGELOG.md

@@ -11,7 +11,7 @@ All notable changes to the `egui-wgpu` integration will be noted in this file.
 * `winit::Painter::set_window` is now `async` ([#2434](https://github.com/emilk/egui/pull/2434)).
 * `egui-wgpu` now only depends on `epaint` instead of the entire `egui` ([#2438](https://github.com/emilk/egui/pull/2438)).
 * `winit::Painter` now supports transparent backbuffer ([#2684](https://github.com/emilk/egui/pull/2684)).
-
+* Add `read_screan_rgba` to the egui-wgpu `Painter`, to allow for capturing the current frame when using wgpu. Used in conjuction with `Frame::request_screenshot`. ([#2676](https://github.com/emilk/egui/pull/2676))


need to move up to the # Unreleased section

emilk · 2023-03-29T08:57:54Z

crates/egui-wgpu/src/winit.rs

        let render_state = match self.render_state.as_mut() {
            Some(rs) => rs,
-            None => return,
+            None => return None,
        };


let render_state = self.render_state.as_mut()?; here and below

Another round of feedback

amfaber · 2023-03-29T14:11:10Z

The changelog changes have been moved to unreleased, and the ? is now employed, as it should it be

emilk

Thanks!

amfaber · 2023-03-29T15:31:11Z

Thanks for the help with my first open source contribution, I appreciate it :)

* implement save_plot * fix for check.sh * clippy * add save_plot to Cargo.lock * adapted for PR #2676 (removes unsafe code) * add some comments * implemented the comments from emilk * update comments in code * rustfmt * remove picked_path * add more comments * removed unused import * use `inner.response.rect` as the plot position * remove plot_location from MyApp members * sort entries * Update examples/save_plot/src/main.rs Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com> * use env_logger instead of tracing subscriber * use env_logger instead of tracing subscriber and combine if let --------- Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>

amfaber added 13 commits February 4, 2023 12:22

first stab at frame capture

c533c46

Merge branch 'master' of https://github.com/emilk/egui

318b1d5

capture with indirection

dd6f898

A bit of cleanup

447b68f

Merge branch 'master' of https://github.com/emilk/egui into emilk-master

1f1e58f

Merge branch 'emilk-master'

6b7bf39

some cleaning

7e2232e

trying to fix a git mess

fb55318

rustfmt

387d81a

rebase in a moment anyways

78650a2

Fix import of cell for single use

2dc00e7

Add notes in changelog

e1aa669

Merge https://github.com/emilk/egui

ae5d24e

emilk reviewed Feb 8, 2023

View reviewed changes

amfaber and others added 10 commits February 8, 2023 09:23

Merge branch 'emilk:master' into master

a3b253b

refactoring of screenshot example WIP

94f3a84

Explicit width and height for region method of colorimage

9cf3b52

Some documentation and cargo fmt

094f284

Final touches

bf6e33a

Guard new feature behind #[cfg(not(target_arch = "wasm32"))].

52f886a

A bit more docs cleanup

ff32cd8

Merge https://github.com/emilk/egui

06e6dc0

cargo.lock troubles

570e941

rename pixel_data to screenshot

204a450

amfaber marked this pull request as ready for review February 8, 2023 17:06

emilk reviewed Feb 8, 2023

View reviewed changes

amfaber and others added 3 commits February 8, 2023 19:15

Update crates/egui-wgpu/src/winit.rs

214146e

Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>

Remove into_raw unsafe method

391629a

Merge https://github.com/emilk/egui

b333b56

amfaber and others added 6 commits March 1, 2023 21:10

another cargo fmt ._.

5fa9ced

Delete and regenerate Cargo.lock for cargo deny

f239e7a

egui -> emath in docs

4a1c38d

Allow two versions of base64 since plink updated their dependency

c154774

without bumping version numbe

Fixing some cranky warnings

f437c12

Merge pull request #2 from amfaber/WIP

00c63d4

Massaged to pass tests

Wumpf approved these changes Mar 2, 2023

View reviewed changes

amfaber and others added 4 commits March 2, 2023 18:34

Revert changes to Cargo.lock and deny.toml

78de848

Labels for capture texture and buffer, wgpu::util::align_to for aligning

516f8c6

Git actions cargo fmt is stricter than my cargo fmt

4d638b1

Merge pull request #3 from amfaber/WIP

c62aa2b

Pushing upstream

emilk reviewed Mar 29, 2023

View reviewed changes

crates/eframe/src/epi.rs Outdated Show resolved Hide resolved

storage for wasm too

e3d937e

emilk changed the title ~~Screen capturing with eframe and wgpu, extensible to other backends~~ eframe: capture a screenshot using Frame::request_screenshot Mar 29, 2023

emilk reviewed Mar 29, 2023

View reviewed changes

amfaber and others added 4 commits March 29, 2023 13:15

Move changes to unreleased

bf08887

? operator for getting (render|surface)state

08ea093

Merge branch 'emilk:master' into WIP

10e072c

Merge pull request #4 from amfaber/WIP

19f7577

Another round of feedback

emilk approved these changes Mar 29, 2023

View reviewed changes

emilk merged commit 870264b into emilk:master Mar 29, 2023

hacknus added a commit to hacknus/egui that referenced this pull request Mar 29, 2023

adapted for PR emilk#2676 (removes unsafe code)

b97fcc3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

eframe: capture a screenshot using `Frame::request_screenshot` #2676

eframe: capture a screenshot using `Frame::request_screenshot` #2676

amfaber commented Feb 5, 2023

emilk left a comment

emilk Feb 8, 2023

emilk Feb 8, 2023

emilk Feb 8, 2023

amfaber Feb 8, 2023

emilk Feb 8, 2023

emilk Feb 8, 2023

amfaber Feb 8, 2023

emilk Feb 8, 2023

amfaber commented Feb 8, 2023

emilk Feb 8, 2023

amfaber Feb 8, 2023

amfaber commented Mar 1, 2023

Wumpf left a comment

Wumpf Mar 2, 2023

Wumpf Mar 2, 2023

Wumpf Mar 2, 2023

Wumpf Mar 2, 2023

Wumpf Mar 2, 2023

emilk commented Mar 2, 2023

amfaber commented Mar 2, 2023

hacknus commented Mar 25, 2023

emilk Mar 29, 2023

emilk Mar 29, 2023

amfaber commented Mar 29, 2023

emilk left a comment

amfaber commented Mar 29, 2023

eframe: capture a screenshot using Frame::request_screenshot #2676

eframe: capture a screenshot using Frame::request_screenshot #2676

Conversation

amfaber commented Feb 5, 2023

Description of the changes

Concerns

emilk left a 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

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

amfaber commented Feb 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amfaber commented Mar 1, 2023

Wumpf left a 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

Choose a reason for hiding this comment

emilk commented Mar 2, 2023

amfaber commented Mar 2, 2023

hacknus commented Mar 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amfaber commented Mar 29, 2023

emilk left a comment

Choose a reason for hiding this comment

amfaber commented Mar 29, 2023

eframe: capture a screenshot using `Frame::request_screenshot` #2676

eframe: capture a screenshot using `Frame::request_screenshot` #2676