-
Notifications
You must be signed in to change notification settings - Fork 220
perf(web): replace softbuffer with direct put_image_data canvas present #1374
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
Changes from 5 commits
5be56f9
45fb1a6
67d73ff
6fc670e
13b7b9a
eb072cc
ad0dc5d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,93 +1,99 @@ | ||
| use core::num::NonZeroU32; | ||
|
|
||
| use anyhow::Context as _; | ||
| use ironrdp::pdu::geometry::{InclusiveRectangle, Rectangle as _}; | ||
| use softbuffer::{NoDisplayHandle, NoWindowHandle}; | ||
| use web_sys::HtmlCanvasElement; | ||
|
|
||
| #[cfg(target_arch = "wasm32")] | ||
| use anyhow::anyhow; | ||
| use ironrdp::pdu::geometry::InclusiveRectangle; | ||
| #[cfg(target_arch = "wasm32")] | ||
| use ironrdp::pdu::geometry::Rectangle as _; | ||
| #[cfg(target_arch = "wasm32")] | ||
| use wasm_bindgen::{Clamped, JsCast as _}; | ||
| #[cfg(target_arch = "wasm32")] | ||
| use web_sys::ImageData; | ||
| use web_sys::{CanvasRenderingContext2d, HtmlCanvasElement}; | ||
|
|
||
| /// Web render surface. Owns the canvas's 2D context; each dirty region is blitted directly with | ||
| /// `put_image_data` at the region's origin, after forcing its alpha opaque in place. The region | ||
| /// buffer is the caller's throwaway copy, so no scratch buffer is kept here. | ||
| /// | ||
| /// This replaced a softbuffer-backed path that converted RGBA -> u32 `0RGB` (our pass) and then let | ||
| /// softbuffer repack u32 -> RGBA per frame into a freshly allocated buffer — two pixel passes over | ||
| /// the whole surface plus a per-frame allocation. The direct path drops the u32 round-trip and the | ||
| /// per-frame allocation, measuring an order of magnitude faster present at 4K with byte-identical | ||
| /// canvas output. Mirrors the same fix in IronVNC. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Keeping some historical documentation on how we ended up converging may be okay in order to justify the current design, but it currently sounds like a "prompt artifact": "Mirrors the same fix in IronVNC" is completely irrelevant outside of this PR. |
||
| pub(crate) struct Canvas { | ||
| width: NonZeroU32, | ||
| surface: softbuffer::Surface<NoDisplayHandle, NoWindowHandle>, | ||
| canvas: HtmlCanvasElement, | ||
| ctx: CanvasRenderingContext2d, | ||
| } | ||
|
|
||
| impl Canvas { | ||
| pub(crate) fn new(render_canvas: HtmlCanvasElement, width: NonZeroU32, height: NonZeroU32) -> anyhow::Result<Self> { | ||
| render_canvas.set_width(width.get()); | ||
| render_canvas.set_height(height.get()); | ||
| let ctx = context_2d(&render_canvas)?; | ||
|
|
||
| #[cfg(target_arch = "wasm32")] | ||
| let mut surface = { | ||
| use softbuffer::SurfaceExtWeb as _; | ||
| softbuffer::Surface::from_canvas(render_canvas).expect("surface") | ||
| }; | ||
|
|
||
| #[cfg(not(target_arch = "wasm32"))] | ||
| let mut surface = { | ||
| fn stub(_: HtmlCanvasElement) -> softbuffer::Surface<NoDisplayHandle, NoWindowHandle> { | ||
| unimplemented!() | ||
| } | ||
|
|
||
| stub(render_canvas) | ||
| }; | ||
|
|
||
| surface.resize(width, height).expect("surface resize"); | ||
|
|
||
| Ok(Self { width, surface }) | ||
| Ok(Self { | ||
| canvas: render_canvas, | ||
| ctx, | ||
| }) | ||
| } | ||
|
|
||
| /// Resizes the canvas backing store to `width` x `height`. Setting width/height clears the | ||
| /// canvas and resets 2D context state (transform, styles, ...); the cached `ctx` handle stays | ||
| /// valid. Callers must not rely on prior canvas content or context configuration surviving. | ||
| pub(crate) fn resize(&mut self, width: NonZeroU32, height: NonZeroU32) { | ||
| self.surface.resize(width, height).expect("surface resize"); | ||
| self.width = width; | ||
| self.canvas.set_width(width.get()); | ||
| self.canvas.set_height(height.get()); | ||
|
irvingoujAtDevolution marked this conversation as resolved.
|
||
| } | ||
|
|
||
| pub(crate) fn draw(&mut self, buffer: &[u8], region: InclusiveRectangle) -> anyhow::Result<()> { | ||
| let region_width = region.width(); | ||
| let region_height = region.height(); | ||
|
|
||
| let mut src = buffer.chunks_exact(4).map(|pixel| { | ||
| let r = pixel[0]; | ||
| let g = pixel[1]; | ||
| let b = pixel[2]; | ||
| u32::from_be_bytes([0, r, g, b]) | ||
| }); | ||
|
|
||
| let mut dst = self.surface.buffer_mut().expect("surface buffer"); | ||
|
|
||
| { | ||
| // Copy src into dst | ||
|
|
||
| let region_top_usize = usize::from(region.top); | ||
| let region_height_usize = usize::from(region_height); | ||
| let region_left_usize = usize::from(region.left); | ||
| let region_width_usize = usize::from(region_width); | ||
|
|
||
| for dst_row in dst | ||
| .chunks_exact_mut(usize::try_from(self.width.get()).context("canvas width")?) | ||
| .skip(region_top_usize) | ||
| .take(region_height_usize) | ||
| { | ||
| let src_row = src.by_ref().take(region_width_usize); | ||
|
|
||
| dst_row | ||
| .iter_mut() | ||
| .skip(region_left_usize) | ||
| .take(region_width_usize) | ||
| .zip(src_row) | ||
| .for_each(|(dst, src)| *dst = src); | ||
| } | ||
| /// Blits one dirty region. `buffer` is the region's RGBA sub-image — a throwaway copy produced | ||
| /// by `extract_partial_image` — mutated in place to force opaque alpha before upload. | ||
|
irvingoujAtDevolution marked this conversation as resolved.
Outdated
|
||
| pub(crate) fn draw(&mut self, buffer: &mut [u8], region: InclusiveRectangle) -> anyhow::Result<()> { | ||
| // Force opaque alpha in place. The decoded framebuffer leaves the alpha channel as | ||
| // don't-care (it starts at 0 and the decode loop skips it), but `put_image_data` stores | ||
| // alpha verbatim, so the region would otherwise render transparent on the canvas. | ||
| for pixel in buffer.chunks_exact_mut(4) { | ||
| pixel[3] = 0xFF; | ||
| } | ||
|
Comment on lines
+42
to
44
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: Just challenging the established pattern here: do we really need to force opaque alpha? I assume that if we don’t do that we end up with visual artifacts? My understanding is that we are just extracting a sub-image from an otherwise fully rendered image, and that we don’t really need any extra cleaning step.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Benoît Cortier (@CBenoit) yeah, works for most cases but it's not guaranteed. Framebuffer starts zero-filled (here) and the QOI-RGBA path keeps source alpha (here) — plus a tall update gets widened to full width, so early / post-resize frames can upload not-yet-painted columns as transparent. Best case we'd guarantee opaque upstream (init the framebuffer to
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good to me! My only suggestion then is to track this with an issue for visibility |
||
|
|
||
| let damage_rect = softbuffer::Rect { | ||
| x: u32::from(region.left), | ||
| y: u32::from(region.top), | ||
| width: NonZeroU32::new(u32::from(region_width)) | ||
| .expect("per InclusiveRectangle invariants: 0 < region_width"), | ||
| height: NonZeroU32::new(u32::from(region_height)) | ||
| .expect("per InclusiveRectangle invariants: 0 < region_height"), | ||
| }; | ||
| blit(&self.ctx, buffer, ®ion) | ||
| } | ||
| } | ||
|
|
||
| dst.present_with_damage(&[damage_rect]).expect("buffer present"); | ||
| /// Acquires the canvas 2D context. Only meaningful on wasm; on other targets it exists solely so | ||
| /// host tooling type-checks, and panics if called. | ||
| fn context_2d(canvas: &HtmlCanvasElement) -> anyhow::Result<CanvasRenderingContext2d> { | ||
| #[cfg(target_arch = "wasm32")] | ||
| { | ||
| canvas | ||
| .get_context("2d") | ||
| .map_err(|err| anyhow!("get_context(\"2d\") failed: {err:?}"))? | ||
| .ok_or_else(|| anyhow!("canvas has no 2d context"))? | ||
| .dyn_into::<CanvasRenderingContext2d>() | ||
| .map_err(|_| anyhow!("2d context is not a CanvasRenderingContext2d")) | ||
| } | ||
| #[cfg(not(target_arch = "wasm32"))] | ||
| { | ||
| let _ = canvas; | ||
| unimplemented!("web canvas is only available on wasm32") | ||
| } | ||
| } | ||
|
|
||
| Ok(()) | ||
| /// Blits `rgba` (a `region`-sized RGBA buffer) onto the canvas at the region's origin. | ||
| fn blit(ctx: &CanvasRenderingContext2d, rgba: &[u8], region: &InclusiveRectangle) -> anyhow::Result<()> { | ||
| #[cfg(target_arch = "wasm32")] | ||
| { | ||
| let image = ImageData::new_with_u8_clamped_array_and_sh( | ||
| Clamped(rgba), | ||
| u32::from(region.width()), | ||
| u32::from(region.height()), | ||
| ) | ||
| .map_err(|err| anyhow!("ImageData::new failed: {err:?}"))?; | ||
|
irvingoujAtDevolution marked this conversation as resolved.
Outdated
|
||
| ctx.put_image_data(&image, f64::from(region.left), f64::from(region.top)) | ||
| .map_err(|err| anyhow!("put_image_data failed: {err:?}")) | ||
| } | ||
| #[cfg(not(target_arch = "wasm32"))] | ||
| { | ||
| let _ = (ctx, rgba, region); | ||
| unimplemented!("web canvas is only available on wasm32") | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.