From e35d05f6b99baa7918f7a61dbe9a6b7b204a4dca Mon Sep 17 00:00:00 2001 From: Simon Hausmann Date: Fri, 5 Jul 2024 16:24:45 +0200 Subject: [PATCH] API Review: Remove SharedImageBuffer from public API and rename Window::grab_window() to take_snapshot() Use SharedPixelBuffer as return value for take_snapshot() and provide counter-parts to from_rgb* in Image as to_rgb* --- CHANGELOG.md | 2 +- api/rs/slint/lib.rs | 3 +- internal/backends/qt/qt_window.rs | 15 +-- internal/backends/testing/slint_systest.proto | 8 +- internal/backends/testing/systest.rs | 32 ++++--- internal/core/api.rs | 8 +- internal/core/graphics/image.rs | 94 +++++++++++++++++-- internal/core/renderer.rs | 8 +- internal/core/software_renderer.rs | 19 +++- internal/renderers/femtovg/lib.rs | 8 +- internal/renderers/skia/lib.rs | 8 +- 11 files changed, 149 insertions(+), 56 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a347aff8852..b668427ae2c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,7 +53,7 @@ All notable changes to this project are documented in this file. - allow all clippy warnings in generated code - Add `slint::Image::image_buffer()` getter to obtain pixels for a `slint::Image` if available. - Fix panic in `slint::Timer` when a new timer is started while stopping another. - - Added `slint::Window::grab_window()` + - Added `slint::Window::take_snapshot()` ### Interpreter - Track model lenght change when accessing a model out of bounds diff --git a/api/rs/slint/lib.rs b/api/rs/slint/lib.rs index ec0f707c17d..2bc86adc489 100644 --- a/api/rs/slint/lib.rs +++ b/api/rs/slint/lib.rs @@ -210,8 +210,7 @@ pub use i_slint_core::component_factory::ComponentFactory; pub use i_slint_core::graphics::{BorrowedOpenGLTextureBuilder, BorrowedOpenGLTextureOrigin}; // keep in sync with internal/interpreter/api.rs pub use i_slint_core::graphics::{ - Brush, Color, Image, LoadImageError, Rgb8Pixel, Rgba8Pixel, RgbaColor, SharedImageBuffer, - SharedPixelBuffer, + Brush, Color, Image, LoadImageError, Rgb8Pixel, Rgba8Pixel, RgbaColor, SharedPixelBuffer, }; pub use i_slint_core::model::{ FilterModel, MapModel, Model, ModelExt, ModelNotify, ModelPeer, ModelRc, ModelTracker, diff --git a/internal/backends/qt/qt_window.rs b/internal/backends/qt/qt_window.rs index c10fcf51c52..9ef61fbc8d1 100644 --- a/internal/backends/qt/qt_window.rs +++ b/internal/backends/qt/qt_window.rs @@ -8,7 +8,8 @@ use i_slint_core::graphics::rendering_metrics_collector::{ RenderingMetrics, RenderingMetricsCollector, }; use i_slint_core::graphics::{ - euclid, Brush, Color, FontRequest, IntRect, Point, SharedImageBuffer, + euclid, Brush, Color, FontRequest, IntRect, Point, Rgba8Pixel, SharedImageBuffer, + SharedPixelBuffer, }; use i_slint_core::input::{KeyEvent, KeyEventType, MouseEvent}; use i_slint_core::item_rendering::{ @@ -2324,26 +2325,26 @@ impl i_slint_core::renderer::RendererSealed for QtWindow { // No-op because QtWindow is also the WindowAdapter } - fn screenshot(&self) -> Result { + fn take_snapshot(&self) -> Result, PlatformError> { let widget_ptr = self.widget_ptr(); let size = cpp! {unsafe [widget_ptr as "QWidget*"] -> qttypes::QSize as "QSize" { return widget_ptr->size(); }}; - let rgb8_data = cpp! {unsafe [widget_ptr as "QWidget*"] -> qttypes::QByteArray as "QByteArray" { + let rgba8_data = cpp! {unsafe [widget_ptr as "QWidget*"] -> qttypes::QByteArray as "QByteArray" { QPixmap pixmap = widget_ptr->grab(); QImage image = pixmap.toImage(); - image.convertTo(QImage::Format_RGB888); + image.convertTo(QImage::Format_ARGB32); return QByteArray(reinterpret_cast(image.constBits()), image.sizeInBytes()); }}; - let buffer = i_slint_core::graphics::SharedPixelBuffer::::clone_from_slice( - rgb8_data.to_slice(), + let buffer = i_slint_core::graphics::SharedPixelBuffer::::clone_from_slice( + rgba8_data.to_slice(), size.width, size.height, ); - Ok(i_slint_core::graphics::SharedImageBuffer::RGB8(buffer)) + Ok(buffer) } } diff --git a/internal/backends/testing/slint_systest.proto b/internal/backends/testing/slint_systest.proto index 4870c5c46f9..b26f9b7268c 100644 --- a/internal/backends/testing/slint_systest.proto +++ b/internal/backends/testing/slint_systest.proto @@ -57,7 +57,7 @@ message RequestSetElementAccessibleValue { string value = 2; } -message RequestGrabWindow { +message RequestTakeSnapshot { Handle window_handle = 1; } @@ -75,7 +75,7 @@ message RequestToAUT { RequestElementProperties request_element_properties = 4; RequestInvokeElementAccessibilityAction request_invoke_element_accessibility_action = 5; RequestSetElementAccessibleValue request_set_element_accessible_value = 6; - RequestGrabWindow request_grab_window = 7; + RequestTakeSnapshot request_take_snapshot = 7; RequestElementClick request_element_click = 8; } } @@ -169,7 +169,7 @@ message InvokeElementAccessibilityActionResponse { message SetElementAccessibleValueResponse { } -message GrabWindowResponse { +message TakeSnapshotResponse { bytes window_contents_as_png = 1; } @@ -185,7 +185,7 @@ message AUTResponse { ElementPropertiesResponse element_properties = 5; InvokeElementAccessibilityActionResponse invoke_element_accessibility_action_response = 6; SetElementAccessibleValueResponse set_element_accessible_value_response = 7; - GrabWindowResponse grab_window_response = 8; + TakeSnapshotResponse take_snapshot_response = 8; ElementClickResponse element_click_response = 9; } } diff --git a/internal/backends/testing/systest.rs b/internal/backends/testing/systest.rs index e92380107d8..431a2dd3a73 100644 --- a/internal/backends/testing/systest.rs +++ b/internal/backends/testing/systest.rs @@ -6,7 +6,6 @@ use futures_lite::AsyncReadExt; use futures_lite::AsyncWriteExt; use i_slint_core::api::EventLoopError; use i_slint_core::debug_log; -use i_slint_core::graphics::SharedImageBuffer; use i_slint_core::item_tree::ItemTreeRc; use i_slint_core::window::WindowAdapter; use i_slint_core::window::WindowInner; @@ -132,10 +131,10 @@ impl TestingClient { proto::SetElementAccessibleValueResponse {}, ) } - proto::mod_RequestToAUT::OneOfmsg::request_grab_window(proto::RequestGrabWindow { - window_handle, - }) => proto::mod_AUTResponse::OneOfmsg::grab_window_response( - self.grab_window(handle_to_index( + proto::mod_RequestToAUT::OneOfmsg::request_take_snapshot( + proto::RequestTakeSnapshot { window_handle }, + ) => proto::mod_AUTResponse::OneOfmsg::take_snapshot_response( + self.take_snapshot(handle_to_index( window_handle .ok_or_else(|| "grab window request missing window handle".to_string())?, ))?, @@ -172,25 +171,30 @@ impl TestingClient { }) } - fn grab_window( + fn take_snapshot( &self, window_index: generational_arena::Index, - ) -> Result { + ) -> Result { use image::ImageEncoder; let adapter = self.window_adapter(window_index)?; let window = adapter.window(); let buffer = - window.grab_window().map_err(|e| format!("Error grabbing window screenshot: {e}"))?; + window.take_snapshot().map_err(|e| format!("Error grabbing window screenshot: {e}"))?; let mut window_contents_as_png: Vec = Vec::new(); let cursor = std::io::Cursor::new(&mut window_contents_as_png); let encoder = image::codecs::png::PngEncoder::new(cursor); - match buffer { - SharedImageBuffer::RGB8(buffer) => encoder.write_image(buffer.as_bytes(), buffer.width(), buffer.height(), image::ColorType::Rgb8), - SharedImageBuffer::RGBA8(buffer) => encoder.write_image(buffer.as_bytes(), buffer.width(), buffer.height(), image::ColorType::Rgba8), - SharedImageBuffer::RGBA8Premultiplied(_) => return Err("window screenshot produced screenshot with pre-multiplied alpha; this is currently not supported".into()), - }.map_err(|encode_err| format!("error encoding png image after screenshot: {encode_err}"))?; - Ok(proto::GrabWindowResponse { window_contents_as_png }) + encoder + .write_image( + buffer.as_bytes(), + buffer.width(), + buffer.height(), + image::ColorType::Rgba8, + ) + .map_err(|encode_err| { + format!("error encoding png image after screenshot: {encode_err}") + })?; + Ok(proto::TakeSnapshotResponse { window_contents_as_png }) } fn find_elements_by_id( diff --git a/internal/core/api.rs b/internal/core/api.rs index ab1e2d45059..7db05cc9abc 100644 --- a/internal/core/api.rs +++ b/internal/core/api.rs @@ -9,7 +9,7 @@ This module contains types that are public and re-exported in the slint-rs as we #[cfg(target_has_atomic = "ptr")] pub use crate::future::*; -use crate::graphics::SharedImageBuffer; +use crate::graphics::{Rgba8Pixel, SharedPixelBuffer}; use crate::input::{KeyEventType, MouseEvent}; use crate::item_tree::ItemTreeVTable; use crate::window::{WindowAdapter, WindowInner}; @@ -656,12 +656,12 @@ impl Window { } } - /// Returns an image buffer with the rendered contents. + /// Takes a snapshot of the window contents and returns it as RGBA8 encoded pixel buffer. /// /// Note that this function may be slow to call. Reading from the framebuffer previously /// rendered, too, may take a long time. - pub fn grab_window(&self) -> Result { - self.0.window_adapter().renderer().screenshot() + pub fn take_snapshot(&self) -> Result, PlatformError> { + self.0.window_adapter().renderer().take_snapshot() } } diff --git a/internal/core/graphics/image.rs b/internal/core/graphics/image.rs index fdce22e7688..21e64ca0f7a 100644 --- a/internal/core/graphics/image.rs +++ b/internal/core/graphics/image.rs @@ -667,6 +667,86 @@ impl Image { }) } + /// Returns the pixel buffer for the Image if available in RGB format without alpha. + /// Returns None if the pixels cannot be obtained, for example when the image was created from borrowed OpenGL textures. + pub fn to_rgb8(&self) -> Option> { + self.0.render_to_buffer(None).and_then(|image| match image { + SharedImageBuffer::RGB8(buffer) => Some(buffer), + _ => None, + }) + } + + /// Returns the pixel buffer for the Image if available in RGBA format. + /// Returns None if the pixels cannot be obtained, for example when the image was created from borrowed OpenGL textures. + pub fn to_rgba8(&self) -> Option> { + self.0.render_to_buffer(None).and_then(|image| match image { + SharedImageBuffer::RGB8(buffer) => Some(SharedPixelBuffer:: { + width: buffer.width, + height: buffer.height, + data: buffer.data.into_iter().map(Into::into).collect(), + }), + SharedImageBuffer::RGBA8(buffer) => Some(buffer), + SharedImageBuffer::RGBA8Premultiplied(buffer) => { + Some(SharedPixelBuffer:: { + width: buffer.width, + height: buffer.height, + data: buffer + .data + .into_iter() + .map(|rgba_premul| { + if rgba_premul.a == 0 { + Rgba8Pixel::new(0, 0, 0, 0) + } else { + let af = rgba_premul.a as f32 / 255.0; + Rgba8Pixel { + r: (rgba_premul.r as f32 * 255. / af) as u8, + g: (rgba_premul.g as f32 * 255. / af) as u8, + b: (rgba_premul.b as f32 * 255. / af) as u8, + a: rgba_premul.a, + } + } + }) + .collect(), + }) + } + }) + } + + /// Returns the pixel buffer for the Image if available in RGBA format, with the alpha channel pre-multiplied + /// to the red, green, and blue channels. + /// Returns None if the pixels cannot be obtained, for example when the image was created from borrowed OpenGL textures. + pub fn to_rgba8_premultiplied(&self) -> Option> { + self.0.render_to_buffer(None).and_then(|image| match image { + SharedImageBuffer::RGB8(buffer) => Some(SharedPixelBuffer:: { + width: buffer.width, + height: buffer.height, + data: buffer.data.into_iter().map(Into::into).collect(), + }), + SharedImageBuffer::RGBA8(buffer) => Some(SharedPixelBuffer:: { + width: buffer.width, + height: buffer.height, + data: buffer + .data + .into_iter() + .map(|rgba| { + if rgba.a == 255 { + rgba + } else { + let af = rgba.a as f32 / 255.0; + Rgba8Pixel { + r: (rgba.r as f32 * af / 255.) as u8, + g: (rgba.g as f32 * af / 255.) as u8, + b: (rgba.b as f32 * af / 255.) as u8, + a: rgba.a, + } + } + }) + .collect(), + }), + SharedImageBuffer::RGBA8Premultiplied(buffer) => Some(buffer), + }) + } + /// Creates a new Image from an existing OpenGL texture. The texture remains borrowed by Slint /// for the duration of being used for rendering, such as when assigned as source property to /// an `Image` element. It's the application's responsibility to delete the texture when it is @@ -757,12 +837,6 @@ impl Image { _ => None, } } - - /// Returns the pixel buffer for the Image, if available. Returns None if the pixels cannot - /// be obtained, for example when the image was created from borrowed OpenGL textures. - pub fn image_buffer(&self) -> Option { - self.0.render_to_buffer(None) - } } /// This enum describes the origin to use when rendering a borrowed OpenGL texture. @@ -848,13 +922,15 @@ pub fn load_image_from_embedded_data(data: Slice<'static, u8>, format: Slice<'_, fn test_image_size_from_buffer_without_backend() { { assert_eq!(Image::default().size(), Default::default()); - assert!(Image::default().image_buffer().is_none()); + assert!(Image::default().to_rgb8().is_none()); + assert!(Image::default().to_rgba8().is_none()); + assert!(Image::default().to_rgba8_premultiplied().is_none()); } { let buffer = SharedPixelBuffer::::new(320, 200); let image = Image::from_rgb8(buffer.clone()); assert_eq!(image.size(), [320, 200].into()); - assert_eq!(image.image_buffer(), Some(SharedImageBuffer::RGB8(buffer))); + assert_eq!(image.to_rgb8().as_ref().map(|b| b.as_slice()), Some(buffer.as_slice())); } } @@ -864,7 +940,7 @@ fn test_image_size_from_svg() { let simple_svg = r#""#; let image = Image::load_from_svg_data(simple_svg.as_bytes()).unwrap(); assert_eq!(image.size(), [320, 200].into()); - assert_eq!(image.image_buffer().unwrap().size(), image.size()); + assert_eq!(image.to_rgba8().unwrap().size(), image.size()); } #[cfg(feature = "svg")] diff --git a/internal/core/renderer.rs b/internal/core/renderer.rs index a406ffd2887..0a41fa51a28 100644 --- a/internal/core/renderer.rs +++ b/internal/core/renderer.rs @@ -7,7 +7,7 @@ use alloc::rc::Rc; use core::pin::Pin; use crate::api::PlatformError; -use crate::graphics::SharedImageBuffer; +use crate::graphics::{Rgba8Pixel, SharedPixelBuffer}; use crate::item_tree::ItemTreeRef; use crate::items::TextWrap; use crate::lengths::{LogicalLength, LogicalPoint, LogicalRect, LogicalSize, ScaleFactor}; @@ -118,9 +118,9 @@ pub trait RendererSealed { Ok(()) } - /// Re-implement this function to support Window::grab_window(), i.e. return + /// Re-implement this function to support Window::take_snapshot(), i.e. return /// the contents of the window in an image buffer. - fn screenshot(&self) -> Result { - Err("WindowAdapter::grab_window is not implemented by the platform".into()) + fn take_snapshot(&self) -> Result, PlatformError> { + Err("WindowAdapter::take_snapshot is not implemented by the platform".into()) } } diff --git a/internal/core/software_renderer.rs b/internal/core/software_renderer.rs index 7889d6de2ac..175fdfdbf7c 100644 --- a/internal/core/software_renderer.rs +++ b/internal/core/software_renderer.rs @@ -14,7 +14,9 @@ mod fonts; use self::fonts::GlyphRenderer; use crate::api::{PlatformError, Window}; use crate::graphics::rendering_metrics_collector::{RefreshMode, RenderingMetricsCollector}; -use crate::graphics::{BorderRadius, PixelFormat, SharedImageBuffer, SharedPixelBuffer}; +use crate::graphics::{ + BorderRadius, PixelFormat, Rgba8Pixel, SharedImageBuffer, SharedPixelBuffer, +}; use crate::item_rendering::{CachedRenderingData, DirtyRegion, RenderBorderRectangle, RenderImage}; use crate::items::{ItemRc, TextOverflow, TextWrap}; use crate::lengths::{ @@ -792,7 +794,7 @@ impl RendererSealed for SoftwareRenderer { self.partial_cache.borrow_mut().clear(); } - fn screenshot(&self) -> Result { + fn take_snapshot(&self) -> Result, PlatformError> { let Some(window_adapter) = self.maybe_window_adapter.borrow().as_ref().and_then(|w| w.upgrade()) else { @@ -807,7 +809,7 @@ impl RendererSealed for SoftwareRenderer { let Some((width, height)) = size.width.try_into().ok().zip(size.height.try_into().ok()) else { // Nothing to render - return Err("grab_window() called on window with invalid size".into()); + return Err("take_snapshot() called on window with invalid size".into()); }; let mut target_buffer = SharedPixelBuffer::::new(width, height); @@ -817,7 +819,16 @@ impl RendererSealed for SoftwareRenderer { // ensure that caches are clear for the next call self.set_repaint_buffer_type(RepaintBufferType::NewBuffer); - Ok(SharedImageBuffer::RGB8(target_buffer)) + let mut target_buffer_with_alpha = + SharedPixelBuffer::::new(target_buffer.width(), target_buffer.height()); + for (target_pixel, source_pixel) in target_buffer_with_alpha + .make_mut_slice() + .iter_mut() + .zip(target_buffer.as_slice().iter()) + { + *target_pixel.rgb_mut() = *source_pixel; + } + Ok(target_buffer_with_alpha) } } diff --git a/internal/renderers/femtovg/lib.rs b/internal/renderers/femtovg/lib.rs index 2c1acbb0289..6b9621aac7d 100644 --- a/internal/renderers/femtovg/lib.rs +++ b/internal/renderers/femtovg/lib.rs @@ -12,7 +12,7 @@ use std::rc::{Rc, Weak}; use i_slint_common::sharedfontdb; use i_slint_core::api::{RenderingNotifier, RenderingState, SetRenderingNotifierError}; use i_slint_core::graphics::{euclid, rendering_metrics_collector::RenderingMetricsCollector}; -use i_slint_core::graphics::{BorderRadius, SharedImageBuffer}; +use i_slint_core::graphics::{BorderRadius, Rgba8Pixel}; use i_slint_core::graphics::{FontRequest, SharedPixelBuffer}; use i_slint_core::item_rendering::ItemRenderer; use i_slint_core::items::TextWrap; @@ -634,7 +634,7 @@ impl RendererSealed for FemtoVGRenderer { } /// Returns an image buffer of what was rendered last by reading the previous front buffer (using glReadPixels). - fn screenshot(&self) -> Result { + fn take_snapshot(&self) -> Result, PlatformError> { self.opengl_context.borrow().ensure_current()?; let Some(canvas) = self.canvas.borrow().as_ref().cloned() else { return Err("FemtoVG renderer cannot take screenshot without a window".into()); @@ -645,11 +645,11 @@ impl RendererSealed for FemtoVGRenderer { .map_err(|e| format!("FemtoVG error reading current back buffer: {e}"))?; use rgb::ComponentBytes; - Ok(SharedImageBuffer::RGBA8(SharedPixelBuffer::clone_from_slice( + Ok(SharedPixelBuffer::clone_from_slice( screenshot.buf().as_bytes(), screenshot.width() as u32, screenshot.height() as u32, - ))) + )) } } diff --git a/internal/renderers/skia/lib.rs b/internal/renderers/skia/lib.rs index 118b8cdb92a..0ebee162d53 100644 --- a/internal/renderers/skia/lib.rs +++ b/internal/renderers/skia/lib.rs @@ -13,7 +13,7 @@ use i_slint_core::api::{ }; use i_slint_core::graphics::euclid::{self, Vector2D}; use i_slint_core::graphics::rendering_metrics_collector::RenderingMetricsCollector; -use i_slint_core::graphics::{BorderRadius, FontRequest, SharedImageBuffer, SharedPixelBuffer}; +use i_slint_core::graphics::{BorderRadius, FontRequest, SharedPixelBuffer}; use i_slint_core::item_rendering::{ItemCache, ItemRenderer}; use i_slint_core::lengths::{ LogicalLength, LogicalPoint, LogicalRect, LogicalSize, PhysicalPx, ScaleFactor, @@ -580,7 +580,9 @@ impl i_slint_core::renderer::RendererSealed for SkiaRenderer { } /// Returns an image buffer of what was rendered last by reading the previous front buffer (using glReadPixels). - fn screenshot(&self) -> Result { + fn take_snapshot( + &self, + ) -> Result, PlatformError> { let window_adapter = self.window_adapter()?; let window = window_adapter.window(); let size = window_adapter.window().size(); @@ -603,7 +605,7 @@ impl i_slint_core::renderer::RendererSealed for SkiaRenderer { self.render_to_canvas(surface_borrow.canvas(), 0., (0.0, 0.0), None, None, window, None); - Ok(SharedImageBuffer::RGBA8(target_buffer)) + Ok(target_buffer) } }