-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
eframe: capture a screenshot using Frame::request_screenshot
#2676
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
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"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's use tracing::error
instead, and also include the unsupported format in the error message
crates/eframe/src/epi.rs
Outdated
@@ -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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should be called something like take_screenshot
instead?
crates/eframe/src/epi.rs
Outdated
@@ -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`]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
crates/eframe/src/epi.rs
Outdated
/// 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
crates/eframe/src/epi.rs
Outdated
/// [`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>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great points! I ended up refactoring to emit an egui::ColorImage from both backends to avoid ambiguity
crates/eframe/src/epi.rs
Outdated
/// Returns None if | ||
/// Called in [`eframe::App::update`] | ||
/// [`eframe::Frame::request_pixels`] wasn't called on this frame during [`eframe::App::update`] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should use *
when making lists. You can check how the docs look with cargo doc -p eframe --no-deps --open
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. 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. |
crates/epaint/src/image.rs
Outdated
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) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I assume the cast_slice's are still fine, as Color32 is annotated with bytemuck::Pod?
Co-authored-by: Emil Ernerfeldt <[email protected]>
without bumping version numbe
Massaged to pass tests
I finally figured out how to run the CI for my own fork so that I could fix the failing tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
crates/egui-wgpu/src/winit.rs
Outdated
impl CaptureState { | ||
fn new(device: &Arc<wgpu::Device>, surface_texture: &wgpu::Texture) -> Self { | ||
let texture = device.create_texture(&wgpu::TextureDescriptor { | ||
label: None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to put a label in here that describes what this texture is used for.
crates/egui-wgpu/src/winit.rs
Outdated
let padding = BufferPadding::new(surface_texture.width()); | ||
|
||
let buffer = device.create_buffer(&wgpu::BufferDescriptor { | ||
label: None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, please add a label
crates/egui-wgpu/src/winit.rs
Outdated
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wgpu has wgpu::util::align_to
for this which is a bit nicer to read :)
buffer_slice.map_async(wgpu::MapMode::Read, move |v| { | ||
drop(sender.send(v)); | ||
}); | ||
device.poll(wgpu::Maintain::WaitForSubmissionIndex(id)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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; | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😢
Why update 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 :)
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 |
any updates on when this can be merged? |
Frame::request_screenshot
crates/egui-wgpu/CHANGELOG.md
Outdated
@@ -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)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to move up to the # Unreleased
section
crates/egui-wgpu/src/winit.rs
Outdated
let render_state = match self.render_state.as_mut() { | ||
Some(rs) => rs, | ||
None => return, | ||
None => return None, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let render_state = self.render_state.as_mut()?;
here and below
The changelog changes have been moved to unreleased, and the ? is now employed, as it should it be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
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 <[email protected]> * use env_logger instead of tracing subscriber * use env_logger instead of tracing subscriber and combine if let --------- Co-authored-by: Emil Ernerfeldt <[email protected]>
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.
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" toFrame
, in order to be able to pass the data back to the app duringApp::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 awgpu::Buffer
which iswgpu::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 eitherwgpu::TextureFormat::Rgba8Unorm
orwgpu::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'spost_rendering
, while allowing the app to take the generated screenshot fromFrame
via compile-time interior mutability withstd::cell::Cell
.One thing that bugs me is that there is now another flag on
AppOutput
that isn't handled by eitherhandle_app_output
orhandle_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 thatwgpu::TextureUsages::COPY_SRC
is not an allowed usage for the surface texture, necessitating that we instead make the surfacewgpu::TextureUsages::COPY_DST
and render to a separate texture which iswgpu::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 withPainter::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
.