From 58a1baa4ed2e963bfa3506c78afef684d2643648 Mon Sep 17 00:00:00 2001 From: dev01prishusoft Date: Fri, 4 Sep 2026 20:38:27 +0530 Subject: [PATCH 1/5] feat(gpui): add an unatlased external texture primitive --- crates/gpui/src/external_texture.rs | 871 ++++++++++++++++++++ crates/gpui/src/gpui.rs | 2 + crates/gpui/src/scene.rs | 46 +- crates/gpui/src/window.rs | 34 +- crates/gpui_macos/src/metal_renderer.rs | 26 +- crates/gpui_wgpu/src/shaders.wgsl | 54 ++ crates/gpui_wgpu/src/wgpu_renderer.rs | 252 +++++- crates/gpui_windows/build.rs | 1 + crates/gpui_windows/src/directx_renderer.rs | 243 ++++++ crates/gpui_windows/src/shaders.hlsl | 55 ++ 10 files changed, 1564 insertions(+), 20 deletions(-) create mode 100644 crates/gpui/src/external_texture.rs diff --git a/crates/gpui/src/external_texture.rs b/crates/gpui/src/external_texture.rs new file mode 100644 index 00000000000000..45e2c99864d37b --- /dev/null +++ b/crates/gpui/src/external_texture.rs @@ -0,0 +1,871 @@ +//! Caller-owned GPU textures composited **inside** the GPUI scene. +//! +//! This exists for one reason: Cherry Pick's embedded browser paints Chromium +//! off-screen and needs those pixels inside the swap chain, at 60fps, without +//! going anywhere near the sprite atlas. +//! +//! # Why not the sprite atlas +//! +//! [`crate::Window::paint_image`] packs its pixels into a shared atlas texture +//! alongside every icon and glyph on screen. That is right for content that is +//! uploaded once and drawn many times. A live web page is the opposite: a +//! 1920x1080 BGRA frame is 8 MB, it changes on its own schedule, and pushing it +//! through the atlas would evict every icon in the app on the first frame and +//! then thrash the allocator forever after. It would also serialise browser +//! uploads behind unrelated text rendering. +//! +//! So an external texture gets its own GPU texture, keyed by [`ExternalTextureId`], +//! that the renderer creates once and re-uses. Only the pixels that changed are +//! re-uploaded, and a page that has settled uploads nothing at all. +//! +//! # Threading +//! +//! The producer (a Chromium OSR paint callback, on a Chromium thread) writes +//! into its own buffer and bumps [`ExternalFrameView::sequence`]. The consumer +//! (a GPUI renderer, on the UI thread during paint) calls +//! [`ExternalTextureSource::with_frame`], which is expected to take whatever +//! lock the producer uses. Implementations must keep that critical section to a +//! memcpy's worth of work — it runs inside the frame budget. +//! +//! # Resize +//! +//! Reallocating the buffer bumps [`ExternalFrameView::generation`]. A renderer +//! that sees a new generation throws its cached texture away and creates a new +//! one, which is what keeps a stale-sized texture from being sampled with new +//! dimensions after a pane resize. + +use std::fmt; +use std::sync::Arc; +use std::sync::atomic::{AtomicU64, Ordering}; + +use crate::{Bounds, DevicePixels, Size}; + +/// Stable identity for one external texture across frames. +/// +/// The renderer caches a GPU texture per id, so this must be stable for the +/// life of the producing surface and must never be reused by a different +/// producer while the old one could still be in a scene. +#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord)] +pub struct ExternalTextureId(pub u64); + +impl ExternalTextureId { + /// Hand out an id no other caller in this process will get. + pub fn next() -> Self { + static NEXT: AtomicU64 = AtomicU64::new(1); + Self(NEXT.fetch_add(1, Ordering::Relaxed)) + } +} + +/// Byte order of an external frame's pixels. +/// +/// Both are 8 bits per channel and 4 bytes per pixel; only the channel order +/// differs. Chromium OSR hands out `Bgra8`. +#[derive(Copy, Clone, Debug, PartialEq, Eq)] +pub enum ExternalTextureFormat { + /// Blue, green, red, alpha. What Chromium's `OnPaint` produces. + Bgra8, + /// Red, green, blue, alpha. + Rgba8, +} + +impl ExternalTextureFormat { + /// Bytes per pixel. Both variants are 4. + pub const fn bytes_per_pixel(self) -> usize { + 4 + } +} + +/// A borrowed look at the producer's current pixels. +/// +/// Only valid for the duration of the [`ExternalTextureSource::with_frame`] +/// callback that produced it. +pub struct ExternalFrameView<'a> { + /// Pixel dimensions of the whole buffer. + pub size: Size, + /// Bytes between the start of one row and the start of the next. May be + /// larger than `size.width * 4` when the producer pads rows. + pub stride: usize, + /// Channel order of `bytes`. + pub format: ExternalTextureFormat, + /// The pixels. At least `stride * size.height` long. + pub bytes: &'a [u8], + /// Bumped whenever the buffer is reallocated, which for a browser means a + /// resize. A renderer must recreate its cached texture when this changes. + pub generation: u64, + /// Bumped on every painted frame. A renderer that has already uploaded this + /// sequence must not upload again — this is what makes a settled page cost + /// zero bandwidth. + pub sequence: u64, + /// The regions that changed since `sequence - 1`, in texture pixels. Empty + /// means "assume everything changed", which is correct but expensive; a + /// producer should always fill this in when it knows. + pub dirty: &'a [Bounds], +} + +impl ExternalFrameView<'_> { + /// Whether the buffer is large enough for its own declared geometry. + /// + /// A renderer must check this before indexing. A producer that gets this + /// wrong is a bug, but it must not be a GPU crash or an out-of-bounds read. + pub fn is_well_formed(&self) -> bool { + let width = self.size.width.0.max(0) as usize; + let height = self.size.height.0.max(0) as usize; + if width == 0 || height == 0 { + return false; + } + if self.stride < width * self.format.bytes_per_pixel() { + return false; + } + self.bytes.len() >= self.stride * height + } + + /// The dirty regions, clamped to the buffer, with an empty list meaning the + /// whole frame. Renderers should upload exactly these. + pub fn dirty_regions(&self) -> Vec> { + let full = Bounds { + origin: crate::point(DevicePixels(0), DevicePixels(0)), + size: self.size, + }; + if self.dirty.is_empty() { + return vec![full]; + } + self.dirty + .iter() + .filter_map(|rect| { + let clamped = rect.intersect(&full); + (!clamped.is_empty()).then_some(clamped) + }) + .collect() + } +} + +/// Something that produces frames for an external texture. +/// +/// Implemented outside GPUI (Cherry Pick's browser engine implements it over a +/// Chromium OSR buffer). GPUI only reads. +pub trait ExternalTextureSource: fmt::Debug + Send + Sync + 'static { + /// Stable identity, so the renderer can cache one GPU texture per source. + fn id(&self) -> ExternalTextureId; + + /// Show the renderer the current frame, if there is one. + /// + /// Called on the UI thread inside paint. `visit` is not called when no + /// frame has been produced yet, which is the normal state for a pane whose + /// page has not painted. + fn with_frame(&self, visit: &mut dyn FnMut(ExternalFrameView<'_>)); + + /// Told to the source after the renderer uploads `sequence`. + /// + /// Sources use it for the upload counter the performance gates read + /// (PERF-B04: a static page must settle at zero uploads per second). + /// + /// **Called from inside [`Self::with_frame`]**, because that is the only + /// place a renderer can see the sequence it just uploaded. An + /// implementation must therefore not take any lock that `with_frame` + /// holds, or the render thread deadlocks on the first painted frame. + fn mark_uploaded(&self, sequence: u64); + + /// How many uploads this source has served. Instrumentation only. + fn upload_count(&self) -> u64; +} + +/// A frame producer plus the bookkeeping every implementation would otherwise +/// duplicate: double buffering, dirty-rect accumulation, sequence and +/// generation counters, and the upload counter the perf gates read. +/// +/// Producers call [`Self::submit`]; GPUI calls the [`ExternalTextureSource`] +/// methods. Neither side needs to know about the other's thread. +pub struct ExternalTextureBuffer { + id: ExternalTextureId, + state: parking_lot::Mutex, + /// Read by perf assertions from another thread, so it lives outside the + /// lock. + uploads: AtomicU64, + /// Mirrors `state.sequence` for lock-free reads. + sequence: AtomicU64, + /// The last sequence a renderer said it uploaded. + /// + /// Outside the mutex on purpose. Renderers acknowledge from inside the + /// [`ExternalTextureSource::with_frame`] visitor, because that is the only + /// place they can see the sequence they just uploaded, and the lock is held + /// for the whole visit. An acknowledgement that took the lock would + /// deadlock the render thread on the first painted frame. + acknowledged: AtomicU64, +} + +#[derive(Default)] +struct BufferState { + bytes: Vec, + size: Size, + stride: usize, + format: Option, + generation: u64, + sequence: u64, + dirty: Vec>, +} + +impl fmt::Debug for ExternalTextureBuffer { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let state = self.state.lock(); + f.debug_struct("ExternalTextureBuffer") + .field("id", &self.id) + .field("size", &state.size) + .field("generation", &state.generation) + .field("sequence", &state.sequence) + .finish() + } +} + +impl Default for ExternalTextureBuffer { + fn default() -> Self { + Self::new() + } +} + +impl ExternalTextureBuffer { + /// A buffer with no frame yet. Allocates nothing until the first + /// [`Self::submit`], which is what lets a browser pane exist without + /// costing a framebuffer (PRD ENG-02). + pub fn new() -> Self { + Self { + id: ExternalTextureId::next(), + state: parking_lot::Mutex::new(BufferState::default()), + uploads: AtomicU64::new(0), + sequence: AtomicU64::new(0), + acknowledged: AtomicU64::new(0), + } + } + + /// Wrap in an `Arc` for handing to [`crate::Window::paint_external_texture`]. + pub fn shared() -> Arc { + Arc::new(Self::new()) + } + + /// Whether any frame has been produced. A pane whose page has not painted + /// yet must not draw a black rectangle over its own placeholder. + pub fn has_frame(&self) -> bool { + self.sequence.load(Ordering::Acquire) > 0 + } + + /// The frame counter, for tests and instrumentation. + pub fn sequence(&self) -> u64 { + self.sequence.load(Ordering::Acquire) + } + + /// Current buffer dimensions, or zero before the first frame. + pub fn size(&self) -> Size { + self.state.lock().size + } + + /// Publish a frame. + /// + /// `dirty` is in texture pixels; an empty slice means the whole frame. A + /// size or format change reallocates and bumps the generation, and the + /// dirty list is then irrelevant because the renderer will recreate the + /// texture anyway. + /// + /// Returns `false` and drops the frame when `bytes` is too small for the + /// declared geometry, so a misbehaving producer cannot make the renderer + /// read out of bounds. + pub fn submit( + &self, + size: Size, + stride: usize, + format: ExternalTextureFormat, + bytes: &[u8], + dirty: &[Bounds], + ) -> bool { + let width = size.width.0.max(0) as usize; + let height = size.height.0.max(0) as usize; + if width == 0 || height == 0 || stride < width * format.bytes_per_pixel() { + return false; + } + let needed = stride + .checked_mul(height) + .filter(|needed| *needed <= bytes.len()); + let Some(needed) = needed else { + return false; + }; + + let mut state = self.state.lock(); + let reallocated = + state.size != size || state.stride != stride || state.format != Some(format); + if reallocated { + state.bytes.clear(); + state.bytes.resize(needed, 0); + state.size = size; + state.stride = stride; + state.format = Some(format); + state.generation += 1; + state.dirty.clear(); + } else if state.bytes.len() < needed { + state.bytes.resize(needed, 0); + } + state.bytes[..needed].copy_from_slice(&bytes[..needed]); + + let acknowledged = self.acknowledged.load(Ordering::Acquire); + if reallocated { + // A recreated texture is uploaded whole; per-rect bookkeeping would + // just be thrown away. + state.dirty.clear(); + } else if state.sequence > acknowledged { + // The renderer has not consumed the previous frame, so this frame's + // dirt is added to it rather than replacing it. Dropping it would + // leave half the page showing stale pixels. + state.dirty.extend_from_slice(dirty); + } else { + // Everything up to here has been uploaded, so the previous rects + // are spent. This is also where an acknowledgement's clean-up + // happens, which is why `mark_uploaded` does not need the lock. + state.dirty.clear(); + state.dirty.extend_from_slice(dirty); + } + + state.sequence += 1; + self.sequence.store(state.sequence, Ordering::Release); + true + } + + /// Forget the frame without dropping the identity, so a hidden pane stops + /// holding a framebuffer while keeping its renderer (PRD HID-01). + pub fn release_frame(&self) { + let mut state = self.state.lock(); + state.bytes = Vec::new(); + state.size = Size::default(); + state.stride = 0; + state.format = None; + state.dirty.clear(); + state.generation += 1; + state.sequence = 0; + self.sequence.store(0, Ordering::Release); + self.acknowledged.store(0, Ordering::Release); + } +} + +impl ExternalTextureSource for ExternalTextureBuffer { + fn id(&self) -> ExternalTextureId { + self.id + } + + fn with_frame(&self, visit: &mut dyn FnMut(ExternalFrameView<'_>)) { + let state = self.state.lock(); + let Some(format) = state.format else { + return; + }; + if state.sequence == 0 { + return; + } + visit(ExternalFrameView { + size: state.size, + stride: state.stride, + format, + bytes: &state.bytes, + generation: state.generation, + sequence: state.sequence, + dirty: &state.dirty, + }); + } + + fn mark_uploaded(&self, sequence: u64) { + // Deliberately lock-free: this is called from inside `with_frame`, + // which holds the state lock for the whole visit. + self.acknowledged.fetch_max(sequence, Ordering::AcqRel); + self.uploads.fetch_add(1, Ordering::Relaxed); + } + + fn upload_count(&self) -> u64 { + self.uploads.load(Ordering::Relaxed) + } +} + +/// What a renderer needs to decide whether to upload, kept out of the renderers +/// so Windows and Linux cannot drift apart on the rule. +#[derive(Copy, Clone, Debug, PartialEq, Eq)] +pub struct ExternalTextureCacheKey { + /// Which source the cached texture belongs to. + pub id: ExternalTextureId, + /// Which allocation. A change means recreate. + pub generation: u64, + /// Which frame. A change means upload the dirty rects. + pub sequence: u64, + /// Cached texture dimensions. + pub size: Size, +} + +/// The decision a renderer makes for one external texture this frame. +#[derive(Copy, Clone, Debug, PartialEq, Eq)] +pub enum ExternalTextureUpdate { + /// No cached texture, or the producer reallocated. Create and upload whole. + Recreate, + /// Same texture, new pixels. Upload the dirty rects. + UploadDirty, + /// Nothing changed. Draw the cached texture and upload nothing. This is the + /// branch a settled page must take every frame (PERF-B04). + Reuse, +} + +/// Decide what to do with a cached texture given the frame on offer. +pub fn plan_external_texture_update( + cached: Option, + frame: &ExternalFrameView<'_>, +) -> ExternalTextureUpdate { + match cached { + Some(cached) + if cached.generation == frame.generation + && cached.size == frame.size + && cached.sequence == frame.sequence => + { + ExternalTextureUpdate::Reuse + } + Some(cached) if cached.generation == frame.generation && cached.size == frame.size => { + ExternalTextureUpdate::UploadDirty + } + _ => ExternalTextureUpdate::Recreate, + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::{point, size}; + + fn bgra(width: i32, height: i32, fill: u8) -> Vec { + vec![fill; (width * height * 4) as usize] + } + + fn dims(width: i32, height: i32) -> Size { + size(DevicePixels(width), DevicePixels(height)) + } + + #[test] + fn a_buffer_with_no_frame_never_calls_the_visitor() { + let buffer = ExternalTextureBuffer::new(); + assert!(!buffer.has_frame()); + let mut seen = 0; + buffer.with_frame(&mut |_| seen += 1); + assert_eq!(seen, 0, "a pane that never painted must draw nothing"); + } + + /// The counter PERF-B04 reads must count uploads, not draws. + /// + /// This is the shape of a bug that shipped in the Windows renderer and not + /// the wgpu one: it acknowledged whenever the cached sequence matched the + /// frame's, which is exactly the `Reuse` condition, so a still page + /// appeared to upload once per composited frame. + #[test] + fn reusing_a_cached_texture_is_not_an_upload() { + let buffer = ExternalTextureBuffer::new(); + let pixels = bgra(4, 2, 0x11); + assert!(buffer.submit(dims(4, 2), 16, ExternalTextureFormat::Bgra8, &pixels, &[])); + + // First look at the frame: nothing cached, so the renderer uploads. + let mut cached = None; + buffer.with_frame(&mut |frame| { + assert_eq!( + plan_external_texture_update(cached, &frame), + ExternalTextureUpdate::Recreate + ); + cached = Some(ExternalTextureCacheKey { + id: buffer.id(), + generation: frame.generation, + sequence: frame.sequence, + size: frame.size, + }); + buffer.mark_uploaded(frame.sequence); + }); + assert_eq!(buffer.upload_count(), 1); + + // Every later frame draws the same pixels. A renderer that + // acknowledges here is counting compositing, not uploading. + for _ in 0..30 { + buffer.with_frame(&mut |frame| { + let plan = plan_external_texture_update(cached, &frame); + assert_eq!(plan, ExternalTextureUpdate::Reuse); + if !matches!(plan, ExternalTextureUpdate::Reuse) { + buffer.mark_uploaded(frame.sequence); + } + }); + } + assert_eq!( + buffer.upload_count(), + 1, + "a settled page must stop adding to the upload counter" + ); + } + + #[test] + fn ids_are_unique_per_source() { + let a = ExternalTextureBuffer::new(); + let b = ExternalTextureBuffer::new(); + assert_ne!(a.id(), b.id()); + } + + #[test] + fn a_submitted_frame_is_visible_to_the_renderer() { + let buffer = ExternalTextureBuffer::new(); + let pixels = bgra(4, 2, 0x7f); + assert!(buffer.submit(dims(4, 2), 16, ExternalTextureFormat::Bgra8, &pixels, &[])); + assert!(buffer.has_frame()); + + let mut observed = None; + buffer.with_frame(&mut |frame| { + assert!(frame.is_well_formed()); + observed = Some((frame.size, frame.sequence, frame.generation, frame.format)); + assert!(frame.bytes.iter().all(|byte| *byte == 0x7f)); + }); + assert_eq!( + observed, + Some((dims(4, 2), 1, 1, ExternalTextureFormat::Bgra8)) + ); + } + + #[test] + fn a_short_buffer_is_refused_rather_than_read_out_of_bounds() { + let buffer = ExternalTextureBuffer::new(); + let too_small = bgra(4, 1, 0); + assert!(!buffer.submit( + dims(4, 2), + 16, + ExternalTextureFormat::Bgra8, + &too_small, + &[] + )); + assert!(!buffer.has_frame()); + } + + #[test] + fn a_stride_narrower_than_the_row_is_refused() { + let buffer = ExternalTextureBuffer::new(); + let pixels = bgra(4, 2, 0); + assert!(!buffer.submit(dims(4, 2), 8, ExternalTextureFormat::Bgra8, &pixels, &[])); + } + + #[test] + fn a_settled_page_uploads_nothing() { + // PERF-B04 in one test: submit once, paint three times, and only the + // first paint may upload. + let buffer = ExternalTextureBuffer::new(); + let pixels = bgra(8, 8, 1); + buffer.submit(dims(8, 8), 32, ExternalTextureFormat::Bgra8, &pixels, &[]); + + let mut cached: Option = None; + let mut plans = Vec::new(); + for _ in 0..3 { + buffer.with_frame(&mut |frame| { + let plan = plan_external_texture_update(cached, &frame); + plans.push(plan); + if !matches!(plan, ExternalTextureUpdate::Reuse) { + buffer.mark_uploaded(frame.sequence); + cached = Some(ExternalTextureCacheKey { + id: buffer.id(), + generation: frame.generation, + sequence: frame.sequence, + size: frame.size, + }); + } + }); + } + assert_eq!( + plans, + vec![ + ExternalTextureUpdate::Recreate, + ExternalTextureUpdate::Reuse, + ExternalTextureUpdate::Reuse + ] + ); + assert_eq!(buffer.upload_count(), 1); + } + + #[test] + fn a_new_frame_at_the_same_size_uploads_only_the_dirty_rects() { + let buffer = ExternalTextureBuffer::new(); + let pixels = bgra(8, 8, 1); + buffer.submit(dims(8, 8), 32, ExternalTextureFormat::Bgra8, &pixels, &[]); + let mut cached = None; + buffer.with_frame(&mut |frame| { + buffer.mark_uploaded(frame.sequence); + cached = Some(ExternalTextureCacheKey { + id: buffer.id(), + generation: frame.generation, + sequence: frame.sequence, + size: frame.size, + }); + }); + + let dirty = [Bounds { + origin: point(DevicePixels(2), DevicePixels(3)), + size: dims(4, 2), + }]; + buffer.submit( + dims(8, 8), + 32, + ExternalTextureFormat::Bgra8, + &bgra(8, 8, 2), + &dirty, + ); + + let mut plan = None; + let mut regions = Vec::new(); + buffer.with_frame(&mut |frame| { + plan = Some(plan_external_texture_update(cached, &frame)); + regions = frame.dirty_regions(); + }); + assert_eq!(plan, Some(ExternalTextureUpdate::UploadDirty)); + assert_eq!(regions, dirty.to_vec()); + } + + #[test] + fn a_resize_forces_the_renderer_to_recreate_its_texture() { + let buffer = ExternalTextureBuffer::new(); + buffer.submit( + dims(8, 8), + 32, + ExternalTextureFormat::Bgra8, + &bgra(8, 8, 1), + &[], + ); + let mut cached = None; + buffer.with_frame(&mut |frame| { + cached = Some(ExternalTextureCacheKey { + id: buffer.id(), + generation: frame.generation, + sequence: frame.sequence, + size: frame.size, + }); + }); + + buffer.submit( + dims(16, 8), + 64, + ExternalTextureFormat::Bgra8, + &bgra(16, 8, 1), + &[], + ); + let mut plan = None; + buffer.with_frame(&mut |frame| { + plan = Some(plan_external_texture_update(cached, &frame)); + assert_eq!(frame.generation, 2, "a reallocation bumps the generation"); + }); + assert_eq!(plan, Some(ExternalTextureUpdate::Recreate)); + } + + #[test] + fn dirt_accumulates_while_the_renderer_is_behind() { + // Two frames land between paints. The renderer must be told about both + // regions, or half the page keeps stale pixels. + let buffer = ExternalTextureBuffer::new(); + buffer.submit( + dims(8, 8), + 32, + ExternalTextureFormat::Bgra8, + &bgra(8, 8, 0), + &[], + ); + buffer.with_frame(&mut |frame| buffer.mark_uploaded(frame.sequence)); + + let first = Bounds { + origin: point(DevicePixels(0), DevicePixels(0)), + size: dims(2, 2), + }; + let second = Bounds { + origin: point(DevicePixels(4), DevicePixels(4)), + size: dims(2, 2), + }; + buffer.submit( + dims(8, 8), + 32, + ExternalTextureFormat::Bgra8, + &bgra(8, 8, 1), + &[first], + ); + buffer.submit( + dims(8, 8), + 32, + ExternalTextureFormat::Bgra8, + &bgra(8, 8, 2), + &[second], + ); + + let mut regions = Vec::new(); + buffer.with_frame(&mut |frame| regions = frame.dirty_regions()); + assert_eq!(regions, vec![first, second]); + } + + #[test] + fn an_empty_dirty_list_means_the_whole_frame() { + let buffer = ExternalTextureBuffer::new(); + buffer.submit( + dims(6, 4), + 24, + ExternalTextureFormat::Bgra8, + &bgra(6, 4, 0), + &[], + ); + let mut regions = Vec::new(); + buffer.with_frame(&mut |frame| regions = frame.dirty_regions()); + assert_eq!( + regions, + vec![Bounds { + origin: point(DevicePixels(0), DevicePixels(0)), + size: dims(6, 4), + }] + ); + } + + #[test] + fn dirty_rects_are_clamped_to_the_buffer() { + // The rect has to arrive on a frame that is *not* a reallocation, + // because a reallocation uploads the whole texture and discards + // per-rect bookkeeping. The interesting case is a stale rect from a + // larger previous frame arriving after a shrink. + let buffer = ExternalTextureBuffer::new(); + buffer.submit( + dims(8, 8), + 32, + ExternalTextureFormat::Bgra8, + &bgra(8, 8, 0), + &[], + ); + buffer.with_frame(&mut |frame| buffer.mark_uploaded(frame.sequence)); + + let outside = Bounds { + origin: point(DevicePixels(4), DevicePixels(4)), + size: dims(64, 64), + }; + buffer.submit( + dims(8, 8), + 32, + ExternalTextureFormat::Bgra8, + &bgra(8, 8, 1), + &[outside], + ); + let mut regions = Vec::new(); + buffer.with_frame(&mut |frame| regions = frame.dirty_regions()); + assert_eq!( + regions, + vec![Bounds { + origin: point(DevicePixels(4), DevicePixels(4)), + size: dims(4, 4), + }], + "a producer's rect must never index past the texture" + ); + } + + #[test] + fn the_first_frame_after_a_reallocation_uploads_everything() { + // The behaviour the test above had to work around, asserted directly: + // a new texture has no pixels, so a partial upload would leave the rest + // of the pane as whatever the driver had in that memory. + let buffer = ExternalTextureBuffer::new(); + let corner = Bounds { + origin: point(DevicePixels(0), DevicePixels(0)), + size: dims(1, 1), + }; + buffer.submit( + dims(8, 8), + 32, + ExternalTextureFormat::Bgra8, + &bgra(8, 8, 0), + &[corner], + ); + let mut regions = Vec::new(); + buffer.with_frame(&mut |frame| regions = frame.dirty_regions()); + assert_eq!( + regions, + vec![Bounds { + origin: point(DevicePixels(0), DevicePixels(0)), + size: dims(8, 8), + }] + ); + } + + #[test] + fn acknowledging_from_inside_the_visitor_does_not_deadlock() { + // The renderers do exactly this: they learn the sequence they uploaded + // from the frame view, and acknowledge it before the view goes out of + // scope. `with_frame` holds the state lock for the whole visit, so an + // acknowledgement that took the same lock would hang the render thread + // on the very first painted frame. + let buffer = ExternalTextureBuffer::new(); + buffer.submit( + dims(4, 4), + 16, + ExternalTextureFormat::Bgra8, + &bgra(4, 4, 3), + &[], + ); + buffer.with_frame(&mut |frame| { + buffer.mark_uploaded(frame.sequence); + }); + assert_eq!(buffer.upload_count(), 1); + } + + #[test] + fn an_acknowledged_frame_stops_re_reporting_its_dirty_rects() { + // The clean-up that used to live in `mark_uploaded` now happens on the + // next submit, so this is where it has to be proven. + let buffer = ExternalTextureBuffer::new(); + buffer.submit( + dims(8, 8), + 32, + ExternalTextureFormat::Bgra8, + &bgra(8, 8, 0), + &[], + ); + buffer.with_frame(&mut |frame| buffer.mark_uploaded(frame.sequence)); + + let first = Bounds { + origin: point(DevicePixels(0), DevicePixels(0)), + size: dims(2, 2), + }; + buffer.submit( + dims(8, 8), + 32, + ExternalTextureFormat::Bgra8, + &bgra(8, 8, 1), + &[first], + ); + buffer.with_frame(&mut |frame| { + assert_eq!(frame.dirty_regions(), vec![first]); + buffer.mark_uploaded(frame.sequence); + }); + + let second = Bounds { + origin: point(DevicePixels(4), DevicePixels(4)), + size: dims(2, 2), + }; + buffer.submit( + dims(8, 8), + 32, + ExternalTextureFormat::Bgra8, + &bgra(8, 8, 2), + &[second], + ); + let mut regions = Vec::new(); + buffer.with_frame(&mut |frame| regions = frame.dirty_regions()); + assert_eq!( + regions, + vec![second], + "the acknowledged rect must not be uploaded a second time" + ); + } + + #[test] + fn releasing_a_frame_keeps_the_identity_but_drops_the_pixels() { + let buffer = ExternalTextureBuffer::new(); + let id = buffer.id(); + buffer.submit( + dims(8, 8), + 32, + ExternalTextureFormat::Bgra8, + &bgra(8, 8, 1), + &[], + ); + buffer.release_frame(); + assert!(!buffer.has_frame()); + assert_eq!(buffer.id(), id); + let mut seen = 0; + buffer.with_frame(&mut |_| seen += 1); + assert_eq!(seen, 0); + } +} diff --git a/crates/gpui/src/gpui.rs b/crates/gpui/src/gpui.rs index a81ff265c3edd8..d68d4c58dbb08f 100644 --- a/crates/gpui/src/gpui.rs +++ b/crates/gpui/src/gpui.rs @@ -21,6 +21,7 @@ pub mod colors; mod element; mod elements; mod executor; +mod external_texture; mod platform_scheduler; pub(crate) use platform_scheduler::PlatformScheduler; mod geometry; @@ -97,6 +98,7 @@ pub use ctor::ctor; pub use element::*; pub use elements::*; pub use executor::*; +pub use external_texture::*; pub use geometry::*; pub use global::*; pub use gpui_macros::{ diff --git a/crates/gpui/src/scene.rs b/crates/gpui/src/scene.rs index bc7f5d79eace55..c53cf58bbfc77c 100644 --- a/crates/gpui/src/scene.rs +++ b/crates/gpui/src/scene.rs @@ -721,8 +721,52 @@ pub struct PaintSurface { pub order: DrawOrder, pub bounds: Bounds, pub content_mask: ContentMask, + pub content: SurfaceContent, +} + +/// What a [`PaintSurface`] samples from. +/// +/// Both variants bypass the sprite atlas: a surface owns its texture. That is +/// the point of the primitive. See [`crate::external_texture`] for why a live +/// video frame or web page must never be atlased. +#[derive(Clone)] +pub enum SurfaceContent { + /// A CoreVideo buffer, as produced by macOS video decode. YUV, sampled by + /// the Metal renderer's two-plane surface shader. #[cfg(target_os = "macos")] - pub image_buffer: core_video::pixel_buffer::CVPixelBuffer, + PixelBuffer(core_video::pixel_buffer::CVPixelBuffer), + /// A caller-owned RGBA/BGRA frame, uploaded to a dedicated texture the + /// renderer caches per [`crate::ExternalTextureId`]. This is the + /// cross-platform path, and the one Chromium off-screen rendering uses. + ExternalTexture(std::sync::Arc), +} + +impl SurfaceContent { + /// The caller-owned texture this surface samples, if it is one. + /// + /// Renderers use this rather than destructuring, because off macOS the enum + /// has a single variant and a `let ... else` on it is irrefutable. It also + /// keeps the mapping in one place instead of once per backend. + pub fn external_texture(&self) -> Option<&std::sync::Arc> { + match self { + #[cfg(target_os = "macos")] + Self::PixelBuffer(_) => None, + Self::ExternalTexture(source) => Some(source), + } + } +} + +impl Debug for SurfaceContent { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + #[cfg(target_os = "macos")] + Self::PixelBuffer(_) => f.write_str("SurfaceContent::PixelBuffer"), + Self::ExternalTexture(source) => f + .debug_tuple("SurfaceContent::ExternalTexture") + .field(&source.id()) + .finish(), + } + } } impl From for Primitive { diff --git a/crates/gpui/src/window.rs b/crates/gpui/src/window.rs index 37a54bcebab17e..32522845715265 100644 --- a/crates/gpui/src/window.rs +++ b/crates/gpui/src/window.rs @@ -4120,7 +4120,7 @@ impl Window { /// This method should only be called as part of the paint phase of element drawing. #[cfg(target_os = "macos")] pub fn paint_surface(&mut self, bounds: Bounds, image_buffer: CVPixelBuffer) { - use crate::PaintSurface; + use crate::{PaintSurface, SurfaceContent}; self.invalidator.debug_assert_paint(); @@ -4130,7 +4130,37 @@ impl Window { order: 0, bounds, content_mask, - image_buffer, + content: SurfaceContent::PixelBuffer(image_buffer), + }); + } + + /// Paint a caller-owned texture into the scene at the current z-index. + /// + /// The renderer keeps one GPU texture per [`crate::ExternalTextureId`] and + /// uploads only what the source reports as dirty, so a source that has + /// stopped producing frames costs a draw call and no bandwidth. + /// + /// This deliberately does **not** go through the sprite atlas. Atlas tiles + /// are for icons and glyphs; a 1080p frame arriving at 60 Hz would evict + /// the entire atlas every frame. See [`crate::external_texture`]. + /// + /// Call only during the paint phase of element drawing. + pub fn paint_external_texture( + &mut self, + bounds: Bounds, + source: Arc, + ) { + use crate::{PaintSurface, SurfaceContent}; + + self.invalidator.debug_assert_paint(); + + let bounds = self.snap_bounds(bounds); + let content_mask = self.snapped_content_mask(); + self.next_frame.scene.insert_primitive(PaintSurface { + order: 0, + bounds, + content_mask, + content: SurfaceContent::ExternalTexture(source), }); } diff --git a/crates/gpui_macos/src/metal_renderer.rs b/crates/gpui_macos/src/metal_renderer.rs index d1f1233efe2f10..0088075811e56b 100644 --- a/crates/gpui_macos/src/metal_renderer.rs +++ b/crates/gpui_macos/src/metal_renderer.rs @@ -1521,35 +1521,43 @@ impl MetalRenderer { ); for surface in surfaces { + // Metal keeps the CoreVideo two-plane path. Caller-owned RGBA + // textures (Chromium off-screen rendering) are a Windows/Linux + // path; nothing on macOS produces one, and silently skipping is + // better than sampling a YUV shader over BGRA bytes. + let image_buffer = match &surface.content { + gpui::SurfaceContent::PixelBuffer(image_buffer) => image_buffer, + gpui::SurfaceContent::ExternalTexture(_) => continue, + }; let texture_size = size( - DevicePixels::from(surface.image_buffer.get_width() as i32), - DevicePixels::from(surface.image_buffer.get_height() as i32), + DevicePixels::from(image_buffer.get_width() as i32), + DevicePixels::from(image_buffer.get_height() as i32), ); assert_eq!( - surface.image_buffer.get_pixel_format(), + image_buffer.get_pixel_format(), kCVPixelFormatType_420YpCbCr8BiPlanarFullRange ); let y_texture = self .core_video_texture_cache .create_texture_from_image( - surface.image_buffer.as_concrete_TypeRef(), + image_buffer.as_concrete_TypeRef(), None, MTLPixelFormat::R8Unorm, - surface.image_buffer.get_width_of_plane(0), - surface.image_buffer.get_height_of_plane(0), + image_buffer.get_width_of_plane(0), + image_buffer.get_height_of_plane(0), 0, ) .unwrap(); let cb_cr_texture = self .core_video_texture_cache .create_texture_from_image( - surface.image_buffer.as_concrete_TypeRef(), + image_buffer.as_concrete_TypeRef(), None, MTLPixelFormat::RG8Unorm, - surface.image_buffer.get_width_of_plane(1), - surface.image_buffer.get_height_of_plane(1), + image_buffer.get_width_of_plane(1), + image_buffer.get_height_of_plane(1), 1, ) .unwrap(); diff --git a/crates/gpui_wgpu/src/shaders.wgsl b/crates/gpui_wgpu/src/shaders.wgsl index 933b88e84d79a9..ec298d6c5263cc 100644 --- a/crates/gpui_wgpu/src/shaders.wgsl +++ b/crates/gpui_wgpu/src/shaders.wgsl @@ -1362,3 +1362,57 @@ fn fs_surface(input: SurfaceVarying) -> @location(0) vec4 { return ycbcr_to_RGB * y_cb_cr; } + +// --- external textures --- // +// +// A caller-owned RGBA/BGRA texture composited inside the scene, used by the +// embedded browser's off-screen Chromium frames. Unlike polychrome sprites +// this never samples the shared sprite atlas: the texture bound at +// group(1) binding(1) belongs to one producer and holds exactly one frame, +// so a 1080p page cannot evict every icon in the app. + +struct ExternalTexture { + bounds: Bounds, + content_mask: Bounds, + opacity: f32, + pad: u32, +} +@group(1) @binding(0) var b_external_textures: array; + +struct ExternalTextureVarying { + @builtin(position) position: vec4, + @location(0) texture_position: vec2, + @location(1) @interpolate(flat) external_id: u32, + @location(2) clip_distances: vec4, +} + +@vertex +fn vs_external_texture(@builtin(vertex_index) vertex_id: u32, @builtin(instance_index) instance_id: u32) -> ExternalTextureVarying { + let unit_vertex = vec2(f32(vertex_id & 1u), 0.5 * f32(vertex_id & 2u)); + // Not named `external`: that is a reserved keyword in WGSL, and naga + // rejects the whole shader module for it. The failure is not local to this + // quad either, because every GPUI shader is compiled as one module, so the + // app panics before it can draw anything at all. + let quad = b_external_textures[instance_id]; + + var out = ExternalTextureVarying(); + out.position = to_device_position(unit_vertex, quad.bounds); + // The texture holds exactly this rect, so unit coordinates are the + // texture coordinates. No atlas tile arithmetic. + out.texture_position = unit_vertex; + out.external_id = instance_id; + out.clip_distances = distance_from_clip_rect(unit_vertex, quad.bounds, quad.content_mask); + return out; +} + +@fragment +fn fs_external_texture(input: ExternalTextureVarying) -> @location(0) vec4 { + let sample = textureSample(t_sprite, s_sprite, input.texture_position); + // Alpha clip after using the derivatives. + if (any(input.clip_distances < vec4(0.0))) { + return vec4(0.0); + } + + let quad = b_external_textures[input.external_id]; + return blend_color(sample, quad.opacity); +} diff --git a/crates/gpui_wgpu/src/wgpu_renderer.rs b/crates/gpui_wgpu/src/wgpu_renderer.rs index 08f30dc0090d3a..412ee7c9b0ca0c 100644 --- a/crates/gpui_wgpu/src/wgpu_renderer.rs +++ b/crates/gpui_wgpu/src/wgpu_renderer.rs @@ -1,14 +1,17 @@ use crate::{CompositorGpuHint, WgpuAtlas, WgpuContext}; use bytemuck::{Pod, Zeroable}; use gpui::{ - AtlasTextureId, Background, Bounds, DevicePixels, GpuSpecs, MonochromeSprite, Path, Point, - PolychromeSprite, PrimitiveBatch, Quad, ScaledPixels, Scene, Shadow, Size, SubpixelSprite, - Underline, get_gamma_correction_ratios, + AtlasTextureId, Background, Bounds, DevicePixels, ExternalTextureCacheKey, + ExternalTextureFormat, ExternalTextureId, ExternalTextureUpdate, GpuSpecs, MonochromeSprite, + PaintSurface, Path, Point, PolychromeSprite, PrimitiveBatch, Quad, ScaledPixels, Scene, Shadow, + Size, SubpixelSprite, Underline, get_gamma_correction_ratios, + plan_external_texture_update, }; use log::warn; #[cfg(not(target_family = "wasm"))] use raw_window_handle::{HasDisplayHandle, HasWindowHandle}; use std::cell::RefCell; +use std::collections::HashMap; use std::num::NonZeroU64; use std::rc::Rc; use std::sync::{Arc, Mutex}; @@ -37,6 +40,25 @@ impl From> for PodBounds { } } +/// One external-texture draw. Mirrors `ExternalTexture` in `shaders.wgsl`; +/// changing either side without the other silently corrupts the geometry. +#[repr(C)] +#[derive(Clone, Copy, Pod, Zeroable)] +struct ExternalTextureInstance { + bounds: PodBounds, + content_mask: PodBounds, + opacity: f32, + pad: u32, +} + +/// A GPU texture owned by one external source, kept across frames so a page +/// that is not repainting costs no upload bandwidth (PERF-B04). +struct CachedExternalTexture { + texture: wgpu::Texture, + view: wgpu::TextureView, + key: ExternalTextureCacheKey, +} + #[repr(C)] #[derive(Clone, Copy, Pod, Zeroable)] struct SurfaceParams { @@ -92,6 +114,7 @@ struct WgpuPipelines { poly_sprites: wgpu::RenderPipeline, #[allow(dead_code)] surfaces: wgpu::RenderPipeline, + external_textures: wgpu::RenderPipeline, } struct WgpuBindGroupLayouts { @@ -120,6 +143,10 @@ struct WgpuResources { path_intermediate_view: Option, path_msaa_texture: Option, path_msaa_view: Option, + /// One cached texture per external source. Interior mutability because the + /// draw path holds `&self` while recording a render pass, and a device loss + /// drops the whole `WgpuResources`, which is exactly when these must go. + external_textures: RefCell>, } impl WgpuResources { @@ -463,6 +490,7 @@ impl WgpuRenderer { path_intermediate_view: None, path_msaa_texture: None, path_msaa_view: None, + external_textures: RefCell::new(HashMap::new()), }; Ok(Self { @@ -865,6 +893,7 @@ impl WgpuRenderer { &shader_module, ); + let external_color_target = color_target.clone(); let surfaces = create_pipeline( "surfaces", "vs_surface", @@ -877,6 +906,22 @@ impl WgpuRenderer { &shader_module, ); + // Deliberately built on `instances_with_texture`, the same layout the + // atlas sprites use, but bound to a caller-owned texture rather than an + // atlas page. See `external_texture.rs` in gpui for why atlasing a live + // browser frame is not an option. + let external_textures = create_pipeline( + "external_textures", + "vs_external_texture", + "fs_external_texture", + &layouts.globals, + &layouts.instances_with_texture, + wgpu::PrimitiveTopology::TriangleStrip, + &[Some(external_color_target)], + 1, + &shader_module, + ); + WgpuPipelines { quads, shadows, @@ -887,6 +932,7 @@ impl WgpuRenderer { subpixel_sprites, poly_sprites, surfaces, + external_textures, } } @@ -1300,11 +1346,11 @@ impl WgpuRenderer { &mut instance_offset, &mut pass, ), - PrimitiveBatch::Surfaces(_surfaces) => { - // Surfaces are macOS-only for video playback - // Not implemented for Linux/wgpu - true - } + PrimitiveBatch::Surfaces(range) => self.draw_surfaces( + &scene.surfaces[range], + &mut instance_offset, + &mut pass, + ), }; if !ok { overflow = true; @@ -1446,6 +1492,196 @@ impl WgpuRenderer { ) } + /// Composite caller-owned textures (embedded browser frames) into the + /// scene. + /// + /// CoreVideo surfaces are a macOS path and never reach here; on Linux a + /// `PaintSurface` is always an external texture. + fn draw_surfaces( + &self, + surfaces: &[PaintSurface], + instance_offset: &mut u64, + pass: &mut wgpu::RenderPass<'_>, + ) -> bool { + for surface in surfaces { + let Some(source) = surface.content.external_texture() else { + continue; + }; + let mut ok = true; + source.with_frame(&mut |frame| { + if !frame.is_well_formed() { + // A producer that mis-declares its geometry is a bug, but + // it must not become an out-of-bounds GPU read. + log::error!( + "external texture {:?} offered a malformed frame ({:?}, stride {})", + source.id(), + frame.size, + frame.stride + ); + return; + } + + let cached_key = self + .resources() + .external_textures + .borrow() + .get(&source.id()) + .map(|cached| cached.key); + let plan = plan_external_texture_update(cached_key, &frame); + + if !matches!(plan, ExternalTextureUpdate::Reuse) { + if !self.upload_external_texture(source.id(), &frame, plan) { + ok = false; + return; + } + source.mark_uploaded(frame.sequence); + } + + let instances = [ExternalTextureInstance { + bounds: surface.bounds.into(), + content_mask: surface.content_mask.bounds.into(), + opacity: 1.0, + pad: 0, + }]; + let data = unsafe { Self::instance_bytes(&instances) }; + + let textures = self.resources().external_textures.borrow(); + let Some(cached) = textures.get(&source.id()) else { + return; + }; + ok = self.draw_instances_with_texture( + data, + 1, + &cached.view, + &self.resources().pipelines.external_textures, + instance_offset, + pass, + ); + }); + if !ok { + return false; + } + } + true + } + + /// Create or refresh the cached texture for one external source. Returns + /// false only when the frame cannot be represented at all. + fn upload_external_texture( + &self, + id: ExternalTextureId, + frame: &gpui::ExternalFrameView<'_>, + plan: ExternalTextureUpdate, + ) -> bool { + let resources = self.resources(); + let format = match frame.format { + ExternalTextureFormat::Bgra8 => wgpu::TextureFormat::Bgra8Unorm, + ExternalTextureFormat::Rgba8 => wgpu::TextureFormat::Rgba8Unorm, + }; + let width = frame.size.width.0.max(0) as u32; + let height = frame.size.height.0.max(0) as u32; + if width == 0 || height == 0 { + return true; + } + + let mut textures = resources.external_textures.borrow_mut(); + if matches!(plan, ExternalTextureUpdate::Recreate) { + let texture = resources.device.create_texture(&wgpu::TextureDescriptor { + label: Some("external_texture"), + size: wgpu::Extent3d { + width, + height, + depth_or_array_layers: 1, + }, + mip_level_count: 1, + sample_count: 1, + dimension: wgpu::TextureDimension::D2, + format, + usage: wgpu::TextureUsages::TEXTURE_BINDING | wgpu::TextureUsages::COPY_DST, + view_formats: &[], + }); + let view = texture.create_view(&wgpu::TextureViewDescriptor::default()); + textures.insert( + id, + CachedExternalTexture { + texture, + view, + key: ExternalTextureCacheKey { + id, + generation: frame.generation, + sequence: 0, + size: frame.size, + }, + }, + ); + } + + let Some(cached) = textures.get_mut(&id) else { + return false; + }; + + // A recreated texture has no pixels yet, so it takes the whole frame + // regardless of what the producer reported as dirty. + let regions = if matches!(plan, ExternalTextureUpdate::Recreate) { + vec![Bounds { + origin: gpui::point(DevicePixels(0), DevicePixels(0)), + size: frame.size, + }] + } else { + frame.dirty_regions() + }; + + for region in regions { + let x = region.origin.x.0.max(0) as u32; + let y = region.origin.y.0.max(0) as u32; + let region_width = region.size.width.0.max(0) as u32; + let region_height = region.size.height.0.max(0) as u32; + if region_width == 0 || region_height == 0 { + continue; + } + let offset = y as usize * frame.stride + x as usize * 4; + // `write_texture` reads `region_height` rows of `region_width * 4` + // bytes at `frame.stride` pitch starting here. A slice shorter than + // that is a wgpu validation panic, so the whole span is checked + // rather than just its first byte. + let span_end = (y + region_height - 1) as usize * frame.stride + + (x + region_width) as usize * 4; + if span_end > frame.bytes.len() { + log::error!( + "external texture {id:?} reported a dirty region outside its own buffer" + ); + continue; + } + resources.queue.write_texture( + wgpu::TexelCopyTextureInfo { + texture: &cached.texture, + mip_level: 0, + origin: wgpu::Origin3d { x, y, z: 0 }, + aspect: wgpu::TextureAspect::All, + }, + &frame.bytes[offset..], + wgpu::TexelCopyBufferLayout { + offset: 0, + bytes_per_row: Some(frame.stride as u32), + rows_per_image: Some(region_height), + }, + wgpu::Extent3d { + width: region_width, + height: region_height, + depth_or_array_layers: 1, + }, + ); + } + + cached.key = ExternalTextureCacheKey { + id, + generation: frame.generation, + sequence: frame.sequence, + size: frame.size, + }; + true + } + fn draw_instances( &self, data: &[u8], diff --git a/crates/gpui_windows/build.rs b/crates/gpui_windows/build.rs index 1db95715e26daf..95da58cfdeeb02 100644 --- a/crates/gpui_windows/build.rs +++ b/crates/gpui_windows/build.rs @@ -38,6 +38,7 @@ mod shader_compilation { "monochrome_sprite", "subpixel_sprite", "polychrome_sprite", + "external_texture", ]; let rust_binding_path = format!("{}/shaders_bytes.rs", out_dir); diff --git a/crates/gpui_windows/src/directx_renderer.rs b/crates/gpui_windows/src/directx_renderer.rs index 03666241e674d5..bd08cf40b32ebc 100644 --- a/crates/gpui_windows/src/directx_renderer.rs +++ b/crates/gpui_windows/src/directx_renderer.rs @@ -1,4 +1,5 @@ use std::{ + collections::HashMap, slice, sync::{Arc, OnceLock}, }; @@ -44,6 +45,10 @@ pub(crate) struct DirectXRenderer { pipelines: DirectXRenderPipelines, direct_composition: Option, font_info: &'static FontInfo, + /// One cached D3D texture per external source (embedded browser panes), so + /// a page that is not repainting costs a draw call and no upload + /// (PERF-B04). Dropped with the device on a device-lost recovery. + external_textures: HashMap, width: u32, height: u32, @@ -90,6 +95,25 @@ struct DirectXRenderPipelines { mono_sprites: PipelineState, subpixel_sprites: PipelineState, poly_sprites: PipelineState, + external_textures: PipelineState, +} + +/// One external-texture draw. Mirrors `ExternalTexture` in `shaders.hlsl`; +/// changing either side without the other silently corrupts the geometry. +#[derive(Clone, Copy)] +#[repr(C)] +struct ExternalTextureInstance { + bounds: Bounds, + content_mask: Bounds, + opacity: f32, + pad: u32, +} + +/// A GPU texture owned by one external source, kept across frames. +struct CachedExternalTexture { + texture: ID3D11Texture2D, + view: Option, + key: ExternalTextureCacheKey, } struct DirectXGlobalElements { @@ -171,6 +195,7 @@ impl DirectXRenderer { pipelines, direct_composition, font_info: Self::get_font_info(), + external_textures: HashMap::new(), width: 1, height: 1, skip_draws: false, @@ -259,6 +284,8 @@ impl DirectXRenderer { self.direct_composition.take(); self.devices.take(); } + // These hold textures created on the dead device. + self.external_textures.clear(); let devices = DirectXRendererDevices::new(directx_devices, disable_direct_composition) .context("Recreating DirectX devices")?; @@ -697,13 +724,213 @@ impl DirectXRenderer { ) } + /// Composite caller-owned textures (embedded browser frames) into the + /// scene. + /// + /// CoreVideo surfaces are a macOS path and never reach here; on Windows a + /// `PaintSurface` is always an external texture. fn draw_surfaces(&mut self, surfaces: &[PaintSurface]) -> Result<()> { if surfaces.is_empty() { return Ok(()); } + for surface in surfaces { + let Some(source) = surface.content.external_texture() else { + continue; + }; + let mut result = Ok(()); + source.with_frame(&mut |frame| { + if !frame.is_well_formed() { + // A producer that mis-declares its geometry is a bug, but it + // must not become an out-of-bounds read. + log::error!( + "external texture {:?} offered a malformed frame ({:?}, stride {})", + source.id(), + frame.size, + frame.stride + ); + return; + } + match self.draw_external_frame(surface, source.id(), &frame) { + // Only an actual upload is acknowledged. Acknowledging a + // reuse would make `upload_count` report the compositing + // rate, and PERF-B04 reads that counter to prove a settled + // page stops uploading. + Ok(uploaded) => { + if uploaded { + source.mark_uploaded(frame.sequence); + } + } + Err(error) => result = Err(error), + } + }); + result?; + } Ok(()) } + /// Upload what changed and draw one external texture. + /// + /// Returns whether pixels were actually sent to the GPU, so the caller can + /// acknowledge an upload and not a reuse. + fn draw_external_frame( + &mut self, + surface: &PaintSurface, + id: ExternalTextureId, + frame: &ExternalFrameView<'_>, + ) -> Result { + let width = frame.size.width.0.max(0) as u32; + let height = frame.size.height.0.max(0) as u32; + if width == 0 || height == 0 { + return Ok(false); + } + + let plan = plan_external_texture_update( + self.external_textures.get(&id).map(|cached| cached.key), + frame, + ); + + { + let devices = self.devices.as_ref().context("devices missing")?; + if matches!(plan, ExternalTextureUpdate::Recreate) { + let format = match frame.format { + ExternalTextureFormat::Bgra8 => DXGI_FORMAT_B8G8R8A8_UNORM, + ExternalTextureFormat::Rgba8 => DXGI_FORMAT_R8G8B8A8_UNORM, + }; + let desc = D3D11_TEXTURE2D_DESC { + Width: width, + Height: height, + MipLevels: 1, + ArraySize: 1, + Format: format, + SampleDesc: DXGI_SAMPLE_DESC { + Count: 1, + Quality: 0, + }, + Usage: D3D11_USAGE_DEFAULT, + BindFlags: D3D11_BIND_SHADER_RESOURCE.0 as u32, + CPUAccessFlags: 0, + MiscFlags: 0, + }; + let mut texture = None; + unsafe { devices.device.CreateTexture2D(&desc, None, Some(&mut texture)) } + .context("Creating an external browser texture")?; + let texture = texture.context("CreateTexture2D returned nothing")?; + let mut view = None; + unsafe { + devices + .device + .CreateShaderResourceView(&texture, None, Some(&mut view)) + } + .context("Creating an external browser texture view")?; + self.external_textures.insert( + id, + CachedExternalTexture { + texture, + view, + key: ExternalTextureCacheKey { + id, + generation: frame.generation, + sequence: 0, + size: frame.size, + }, + }, + ); + } + + if !matches!(plan, ExternalTextureUpdate::Reuse) { + let cached = self + .external_textures + .get_mut(&id) + .context("external texture went missing between create and upload")?; + // A recreated texture has no pixels yet, so it takes the whole + // frame regardless of what the producer reported as dirty. + let regions = if matches!(plan, ExternalTextureUpdate::Recreate) { + vec![Bounds { + origin: point(DevicePixels(0), DevicePixels(0)), + size: frame.size, + }] + } else { + frame.dirty_regions() + }; + for region in regions { + let x = region.origin.x.0.max(0) as u32; + let y = region.origin.y.0.max(0) as u32; + let region_width = region.size.width.0.max(0) as u32; + let region_height = region.size.height.0.max(0) as u32; + if region_width == 0 || region_height == 0 { + continue; + } + let offset = y as usize * frame.stride + x as usize * 4; + let last_row = (y + region_height - 1) as usize * frame.stride + + (x + region_width) as usize * 4; + if last_row > frame.bytes.len() { + continue; + } + let destination = D3D11_BOX { + left: x, + top: y, + front: 0, + right: x + region_width, + bottom: y + region_height, + back: 1, + }; + // SAFETY: `destination` is clipped to the texture by + // `dirty_regions`, and the bounds check above proves the + // source rows for that box are inside `frame.bytes`, which + // stays alive for this call because the producer holds its + // lock for the duration of `with_frame`. + unsafe { + devices.device_context.UpdateSubresource( + &cached.texture, + 0, + Some(&destination), + frame.bytes[offset..].as_ptr() as *const _, + frame.stride as u32, + 0, + ); + } + } + cached.key = ExternalTextureCacheKey { + id, + generation: frame.generation, + sequence: frame.sequence, + size: frame.size, + }; + } + } + + let instance = ExternalTextureInstance { + bounds: surface.bounds, + content_mask: surface.content_mask.bounds, + opacity: 1.0, + pad: 0, + }; + let devices = self.devices.as_ref().context("devices missing")?; + let resources = self.resources.as_ref().context("resources missing")?; + let view = self + .external_textures + .get(&id) + .context("external texture missing at draw time")? + .view + .clone(); + self.pipelines.external_textures.update_buffer( + &devices.device, + &devices.device_context, + slice::from_ref(&instance), + )?; + self.pipelines.external_textures.draw_range_with_texture( + &devices.device, + &devices.device_context, + slice::from_ref(&view), + slice::from_ref(&resources.viewport), + slice::from_ref(&self.globals.global_params_buffer), + slice::from_ref(&self.globals.sampler), + 0, + 1, + )?; + Ok(!matches!(plan, ExternalTextureUpdate::Reuse)) + } + pub(crate) fn gpu_specs(&self) -> Result { let devices = self.devices.as_ref().context("devices missing")?; let desc = unsafe { devices.adapter.GetDesc1() }?; @@ -881,6 +1108,15 @@ impl DirectXRenderPipelines { 16, create_blend_state(device)?, )?; + // One instance per visible browser pane; the 8-pane cap (PRD) is the + // real bound, so this starts small and grows if it ever needs to. + let external_textures = PipelineState::new( + device, + "external_texture_pipeline", + ShaderModule::ExternalTexture, + 8, + create_blend_state(device)?, + )?; Ok(Self { shadow_pipeline, @@ -891,6 +1127,7 @@ impl DirectXRenderPipelines { mono_sprites, subpixel_sprites, poly_sprites, + external_textures, }) } } @@ -1603,6 +1840,7 @@ pub(crate) mod shader_resources { MonochromeSprite, SubpixelSprite, PolychromeSprite, + ExternalTexture, EmojiRasterization, } @@ -1677,6 +1915,10 @@ pub(crate) mod shader_resources { ShaderTarget::Vertex => POLYCHROME_SPRITE_VERTEX_BYTES, ShaderTarget::Fragment => POLYCHROME_SPRITE_FRAGMENT_BYTES, }, + ShaderModule::ExternalTexture => match target { + ShaderTarget::Vertex => EXTERNAL_TEXTURE_VERTEX_BYTES, + ShaderTarget::Fragment => EXTERNAL_TEXTURE_FRAGMENT_BYTES, + }, ShaderModule::EmojiRasterization => match target { ShaderTarget::Vertex => EMOJI_RASTERIZATION_VERTEX_BYTES, ShaderTarget::Fragment => EMOJI_RASTERIZATION_FRAGMENT_BYTES, @@ -1767,6 +2009,7 @@ pub(crate) mod shader_resources { ShaderModule::MonochromeSprite => "monochrome_sprite", ShaderModule::SubpixelSprite => "subpixel_sprite", ShaderModule::PolychromeSprite => "polychrome_sprite", + ShaderModule::ExternalTexture => "external_texture", ShaderModule::EmojiRasterization => "emoji_rasterization", } } diff --git a/crates/gpui_windows/src/shaders.hlsl b/crates/gpui_windows/src/shaders.hlsl index 89c12489b24cf6..88329e90fca294 100644 --- a/crates/gpui_windows/src/shaders.hlsl +++ b/crates/gpui_windows/src/shaders.hlsl @@ -1256,3 +1256,58 @@ float4 polychrome_sprite_fragment(PolychromeSpriteFragmentInput input): SV_Targe color.a *= sprite.opacity * saturate(0.5 - distance); return color; } + +/* +** +** External textures +** +*/ +// A caller-owned RGBA/BGRA texture composited inside the scene, used by the +// embedded browser's off-screen Chromium frames. Unlike a polychrome sprite +// this samples a texture that belongs to one producer and holds exactly one +// frame, so a 1080p page never touches the shared sprite atlas. + +struct ExternalTexture { + Bounds bounds; + Bounds content_mask; + float opacity; + uint pad; +}; +StructuredBuffer external_textures: register(t1); + +struct ExternalTextureVertexOutput { + nointerpolation uint external_id: TEXCOORD0; + float4 position: SV_Position; + float2 texture_position: POSITION; + float4 clip_distance: SV_ClipDistance; +}; + +struct ExternalTextureFragmentInput { + nointerpolation uint external_id: TEXCOORD0; + float4 position: SV_Position; + float2 texture_position: POSITION; +}; + +ExternalTextureVertexOutput external_texture_vertex(uint vertex_id: SV_VertexID, uint external_id: SV_InstanceID) { + float2 unit_vertex = float2(float(vertex_id & 1u), 0.5 * float(vertex_id & 2u)); + ExternalTexture external = external_textures[external_id]; + float4 device_position = to_device_position(unit_vertex, external.bounds); + float4 clip_distance = distance_from_clip_rect(unit_vertex, external.bounds, + external.content_mask); + + ExternalTextureVertexOutput output; + output.position = device_position; + // The texture holds exactly this rect, so unit coordinates are the texture + // coordinates. No atlas tile arithmetic. + output.texture_position = unit_vertex; + output.external_id = external_id; + output.clip_distance = clip_distance; + return output; +} + +float4 external_texture_fragment(ExternalTextureFragmentInput input): SV_Target { + ExternalTexture external = external_textures[input.external_id]; + float4 color = t_sprite.Sample(s_sprite, input.texture_position); + color.a *= external.opacity; + return color; +} From f3d2e6584d9a31518a6ec06ebc414da8fb7397c2 Mon Sep 17 00:00:00 2001 From: soyboyscout Date: Sat, 5 Sep 2026 09:01:27 -0400 Subject: [PATCH 2/5] Harden external browser frame ownership and rendering --- crates/gpui/src/external_texture.rs | 197 +++++++++++++++++++- crates/gpui_wgpu/src/shaders.wgsl | 5 +- crates/gpui_wgpu/src/wgpu_renderer.rs | 38 +++- crates/gpui_windows/src/directx_renderer.rs | 21 ++- 4 files changed, 237 insertions(+), 24 deletions(-) diff --git a/crates/gpui/src/external_texture.rs b/crates/gpui/src/external_texture.rs index 45e2c99864d37b..c216a4b4b6e3fc 100644 --- a/crates/gpui/src/external_texture.rs +++ b/crates/gpui/src/external_texture.rs @@ -113,10 +113,16 @@ impl ExternalFrameView<'_> { if width == 0 || height == 0 { return false; } - if self.stride < width * self.format.bytes_per_pixel() { + let Some(row_bytes) = width.checked_mul(self.format.bytes_per_pixel()) else { return false; - } - self.bytes.len() >= self.stride * height + }; + // Both native upload APIs represent the row pitch as u32. + self.stride >= row_bytes + && u32::try_from(self.stride).is_ok() + && self + .stride + .checked_mul(height) + .is_some_and(|needed| needed <= self.bytes.len()) } /// The dirty regions, clamped to the buffer, with an empty list meaning the @@ -129,13 +135,38 @@ impl ExternalFrameView<'_> { if self.dirty.is_empty() { return vec![full]; } - self.dirty + let regions: Vec<_> = self + .dirty .iter() .filter_map(|rect| { - let clamped = rect.intersect(&full); - (!clamped.is_empty()).then_some(clamped) + // External coordinates are untrusted. Widen before adding so + // malformed i32 endpoints cannot wrap into a native GPU box. + if rect.size.width.0 <= 0 || rect.size.height.0 <= 0 { + return None; + } + let left = i64::from(rect.origin.x.0).clamp(0, i64::from(self.size.width.0.max(0))); + let top = i64::from(rect.origin.y.0).clamp(0, i64::from(self.size.height.0.max(0))); + let right = (i64::from(rect.origin.x.0) + i64::from(rect.size.width.0)) + .clamp(left, i64::from(self.size.width.0.max(0))); + let bottom = (i64::from(rect.origin.y.0) + i64::from(rect.size.height.0)) + .clamp(top, i64::from(self.size.height.0.max(0))); + (right > left && bottom > top).then_some(Bounds { + origin: crate::point(DevicePixels(left as i32), DevicePixels(top as i32)), + size: crate::size( + DevicePixels((right - left) as i32), + DevicePixels((bottom - top) as i32), + ), + }) }) - .collect() + .collect(); + // Dirty rectangles are hints about a complete frame. If every hint + // falls outside the current texture, acknowledging the sequence with + // no upload would leave stale pixels cached indefinitely. + if regions.is_empty() { + vec![full] + } else { + regions + } } } @@ -252,6 +283,11 @@ impl ExternalTextureBuffer { self.sequence.load(Ordering::Acquire) } + /// Last frame committed by a renderer. CPU submissions do not advance it. + pub fn uploaded_sequence(&self) -> u64 { + self.acknowledged.load(Ordering::Acquire) + } + /// Current buffer dimensions, or zero before the first frame. pub fn size(&self) -> Size { self.state.lock().size @@ -277,7 +313,13 @@ impl ExternalTextureBuffer { ) -> bool { let width = size.width.0.max(0) as usize; let height = size.height.0.max(0) as usize; - if width == 0 || height == 0 || stride < width * format.bytes_per_pixel() { + if width == 0 + || height == 0 + || u32::try_from(stride).is_err() + || width + .checked_mul(format.bytes_per_pixel()) + .is_none_or(|minimum| stride < minimum) + { return false; } let needed = stride @@ -312,13 +354,22 @@ impl ExternalTextureBuffer { // The renderer has not consumed the previous frame, so this frame's // dirt is added to it rather than replacing it. Dropping it would // leave half the page showing stale pixels. - state.dirty.extend_from_slice(dirty); + // Empty means a full repaint. It must dominate in either order; + // appending a small rectangle to it would lose that full repaint. + // Bound the list when a hidden/slow consumer misses many frames. + if dirty.is_empty() || state.dirty.len().saturating_add(dirty.len()) > 64 { + state.dirty.clear(); + } else if !state.dirty.is_empty() { + state.dirty.extend_from_slice(dirty); + } } else { // Everything up to here has been uploaded, so the previous rects // are spent. This is also where an acknowledgement's clean-up // happens, which is why `mark_uploaded` does not need the lock. state.dirty.clear(); - state.dirty.extend_from_slice(dirty); + if dirty.len() <= 64 { + state.dirty.extend_from_slice(dirty); + } } state.sequence += 1; @@ -868,4 +919,130 @@ mod tests { buffer.with_frame(&mut |_| seen += 1); assert_eq!(seen, 0); } + #[test] + fn full_repaint_dominates_partial_frames_in_both_orders() { + for full_first in [true, false] { + let buffer = ExternalTextureBuffer::new(); + let pixels = bgra(8, 8, 1); + buffer.submit(dims(8, 8), 32, ExternalTextureFormat::Bgra8, &pixels, &[]); + buffer.with_frame(&mut |frame| buffer.mark_uploaded(frame.sequence)); + let partial = [Bounds { + origin: point(DevicePixels(1), DevicePixels(1)), + size: dims(2, 2), + }]; + let (first, second): (&[_], &[_]) = if full_first { + (&[], &partial) + } else { + (&partial, &[]) + }; + buffer.submit(dims(8, 8), 32, ExternalTextureFormat::Bgra8, &pixels, first); + buffer.submit( + dims(8, 8), + 32, + ExternalTextureFormat::Bgra8, + &pixels, + second, + ); + buffer.with_frame(&mut |frame| { + assert_eq!( + frame.dirty_regions(), + vec![Bounds { + origin: point(DevicePixels(0), DevicePixels(0)), + size: dims(8, 8) + }] + ); + }); + } + } + + #[test] + fn missed_partial_frames_have_bounded_dirty_bookkeeping() { + let buffer = ExternalTextureBuffer::new(); + let pixels = bgra(8, 8, 1); + buffer.submit(dims(8, 8), 32, ExternalTextureFormat::Bgra8, &pixels, &[]); + buffer.with_frame(&mut |frame| buffer.mark_uploaded(frame.sequence)); + let partial = [Bounds { + origin: point(DevicePixels(1), DevicePixels(1)), + size: dims(2, 2), + }]; + for _ in 0..1000 { + buffer.submit( + dims(8, 8), + 32, + ExternalTextureFormat::Bgra8, + &pixels, + &partial, + ); + } + buffer.with_frame(&mut |frame| assert!(frame.dirty.is_empty())); + } + + #[test] + fn overflowing_frame_geometry_is_rejected() { + let frame = ExternalFrameView { + size: dims(1, 2), + stride: usize::MAX / 2 + 1, + format: ExternalTextureFormat::Bgra8, + bytes: &[], + generation: 1, + sequence: 1, + dirty: &[], + }; + assert!(!frame.is_well_formed()); + } + + #[test] + fn dirty_rectangles_cannot_overflow_when_clipped() { + let dirty = [Bounds { + origin: point(DevicePixels(i32::MAX - 1), DevicePixels(0)), + size: dims(8, 8), + }]; + let pixels = bgra(8, 8, 0); + let frame = ExternalFrameView { + size: dims(8, 8), + stride: 32, + format: ExternalTextureFormat::Bgra8, + bytes: &pixels, + generation: 1, + sequence: 1, + dirty: &dirty, + }; + assert_eq!( + frame.dirty_regions(), + vec![Bounds { + origin: point(DevicePixels(0), DevicePixels(0)), + size: dims(8, 8), + }] + ); + } + + #[test] + fn unusable_dirty_hints_cannot_acknowledge_stale_pixels() { + let buffer = ExternalTextureBuffer::new(); + let mut texture = bgra(4, 4, 0); + buffer.submit(dims(4, 4), 16, ExternalTextureFormat::Bgra8, &texture, &[]); + buffer.with_frame(&mut |frame| buffer.mark_uploaded(frame.sequence)); + let next = bgra(4, 4, 17); + let dirty = [Bounds { + origin: point(DevicePixels(100), DevicePixels(100)), + size: dims(1, 1), + }]; + buffer.submit(dims(4, 4), 16, ExternalTextureFormat::Bgra8, &next, &dirty); + buffer.with_frame(&mut |frame| { + for region in frame.dirty_regions() { + let left = region.origin.x.0 as usize * 4; + let right = left + region.size.width.0 as usize * 4; + for y in region.origin.y.0..region.origin.y.0 + region.size.height.0 { + let row = y as usize * frame.stride; + texture[row + left..row + right] + .copy_from_slice(&frame.bytes[row + left..row + right]); + } + } + buffer.mark_uploaded(frame.sequence); + }); + assert_eq!( + texture, next, + "an acknowledged frame must update the texture" + ); + } } diff --git a/crates/gpui_wgpu/src/shaders.wgsl b/crates/gpui_wgpu/src/shaders.wgsl index ec298d6c5263cc..ed300d8d29d8bf 100644 --- a/crates/gpui_wgpu/src/shaders.wgsl +++ b/crates/gpui_wgpu/src/shaders.wgsl @@ -1375,7 +1375,7 @@ struct ExternalTexture { bounds: Bounds, content_mask: Bounds, opacity: f32, - pad: u32, + swap_red_blue: u32, } @group(1) @binding(0) var b_external_textures: array; @@ -1414,5 +1414,6 @@ fn fs_external_texture(input: ExternalTextureVarying) -> @location(0) vec4 } let quad = b_external_textures[input.external_id]; - return blend_color(sample, quad.opacity); + let color = select(sample, sample.bgra, quad.swap_red_blue != 0u); + return blend_color(color, quad.opacity); } diff --git a/crates/gpui_wgpu/src/wgpu_renderer.rs b/crates/gpui_wgpu/src/wgpu_renderer.rs index 412ee7c9b0ca0c..76b33da99cdc01 100644 --- a/crates/gpui_wgpu/src/wgpu_renderer.rs +++ b/crates/gpui_wgpu/src/wgpu_renderer.rs @@ -4,8 +4,7 @@ use gpui::{ AtlasTextureId, Background, Bounds, DevicePixels, ExternalTextureCacheKey, ExternalTextureFormat, ExternalTextureId, ExternalTextureUpdate, GpuSpecs, MonochromeSprite, PaintSurface, Path, Point, PolychromeSprite, PrimitiveBatch, Quad, ScaledPixels, Scene, Shadow, - Size, SubpixelSprite, Underline, get_gamma_correction_ratios, - plan_external_texture_update, + Size, SubpixelSprite, Underline, get_gamma_correction_ratios, plan_external_texture_update, }; use log::warn; #[cfg(not(target_family = "wasm"))] @@ -48,7 +47,7 @@ struct ExternalTextureInstance { bounds: PodBounds, content_mask: PodBounds, opacity: f32, - pad: u32, + swap_red_blue: u32, } /// A GPU texture owned by one external source, kept across frames so a page @@ -147,6 +146,7 @@ struct WgpuResources { /// draw path holds `&self` while recording a render pass, and a device loss /// drops the whole `WgpuResources`, which is exactly when these must go. external_textures: RefCell>, + external_texture_format: wgpu::TextureFormat, } impl WgpuResources { @@ -491,6 +491,7 @@ impl WgpuRenderer { path_msaa_texture: None, path_msaa_view: None, external_textures: RefCell::new(HashMap::new()), + external_texture_format: context.color_texture_format(), }; Ok(Self { @@ -1126,6 +1127,18 @@ impl WgpuRenderer { } pub fn draw(&mut self, scene: &Scene) -> bool { + // Drop GPU resources for sources absent from the complete next scene. + if let Some(resources) = self.resources.as_mut() { + resources.external_textures.get_mut().retain(|id, _| { + scene.surfaces.iter().any(|surface| { + surface + .content + .external_texture() + .is_some_and(|source| source.id() == *id) + }) + }); + } + // Bail out early if the surface has been unconfigured (e.g. during // Android background/rotation transitions). Attempting to acquire // a texture from an unconfigured surface can block indefinitely on @@ -1541,8 +1554,14 @@ impl WgpuRenderer { bounds: surface.bounds.into(), content_mask: surface.content_mask.bounds.into(), opacity: 1.0, - pad: 0, + swap_red_blue: u32::from( + matches!(frame.format, ExternalTextureFormat::Bgra8) + != (self.resources().external_texture_format + == wgpu::TextureFormat::Bgra8Unorm), + ), }]; + // SAFETY: ExternalTextureInstance is Pod and its layout matches + // the ExternalTexture storage-buffer element in shaders.wgsl. let data = unsafe { Self::instance_bytes(&instances) }; let textures = self.resources().external_textures.borrow(); @@ -1574,10 +1593,9 @@ impl WgpuRenderer { plan: ExternalTextureUpdate, ) -> bool { let resources = self.resources(); - let format = match frame.format { - ExternalTextureFormat::Bgra8 => wgpu::TextureFormat::Bgra8Unorm, - ExternalTextureFormat::Rgba8 => wgpu::TextureFormat::Rgba8Unorm, - }; + // The context validated sampled/copy support for this format. Upload + // producer bytes directly; the draw instance swaps channels if needed. + let format = resources.external_texture_format; let width = frame.size.width.0.max(0) as u32; let height = frame.size.height.0.max(0) as u32; if width == 0 || height == 0 { @@ -1644,8 +1662,8 @@ impl WgpuRenderer { // bytes at `frame.stride` pitch starting here. A slice shorter than // that is a wgpu validation panic, so the whole span is checked // rather than just its first byte. - let span_end = (y + region_height - 1) as usize * frame.stride - + (x + region_width) as usize * 4; + let span_end = + (y + region_height - 1) as usize * frame.stride + (x + region_width) as usize * 4; if span_end > frame.bytes.len() { log::error!( "external texture {id:?} reported a dirty region outside its own buffer" diff --git a/crates/gpui_windows/src/directx_renderer.rs b/crates/gpui_windows/src/directx_renderer.rs index bd08cf40b32ebc..259993c50f7330 100644 --- a/crates/gpui_windows/src/directx_renderer.rs +++ b/crates/gpui_windows/src/directx_renderer.rs @@ -333,6 +333,15 @@ impl DirectXRenderer { scene: &Scene, background_appearance: WindowBackgroundAppearance, ) -> Result<()> { + // Prune once against the complete scene, including frames with no surfaces. + self.external_textures.retain(|id, _| { + scene.surfaces.iter().any(|surface| { + surface + .content + .external_texture() + .is_some_and(|source| source.id() == *id) + }) + }); if self.skip_draws { // skip drawing this frame, we just recovered from a device lost event // and so likely do not have the textures anymore that are required for drawing @@ -812,10 +821,18 @@ impl DirectXRenderer { MiscFlags: 0, }; let mut texture = None; - unsafe { devices.device.CreateTexture2D(&desc, None, Some(&mut texture)) } - .context("Creating an external browser texture")?; + // SAFETY: desc is initialized, dimensions were validated, and the + // output slot lives through the COM call; no initial data is read. + unsafe { + devices + .device + .CreateTexture2D(&desc, None, Some(&mut texture)) + } + .context("Creating an external browser texture")?; let texture = texture.context("CreateTexture2D returned nothing")?; let mut view = None; + // SAFETY: texture is a live COM resource with shader-resource + // binding enabled; the output slot is valid for this call. unsafe { devices .device From 90f4df85d91756422a3572b2c2c4454925a6dfb6 Mon Sep 17 00:00:00 2001 From: dev01prishusoft Date: Fri, 18 Sep 2026 20:32:27 +0530 Subject: [PATCH 3/5] feat(gpui): expose item offset and content height on ListState --- crates/gpui/src/elements/list.rs | 63 ++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/crates/gpui/src/elements/list.rs b/crates/gpui/src/elements/list.rs index 85b38d5234efec..76be4cd09653fc 100644 --- a/crates/gpui/src/elements/list.rs +++ b/crates/gpui/src/elements/list.rs @@ -296,6 +296,7 @@ struct ListItemSummary { height: Pixels, has_focus_handles: bool, has_unknown_height: bool, + unknown_height_count: usize, } #[derive(Clone, Debug, Default, PartialEq, Eq, PartialOrd, Ord)] @@ -685,6 +686,26 @@ impl ListState { None } + /// Top of item `ix` within the list content, and the content height. + /// Items with no known height count as the mean known height, so the + /// numbers stay proportional before every item has been laid out. + pub fn item_top_and_content_height(&self, ix: usize) -> (Pixels, Pixels) { + let state = &*self.0.borrow(); + let total = state.items.summary(); + let known = total.count - total.unknown_height_count; + let mean = if known > 0 { + total.height.0 / known as f32 + } else { + 0. + }; + let mut cursor = state.items.cursor::(()); + let before: ListItemSummary = cursor.summary(&Count(ix), Bias::Right); + ( + px(before.height.0 + mean * before.unknown_height_count as f32), + px(total.height.0 + mean * total.unknown_height_count as f32), + ) + } + /// Call this method when the user starts dragging the scrollbar. /// /// This will prevent the height reported to the scrollbar from changing during the drag @@ -1597,6 +1618,7 @@ impl sum_tree::Item for ListItem { }, has_focus_handles: focus_handle.is_some(), has_unknown_height: size_hint.is_none(), + unknown_height_count: usize::from(size_hint.is_none()), }, ListItem::Measured { size, focus_handle, .. @@ -1607,6 +1629,7 @@ impl sum_tree::Item for ListItem { height: size.height, has_focus_handles: focus_handle.is_some(), has_unknown_height: false, + unknown_height_count: 0, }, } } @@ -1624,6 +1647,7 @@ impl sum_tree::ContextLessSummary for ListItemSummary { self.height += summary.height; self.has_focus_handles |= summary.has_focus_handles; self.has_unknown_height |= summary.has_unknown_height; + self.unknown_height_count += summary.unknown_height_count; } } @@ -1951,6 +1975,45 @@ mod test { assert_eq!(state.item_is_below_viewport(0), Some(false)); } + #[gpui::test] + fn test_item_top_tracks_measured_and_unmeasured_heights(cx: &mut TestAppContext) { + let cx = cx.add_empty_window(); + + struct TestView(ListState); + impl Render for TestView { + fn render(&mut self, _: &mut Window, _: &mut Context) -> impl IntoElement { + list(self.0.clone(), |ix, _, _| { + div().h(px(10. * (ix + 1) as f32)).w_full().into_any() + }) + .w_full() + .h_full() + } + } + + let state = ListState::new(4, crate::ListAlignment::Top, px(0.)).measure_all(); + let view = cx.update(|_, cx| cx.new(|_| TestView(state.clone()))); + cx.draw(point(px(0.), px(0.)), size(px(100.), px(200.)), |_, _| { + view.into_any_element() + }); + assert_eq!(state.item_top_and_content_height(2), (px(30.), px(100.))); + + struct Uniform(ListState); + impl Render for Uniform { + fn render(&mut self, _: &mut Window, _: &mut Context) -> impl IntoElement { + list(self.0.clone(), |_, _, _| div().h(px(50.)).w_full().into_any()) + .w_full() + .h_full() + } + } + let state = ListState::new(10, crate::ListAlignment::Top, px(0.)); + let view = cx.update(|_, cx| cx.new(|_| Uniform(state.clone()))); + cx.draw(point(px(0.), px(0.)), size(px(100.), px(100.)), |_, _| { + view.into_any_element() + }); + // Only the visible items are measured; the rest count as the mean. + assert_eq!(state.item_top_and_content_height(5), (px(250.), px(500.))); + } + #[gpui::test] fn test_measure_all_after_width_change(cx: &mut TestAppContext) { let cx = cx.add_empty_window(); From eb92d599979c1c8745da69f87a8bc5403da7dc68 Mon Sep 17 00:00:00 2001 From: soyboyscout Date: Sun, 20 Sep 2026 17:07:32 -0400 Subject: [PATCH 4/5] fix(gpui): address review findings on external textures Review follow-ups on the external texture primitive, all seven threads from the PR: - The visit no longer holds the producer's lock. `submit` publishes an immutable `Arc` snapshot and `with_frame` clones it out before calling the visitor, so a Chromium paint callback never waits for a GPU upload. `a_visit_does_not_block_the_producer` fails against the previous implementation. - Acknowledgements are per renderer. `ExternalTextureConsumerId` plus a per-consumer table means the first window to draw a shared source can no longer discard dirty regions the second one still needs; `mark_uploaded_for` defaults to `mark_uploaded`, so a single-consumer source keeps the simpler contract. A cached texture retires its consumer on drop, so a closed window, a pruned cache or a recovered device cannot hold the dirty union forever. - `release_frame` drops the spare allocation as well: a hidden pane must not keep an 8 MB framebuffer alive, which is the point of the call. - `paint_external_texture` is documented as unavailable on macOS, records nothing there, warns once, and still runs the paint-phase debug assert. Metal keeps the CoreVideo surface path. - `PaintSurface` carries the element's opacity, and both external-texture backends feed it to their shader instead of hardcoding 1.0, so a pane inside a faded ancestor fades with it. - DirectX binds a dedicated CLAMP sampler for external textures; the shared sampler wraps, which bleeds the opposite edge into a page drawn at a size other than its texture. - Both backends skip a frame past the device's texture limit with a log and no acknowledgement, instead of creating an invalid texture (wgpu) or failing the whole window's frame (Direct3D). - `ListState::item_top_and_content_height` adds the list padding to both the item top and the content height, matching where `List` places the first item and the height it scrolls. The mirrored sequence counter is stored under the state lock so two producers cannot write it out of order, and the staging copy no longer memSets every byte it is about to overwrite. --- crates/gpui/src/elements/list.rs | 89 ++++- crates/gpui/src/external_texture.rs | 369 ++++++++++++++++---- crates/gpui/src/scene.rs | 7 + crates/gpui/src/window.rs | 22 ++ crates/gpui_wgpu/src/wgpu_renderer.rs | 50 ++- crates/gpui_windows/src/directx_renderer.rs | 84 ++++- 6 files changed, 539 insertions(+), 82 deletions(-) diff --git a/crates/gpui/src/elements/list.rs b/crates/gpui/src/elements/list.rs index 76be4cd09653fc..096c0f61457a90 100644 --- a/crates/gpui/src/elements/list.rs +++ b/crates/gpui/src/elements/list.rs @@ -689,8 +689,17 @@ impl ListState { /// Top of item `ix` within the list content, and the content height. /// Items with no known height count as the mean known height, so the /// numbers stay proportional before every item has been laid out. + /// + /// Both numbers are in the list's own content space, padding included: + /// [`List`] places the first item at `padding.top` and the content it + /// scrolls spans both edges, matching [`ListState::scroll`] and + /// [`ListState::is_scrolled_to_end`]. Note that + /// [`ListState::max_offset_for_scrollbar`] and + /// [`ListState::scroll_px_offset_for_scrollbar`] still measure the items + /// without padding, so a caller mixing the two is off by the padding. pub fn item_top_and_content_height(&self, ix: usize) -> (Pixels, Pixels) { let state = &*self.0.borrow(); + let padding = state.last_padding.unwrap_or_default(); let total = state.items.summary(); let known = total.count - total.unknown_height_count; let mean = if known > 0 { @@ -700,9 +709,11 @@ impl ListState { }; let mut cursor = state.items.cursor::(()); let before: ListItemSummary = cursor.summary(&Count(ix), Bias::Right); + let measured_top = px(before.height.0 + mean * before.unknown_height_count as f32); + let measured_total = px(total.height.0 + mean * total.unknown_height_count as f32); ( - px(before.height.0 + mean * before.unknown_height_count as f32), - px(total.height.0 + mean * total.unknown_height_count as f32), + padding.top + measured_top, + measured_total + padding.top + padding.bottom, ) } @@ -2000,9 +2011,11 @@ mod test { struct Uniform(ListState); impl Render for Uniform { fn render(&mut self, _: &mut Window, _: &mut Context) -> impl IntoElement { - list(self.0.clone(), |_, _, _| div().h(px(50.)).w_full().into_any()) - .w_full() - .h_full() + list(self.0.clone(), |_, _, _| { + div().h(px(50.)).w_full().into_any() + }) + .w_full() + .h_full() } } let state = ListState::new(10, crate::ListAlignment::Top, px(0.)); @@ -2014,6 +2027,72 @@ mod test { assert_eq!(state.item_top_and_content_height(5), (px(250.), px(500.))); } + /// An item whose height is not known yet still has to report a top: the + /// mean known height is what keeps a scroll position proportional while a + /// long list is still measuring. + #[gpui::test] + fn test_item_top_extrapolates_unmeasured_items_from_the_mean(cx: &mut TestAppContext) { + let cx = cx.add_empty_window(); + + struct Mixed(ListState); + impl Render for Mixed { + fn render(&mut self, _: &mut Window, _: &mut Context) -> impl IntoElement { + list(self.0.clone(), |ix, _, _| { + // The first two are 20px, the rest would be 40px if they + // were ever measured. + div() + .h(if ix < 2 { px(20.) } else { px(40.) }) + .w_full() + .into_any() + }) + .w_full() + .h_full() + } + } + + // The viewport fits exactly two items, so items 2 and up stay unknown. + let state = ListState::new(6, crate::ListAlignment::Top, px(0.)); + let view = cx.update(|_, cx| cx.new(|_| Mixed(state.clone()))); + cx.draw(point(px(0.), px(0.)), size(px(100.), px(40.)), |_, _| { + view.into_any_element() + }); + + // Two 20px items are known: mean 20. Item 3's top is therefore + // 20 + 20 + 20, not 20 + 20 + 40. + let (top, total) = state.item_top_and_content_height(3); + assert_eq!(top, px(60.)); + assert_eq!(total, px(120.)); + } + + #[gpui::test] + fn test_item_top_includes_list_padding(cx: &mut TestAppContext) { + let cx = cx.add_empty_window(); + + struct Padded(ListState); + impl Render for Padded { + fn render(&mut self, _: &mut Window, _: &mut Context) -> impl IntoElement { + list(self.0.clone(), |_, _, _| { + div().h(px(20.)).w_full().into_any() + }) + .w_full() + .h_full() + .p(px(7.)) + } + } + + let state = ListState::new(3, crate::ListAlignment::Top, px(0.)).measure_all(); + let view = cx.update(|_, cx| cx.new(|_| Padded(state.clone()))); + cx.draw(point(px(0.), px(0.)), size(px(100.), px(200.)), |_, _| { + view.into_any_element() + }); + + // `List` puts the first item at `padding.top` and its scrollable + // content spans both padded edges, so both reported numbers carry the + // padding: item 2 sits 7 + 2*20 from the top of the content, and the + // content is 3*20 + 2*7 tall. + assert_eq!(state.item_top_and_content_height(2), (px(47.), px(74.))); + } + #[gpui::test] fn test_measure_all_after_width_change(cx: &mut TestAppContext) { let cx = cx.add_empty_window(); diff --git a/crates/gpui/src/external_texture.rs b/crates/gpui/src/external_texture.rs index c216a4b4b6e3fc..90e6820bd7b007 100644 --- a/crates/gpui/src/external_texture.rs +++ b/crates/gpui/src/external_texture.rs @@ -23,9 +23,11 @@ //! The producer (a Chromium OSR paint callback, on a Chromium thread) writes //! into its own buffer and bumps [`ExternalFrameView::sequence`]. The consumer //! (a GPUI renderer, on the UI thread during paint) calls -//! [`ExternalTextureSource::with_frame`], which is expected to take whatever -//! lock the producer uses. Implementations must keep that critical section to a -//! memcpy's worth of work — it runs inside the frame budget. +//! [`ExternalTextureSource::with_frame`]. [`ExternalTextureBuffer`] publishes +//! each frame as an immutable snapshot, so the visit — which uploads to the GPU +//! — runs without any producer lock held and a submit never waits for a render. +//! A source that does take a lock in `with_frame` must keep that critical +//! section to a memcpy's worth of work; it runs inside the frame budget. //! //! # Resize //! @@ -96,9 +98,11 @@ pub struct ExternalFrameView<'a> { /// sequence must not upload again — this is what makes a settled page cost /// zero bandwidth. pub sequence: u64, - /// The regions that changed since `sequence - 1`, in texture pixels. Empty - /// means "assume everything changed", which is correct but expensive; a - /// producer should always fill this in when it knows. + /// The regions that changed since the oldest frame a renderer still needs, + /// in texture pixels: a union, not just this frame's changes, because a + /// renderer that missed a frame needs everything that moved while it was + /// away. Empty means "assume everything changed", which is correct but + /// expensive; a producer should always fill this in when it knows. pub dirty: &'a [Bounds], } @@ -194,12 +198,58 @@ pub trait ExternalTextureSource: fmt::Debug + Send + Sync + 'static { /// place a renderer can see the sequence it just uploaded. An /// implementation must therefore not take any lock that `with_frame` /// holds, or the render thread deadlocks on the first painted frame. + /// + /// This is the single-consumer shorthand, and it counts as an + /// acknowledgement from [`ExternalTextureConsumerId::LEGACY`]. A renderer + /// that may share the source with another renderer should call + /// [`Self::mark_uploaded_for`] with its own consumer id instead. fn mark_uploaded(&self, sequence: u64); + /// Told to the source after `consumer` uploads `sequence`. + /// + /// The default forwards to [`Self::mark_uploaded`], which is all a source + /// with one consumer needs. [`ExternalTextureBuffer`] overrides it so dirty + /// regions are kept until *every* renderer that has drawn the source has + /// caught up, because one renderer acknowledging a frame must not make a + /// second renderer's texture stale. + fn mark_uploaded_for(&self, _consumer: ExternalTextureConsumerId, sequence: u64) { + self.mark_uploaded(sequence); + } + + /// The renderer behind `consumer` no longer draws this source. + /// + /// Called when a renderer drops the texture it cached for this source — a + /// closed window, a GPU device loss, or a cache prune. A source that keeps + /// per-consumer state must forget the consumer, or a renderer that will + /// never upload again would hold dirty regions for the rest of the source's + /// life. The default does nothing, for sources that keep no such state. + fn remove_consumer(&self, _consumer: ExternalTextureConsumerId) {} + /// How many uploads this source has served. Instrumentation only. fn upload_count(&self) -> u64; } +/// Identity of one renderer drawing a source's frames. +/// +/// A source can be drawn by more than one renderer — the same pane shown in two +/// windows — and each renderer uploads on its own schedule. Acknowledgements are +/// per consumer so that the renderer which draws second is not left with stale +/// pixels because the renderer which drew first acknowledged the frame. +#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord)] +pub struct ExternalTextureConsumerId(pub u64); + +impl ExternalTextureConsumerId { + /// The slot [`ExternalTextureSource::mark_uploaded`] acknowledges, for + /// sources and callers that only ever have one consumer. + pub const LEGACY: Self = Self(0); + + /// Hand out an id no other renderer in this process will get. + pub fn next() -> Self { + static NEXT: AtomicU64 = AtomicU64::new(1); + Self(NEXT.fetch_add(1, Ordering::Relaxed)) + } +} + /// A frame producer plus the bookkeeping every implementation would otherwise /// duplicate: double buffering, dirty-rect accumulation, sequence and /// generation counters, and the upload counter the perf gates read. @@ -208,25 +258,27 @@ pub trait ExternalTextureSource: fmt::Debug + Send + Sync + 'static { /// methods. Neither side needs to know about the other's thread. pub struct ExternalTextureBuffer { id: ExternalTextureId, + /// Producer-side bookkeeping. It holds no pixels, so a renderer never holds + /// this lock while it uploads and a producer never waits for one. state: parking_lot::Mutex, - /// Read by perf assertions from another thread, so it lives outside the + /// The frame renderers see. `submit` swaps a fresh snapshot in and + /// `with_frame` clones the `Arc` out before the visit, so a GPU upload + /// cannot block the producer's next frame. + frame: parking_lot::Mutex>>, + /// The allocation the next snapshot reuses. A steady stream of same-sized + /// frames therefore does not churn the allocator. + spare: parking_lot::Mutex>, + /// Read by perf assertions from another thread, so it lives outside every /// lock. uploads: AtomicU64, /// Mirrors `state.sequence` for lock-free reads. sequence: AtomicU64, - /// The last sequence a renderer said it uploaded. - /// - /// Outside the mutex on purpose. Renderers acknowledge from inside the - /// [`ExternalTextureSource::with_frame`] visitor, because that is the only - /// place they can see the sequence they just uploaded, and the lock is held - /// for the whole visit. An acknowledgement that took the lock would - /// deadlock the render thread on the first painted frame. - acknowledged: AtomicU64, + /// Last uploaded sequence per renderer that has drawn this source. + consumers: parking_lot::Mutex>, } #[derive(Default)] struct BufferState { - bytes: Vec, size: Size, stride: usize, format: Option, @@ -235,6 +287,18 @@ struct BufferState { dirty: Vec>, } +/// The pixels and metadata of one published frame, shared with whichever +/// renderers are drawing the source. +struct ExternalFrame { + bytes: Vec, + size: Size, + stride: usize, + format: ExternalTextureFormat, + generation: u64, + sequence: u64, + dirty: Vec>, +} + impl fmt::Debug for ExternalTextureBuffer { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { let state = self.state.lock(); @@ -261,9 +325,11 @@ impl ExternalTextureBuffer { Self { id: ExternalTextureId::next(), state: parking_lot::Mutex::new(BufferState::default()), + frame: parking_lot::Mutex::new(None), + spare: parking_lot::Mutex::new(Vec::new()), uploads: AtomicU64::new(0), sequence: AtomicU64::new(0), - acknowledged: AtomicU64::new(0), + consumers: parking_lot::Mutex::new(std::collections::BTreeMap::new()), } } @@ -283,9 +349,9 @@ impl ExternalTextureBuffer { self.sequence.load(Ordering::Acquire) } - /// Last frame committed by a renderer. CPU submissions do not advance it. + /// Last frame committed by any renderer. CPU submissions do not advance it. pub fn uploaded_sequence(&self) -> u64 { - self.acknowledged.load(Ordering::Acquire) + self.consumers.lock().values().copied().max().unwrap_or(0) } /// Current buffer dimensions, or zero before the first frame. @@ -329,43 +395,39 @@ impl ExternalTextureBuffer { return false; }; + // Bookkeeping and publication share one critical section, so two + // producers cannot publish frames out of sequence order. The lock holds + // a frame-sized memcpy, exactly as it did before the pixels moved into + // a snapshot — but it is *not* held across a visit, which is the part + // that used to stall a producer behind a GPU upload. let mut state = self.state.lock(); let reallocated = state.size != size || state.stride != stride || state.format != Some(format); if reallocated { - state.bytes.clear(); - state.bytes.resize(needed, 0); state.size = size; state.stride = stride; state.format = Some(format); state.generation += 1; + // A recreated texture is uploaded whole; per-rect bookkeeping + // would just be thrown away. state.dirty.clear(); - } else if state.bytes.len() < needed { - state.bytes.resize(needed, 0); - } - state.bytes[..needed].copy_from_slice(&bytes[..needed]); - - let acknowledged = self.acknowledged.load(Ordering::Acquire); - if reallocated { - // A recreated texture is uploaded whole; per-rect bookkeeping would - // just be thrown away. - state.dirty.clear(); - } else if state.sequence > acknowledged { - // The renderer has not consumed the previous frame, so this frame's + } else if self.has_unconsumed(&state) { + // Some renderer has not seen the previous frame, so this frame's // dirt is added to it rather than replacing it. Dropping it would // leave half the page showing stale pixels. // Empty means a full repaint. It must dominate in either order; // appending a small rectangle to it would lose that full repaint. - // Bound the list when a hidden/slow consumer misses many frames. + // Bound the list when a hidden or slow consumer misses many frames: + // an empty list is a full upload, which is correct and only + // expensive. if dirty.is_empty() || state.dirty.len().saturating_add(dirty.len()) > 64 { state.dirty.clear(); } else if !state.dirty.is_empty() { state.dirty.extend_from_slice(dirty); } } else { - // Everything up to here has been uploaded, so the previous rects - // are spent. This is also where an acknowledgement's clean-up - // happens, which is why `mark_uploaded` does not need the lock. + // Every renderer that has drawn this source is caught up, so the + // previous rects are spent. state.dirty.clear(); if dirty.len() <= 64 { state.dirty.extend_from_slice(dirty); @@ -373,23 +435,93 @@ impl ExternalTextureBuffer { } state.sequence += 1; - self.sequence.store(state.sequence, Ordering::Release); + let sequence = state.sequence; + + let mut spare = self.spare.lock(); + let mut staged = std::mem::take(&mut *spare); + // `resize` truncates a larger spare and zero-fills only the tail that + // grows, so a clear() first would memset every byte the copy is about + // to overwrite — 8 MB per 1080p frame, inside the producer's lock. + staged.resize(needed, 0); + staged.copy_from_slice(&bytes[..needed]); + let previous = self.frame.lock().replace(Arc::new(ExternalFrame { + bytes: staged, + size, + stride, + format, + generation: state.generation, + sequence, + dirty: state.dirty.clone(), + })); + // Reuse the previous snapshot's allocation when no renderer is still + // looking at it, so a steady stream of frames settles at zero + // allocations. + if let Some(previous) = previous + && let Ok(previous) = Arc::try_unwrap(previous) + { + *spare = previous.bytes; + } + // Stored under the state lock: two producers must not write the mirror + // out of order, and a concurrent `release_frame` must not leave it + // ahead of a frame that no longer exists. + self.sequence.store(sequence, Ordering::Release); + drop(spare); + drop(state); + true } + /// Whether any renderer that has drawn this source is behind the frame the + /// pending dirty regions belong to. + fn has_unconsumed(&self, state: &BufferState) -> bool { + if state.sequence == 0 { + return false; + } + let consumers = self.consumers.lock(); + consumers.values().any(|ack| *ack < state.sequence) + } + + /// Record one renderer's upload. Lock-free from the producer's point of + /// view, and safe from inside a visit: it takes no lock `with_frame` holds. + fn acknowledge(&self, consumer: ExternalTextureConsumerId, sequence: u64) { + self.consumers + .lock() + .entry(consumer) + .and_modify(|ack| *ack = (*ack).max(sequence)) + .or_insert(sequence); + self.uploads.fetch_add(1, Ordering::Relaxed); + } + + /// Forget a renderer that has dropped this source's texture. + /// + /// Without this a renderer that will never draw again — a closed window, a + /// recovered GPU device — would keep `has_unconsumed` true forever, and the + /// dirty list would degrade to periodic full-frame uploads for every + /// remaining consumer. + pub fn remove_consumer(&self, consumer: ExternalTextureConsumerId) { + self.consumers.lock().remove(&consumer); + } + /// Forget the frame without dropping the identity, so a hidden pane stops /// holding a framebuffer while keeping its renderer (PRD HID-01). pub fn release_frame(&self) { - let mut state = self.state.lock(); - state.bytes = Vec::new(); - state.size = Size::default(); - state.stride = 0; - state.format = None; - state.dirty.clear(); - state.generation += 1; - state.sequence = 0; + { + let mut state = self.state.lock(); + state.size = Size::default(); + state.stride = 0; + state.format = None; + state.dirty.clear(); + state.generation += 1; + state.sequence = 0; + } + *self.frame.lock() = None; + // The spare is a framebuffer as well, and this call exists to stop a + // hidden pane holding one (PRD HID-01), so it goes too. + *self.spare.lock() = Vec::new(); + // A renderer that draws this source again starts from a recreate and + // registers itself again on its first upload. + self.consumers.lock().clear(); self.sequence.store(0, Ordering::Release); - self.acknowledged.store(0, Ordering::Release); } } @@ -399,29 +531,30 @@ impl ExternalTextureSource for ExternalTextureBuffer { } fn with_frame(&self, visit: &mut dyn FnMut(ExternalFrameView<'_>)) { - let state = self.state.lock(); - let Some(format) = state.format else { + // Clone the snapshot out and let the lock go before the visit: the + // visitor uploads to the GPU, and the producer's next frame must not + // wait for that. + let frame = self.frame.lock().clone(); + let Some(frame) = frame else { return; }; - if state.sequence == 0 { - return; - } visit(ExternalFrameView { - size: state.size, - stride: state.stride, - format, - bytes: &state.bytes, - generation: state.generation, - sequence: state.sequence, - dirty: &state.dirty, + size: frame.size, + stride: frame.stride, + format: frame.format, + bytes: &frame.bytes, + generation: frame.generation, + sequence: frame.sequence, + dirty: &frame.dirty, }); } fn mark_uploaded(&self, sequence: u64) { - // Deliberately lock-free: this is called from inside `with_frame`, - // which holds the state lock for the whole visit. - self.acknowledged.fetch_max(sequence, Ordering::AcqRel); - self.uploads.fetch_add(1, Ordering::Relaxed); + self.acknowledge(ExternalTextureConsumerId::LEGACY, sequence); + } + + fn mark_uploaded_for(&self, consumer: ExternalTextureConsumerId, sequence: u64) { + self.acknowledge(consumer, sequence); } fn upload_count(&self) -> u64 { @@ -544,6 +677,110 @@ mod tests { ); } + /// Two renderers drawing one source must both see the pixels that changed + /// while they were behind. The first one to acknowledge must not make the + /// source discard the dirt the second one still needs. + #[test] + fn dirty_regions_wait_for_every_consumer() { + let buffer = ExternalTextureBuffer::new(); + let pixels = bgra(8, 8, 0x22); + let first = ExternalTextureConsumerId::next(); + let second = ExternalTextureConsumerId::next(); + let dirty = |x: i32, y: i32| Bounds { + origin: point(DevicePixels(x), DevicePixels(y)), + size: size(DevicePixels(2), DevicePixels(2)), + }; + + // Both renderers upload the first frame (a recreate is a full upload). + assert!(buffer.submit(dims(8, 8), 32, ExternalTextureFormat::Bgra8, &pixels, &[])); + buffer.with_frame(&mut |frame| { + buffer.mark_uploaded_for(first, frame.sequence); + buffer.mark_uploaded_for(second, frame.sequence); + }); + + // Frame 2: the first renderer uploads it, the second has not yet. + assert!(buffer.submit( + dims(8, 8), + 32, + ExternalTextureFormat::Bgra8, + &pixels, + &[dirty(0, 0)] + )); + buffer.with_frame(&mut |frame| { + assert_eq!(frame.dirty, [dirty(0, 0)]); + buffer.mark_uploaded_for(first, frame.sequence); + }); + + // Frame 3 arrives before the second renderer drew frame 2, so its + // regions are the union, not just this frame's. + assert!(buffer.submit( + dims(8, 8), + 32, + ExternalTextureFormat::Bgra8, + &pixels, + &[dirty(4, 4)] + )); + buffer.with_frame(&mut |frame| { + assert_eq!( + frame.dirty, + [dirty(0, 0), dirty(4, 4)], + "a renderer that has not drawn frame 2 still needs its pixels" + ); + buffer.mark_uploaded_for(second, frame.sequence); + buffer.mark_uploaded_for(first, frame.sequence); + }); + + // Both are caught up, so the next frame's dirt replaces the list. + assert!(buffer.submit( + dims(8, 8), + 32, + ExternalTextureFormat::Bgra8, + &pixels, + &[dirty(6, 6)] + )); + buffer.with_frame(&mut |frame| { + assert_eq!(frame.dirty, [dirty(6, 6)]); + }); + } + + /// The visit uploads to the GPU. A producer submitting the next frame must + /// not wait for that, or Chromium's paint callback blocks on our bandwidth. + #[test] + fn a_visit_does_not_block_the_producer() { + use std::sync::mpsc; + + let buffer = Arc::new(ExternalTextureBuffer::new()); + let pixels = bgra(4, 4, 0x33); + assert!(buffer.submit(dims(4, 4), 16, ExternalTextureFormat::Bgra8, &pixels, &[])); + + let (entered_tx, entered_rx) = mpsc::channel(); + let (release_tx, release_rx) = mpsc::channel::<()>(); + let visitor_buffer = Arc::clone(&buffer); + let visitor = std::thread::spawn(move || { + visitor_buffer.with_frame(&mut |frame| { + entered_tx.send(()).unwrap(); + // Stand in for a GPU upload: the producer's next submit has to + // get through while this is in flight. + release_rx.recv().unwrap(); + assert!(frame.is_well_formed()); + }); + }); + entered_rx.recv().unwrap(); + + let (submitted_tx, submitted_rx) = mpsc::channel(); + let producer_buffer = Arc::clone(&buffer); + let producer = std::thread::spawn(move || { + let ok = + producer_buffer.submit(dims(4, 4), 16, ExternalTextureFormat::Bgra8, &pixels, &[]); + submitted_tx.send(ok).unwrap(); + }); + let submitted = submitted_rx.recv_timeout(std::time::Duration::from_secs(5)); + release_tx.send(()).unwrap(); + visitor.join().unwrap(); + producer.join().unwrap(); + assert_eq!(submitted.expect("a submit must not wait for a visit"), true); + } + #[test] fn ids_are_unique_per_source() { let a = ExternalTextureBuffer::new(); @@ -834,9 +1071,9 @@ mod tests { fn acknowledging_from_inside_the_visitor_does_not_deadlock() { // The renderers do exactly this: they learn the sequence they uploaded // from the frame view, and acknowledge it before the view goes out of - // scope. `with_frame` holds the state lock for the whole visit, so an - // acknowledgement that took the same lock would hang the render thread - // on the very first painted frame. + // scope. The visit must stay free of every lock a source holds, or a + // source that takes one in `with_frame` would hang the render thread on + // the very first painted frame. let buffer = ExternalTextureBuffer::new(); buffer.submit( dims(4, 4), diff --git a/crates/gpui/src/scene.rs b/crates/gpui/src/scene.rs index c53cf58bbfc77c..b3410309864648 100644 --- a/crates/gpui/src/scene.rs +++ b/crates/gpui/src/scene.rs @@ -722,6 +722,13 @@ pub struct PaintSurface { pub bounds: Bounds, pub content_mask: ContentMask, pub content: SurfaceContent, + /// Opacity of the element this surface was painted inside. + /// + /// A surface is drawn from its own texture, so the renderer cannot inherit + /// the sprite atlas's element opacity: without this an external texture + /// inside a faded ancestor would stay fully opaque. The CoreVideo path does + /// not consume it yet. + pub opacity: f32, } /// What a [`PaintSurface`] samples from. diff --git a/crates/gpui/src/window.rs b/crates/gpui/src/window.rs index 32522845715265..76948cf2d66918 100644 --- a/crates/gpui/src/window.rs +++ b/crates/gpui/src/window.rs @@ -4126,11 +4126,13 @@ impl Window { let bounds = self.snap_bounds(bounds); let content_mask = self.snapped_content_mask(); + let opacity = self.element_opacity(); self.next_frame.scene.insert_primitive(PaintSurface { order: 0, bounds, content_mask, content: SurfaceContent::PixelBuffer(image_buffer), + opacity, }); } @@ -4145,6 +4147,12 @@ impl Window { /// the entire atlas every frame. See [`crate::external_texture`]. /// /// Call only during the paint phase of element drawing. + /// + /// **Not available on macOS.** The Metal backend composites CoreVideo + /// surfaces only ([`Self::paint_surface`]), which is the path macOS browser + /// and video panes use; a caller-owned RGBA texture has no Metal pipeline + /// here. On macOS this records nothing and warns once, rather than + /// appearing to paint and showing no pixels. pub fn paint_external_texture( &mut self, bounds: Bounds, @@ -4154,13 +4162,27 @@ impl Window { self.invalidator.debug_assert_paint(); + if cfg!(target_os = "macos") { + static WARNED: std::sync::OnceLock<()> = std::sync::OnceLock::new(); + if WARNED.set(()).is_ok() { + log::warn!( + "paint_external_texture is unavailable on macOS: the Metal backend \ + composites CoreVideo surfaces only; use paint_surface instead" + ); + } + let _ = (bounds, source); + return; + } + let bounds = self.snap_bounds(bounds); let content_mask = self.snapped_content_mask(); + let opacity = self.element_opacity(); self.next_frame.scene.insert_primitive(PaintSurface { order: 0, bounds, content_mask, content: SurfaceContent::ExternalTexture(source), + opacity, }); } diff --git a/crates/gpui_wgpu/src/wgpu_renderer.rs b/crates/gpui_wgpu/src/wgpu_renderer.rs index 76b33da99cdc01..f5bc5829e183a1 100644 --- a/crates/gpui_wgpu/src/wgpu_renderer.rs +++ b/crates/gpui_wgpu/src/wgpu_renderer.rs @@ -56,6 +56,21 @@ struct CachedExternalTexture { texture: wgpu::Texture, view: wgpu::TextureView, key: ExternalTextureCacheKey, + /// The source this texture belongs to, and this renderer's slot in its + /// acknowledgement table. + /// + /// Both are here so `Drop` can retire the consumer: a renderer that loses + /// its device, prunes the cache, or goes away must stop counting as a + /// consumer that has not uploaded, or the source would keep dirty regions + /// for a renderer that will never draw again. + source: Arc, + consumer: gpui::ExternalTextureConsumerId, +} + +impl Drop for CachedExternalTexture { + fn drop(&mut self) { + self.source.remove_consumer(self.consumer); + } } #[repr(C)] @@ -180,6 +195,12 @@ pub struct WgpuRenderer { transparent_alpha_mode: wgpu::CompositeAlphaMode, opaque_alpha_mode: wgpu::CompositeAlphaMode, max_texture_size: u32, + /// This renderer's slot in every external source's acknowledgement table. + /// + /// Stable for the renderer's life, because a source holds dirty regions + /// until the *oldest* consumer has uploaded them: a per-frame id would look + /// like a new, un-caught-up renderer every frame. + external_texture_consumer: gpui::ExternalTextureConsumerId, last_error: Arc>>, failed_frame_count: u32, device_lost: std::sync::Arc, @@ -512,6 +533,7 @@ impl WgpuRenderer { transparent_alpha_mode, opaque_alpha_mode, max_texture_size, + external_texture_consumer: gpui::ExternalTextureConsumerId::next(), last_error, failed_frame_count: 0, device_lost: context.device_lost_flag(), @@ -1534,6 +1556,23 @@ impl WgpuRenderer { return; } + // A frame past the adapter's limit cannot be a texture at all. + // `create_texture` would fail validation and poison the frame, + // so the surface is skipped: nothing is uploaded, nothing is + // acknowledged (the producer keeps its dirt), and the rest of + // the scene still draws. + let width = frame.size.width.0.max(0) as u32; + let height = frame.size.height.0.max(0) as u32; + if width > self.max_texture_size || height > self.max_texture_size { + log::error!( + "external texture {:?} is {width}x{height}, past the adapter's \ + {}-pixel limit; skipping it", + source.id(), + self.max_texture_size + ); + return; + } + let cached_key = self .resources() .external_textures @@ -1543,17 +1582,17 @@ impl WgpuRenderer { let plan = plan_external_texture_update(cached_key, &frame); if !matches!(plan, ExternalTextureUpdate::Reuse) { - if !self.upload_external_texture(source.id(), &frame, plan) { + if !self.upload_external_texture(source, &frame, plan) { ok = false; return; } - source.mark_uploaded(frame.sequence); + source.mark_uploaded_for(self.external_texture_consumer, frame.sequence); } let instances = [ExternalTextureInstance { bounds: surface.bounds.into(), content_mask: surface.content_mask.bounds.into(), - opacity: 1.0, + opacity: surface.opacity, swap_red_blue: u32::from( matches!(frame.format, ExternalTextureFormat::Bgra8) != (self.resources().external_texture_format @@ -1588,10 +1627,11 @@ impl WgpuRenderer { /// false only when the frame cannot be represented at all. fn upload_external_texture( &self, - id: ExternalTextureId, + source: &Arc, frame: &gpui::ExternalFrameView<'_>, plan: ExternalTextureUpdate, ) -> bool { + let id = source.id(); let resources = self.resources(); // The context validated sampled/copy support for this format. Upload // producer bytes directly; the draw instance swaps channels if needed. @@ -1630,6 +1670,8 @@ impl WgpuRenderer { sequence: 0, size: frame.size, }, + source: Arc::clone(source), + consumer: self.external_texture_consumer, }, ); } diff --git a/crates/gpui_windows/src/directx_renderer.rs b/crates/gpui_windows/src/directx_renderer.rs index 259993c50f7330..e6bb4a619b4371 100644 --- a/crates/gpui_windows/src/directx_renderer.rs +++ b/crates/gpui_windows/src/directx_renderer.rs @@ -49,6 +49,12 @@ pub(crate) struct DirectXRenderer { /// a page that is not repainting costs a draw call and no upload /// (PERF-B04). Dropped with the device on a device-lost recovery. external_textures: HashMap, + /// This renderer's slot in every external source's acknowledgement table. + /// + /// Stable for the renderer's life, because a source holds dirty regions + /// until the *oldest* consumer has uploaded them: a per-frame id would look + /// like a new, un-caught-up renderer every frame. + external_texture_consumer: gpui::ExternalTextureConsumerId, width: u32, height: u32, @@ -114,11 +120,39 @@ struct CachedExternalTexture { texture: ID3D11Texture2D, view: Option, key: ExternalTextureCacheKey, + /// The source this texture belongs to, and this renderer's slot in its + /// acknowledgement table. + /// + /// Both are here so `Drop` can retire the consumer: a renderer that loses + /// its device, prunes the cache, or goes away must stop counting as a + /// consumer that has not uploaded, or the source would keep dirty regions + /// for a renderer that will never draw again. + source: Arc, + consumer: gpui::ExternalTextureConsumerId, } +impl Drop for CachedExternalTexture { + fn drop(&mut self) { + self.source.remove_consumer(self.consumer); + } +} + +/// `D3D11_REQ_TEXTURE2D_U_OR_V_DIMENSION`: the largest 2D texture D3D11 +/// guarantees. The header defines it as a macro, which windows-rs does not +/// export, so it is spelled out here. +const MAX_TEXTURE2D_DIMENSION: u32 = 16_384; + struct DirectXGlobalElements { global_params_buffer: Option, sampler: Option, + /// Clamped sibling of `sampler`, for surfaces that are not atlas tiles. + /// + /// The shared sampler wraps, which is right for a sprite tile whose UVs sit + /// inside the atlas: it keeps bilinear taps from sampling the neighbouring + /// tile. An external texture drawn at a size other than its own would wrap + /// the page around and bleed the opposite edge into the frame, so it gets a + /// sampler that clamps to the edge instead. + external_texture_sampler: Option, } struct DirectComposition { @@ -196,6 +230,7 @@ impl DirectXRenderer { direct_composition, font_info: Self::get_font_info(), external_textures: HashMap::new(), + external_texture_consumer: gpui::ExternalTextureConsumerId::next(), width: 1, height: 1, skip_draws: false, @@ -759,14 +794,15 @@ impl DirectXRenderer { ); return; } - match self.draw_external_frame(surface, source.id(), &frame) { + match self.draw_external_frame(surface, source, &frame) { // Only an actual upload is acknowledged. Acknowledging a // reuse would make `upload_count` report the compositing // rate, and PERF-B04 reads that counter to prove a settled // page stops uploading. Ok(uploaded) => { if uploaded { - source.mark_uploaded(frame.sequence); + source + .mark_uploaded_for(self.external_texture_consumer, frame.sequence); } } Err(error) => result = Err(error), @@ -784,14 +820,26 @@ impl DirectXRenderer { fn draw_external_frame( &mut self, surface: &PaintSurface, - id: ExternalTextureId, + source: &Arc, frame: &ExternalFrameView<'_>, ) -> Result { + let id = source.id(); let width = frame.size.width.0.max(0) as u32; let height = frame.size.height.0.max(0) as u32; if width == 0 || height == 0 { return Ok(false); } + if width > MAX_TEXTURE2D_DIMENSION || height > MAX_TEXTURE2D_DIMENSION { + // A frame past the device's limit cannot be a texture at all, and + // `CreateTexture2D` would fail the whole window's frame. Skipping + // the surface leaves the rest of the scene drawing; nothing is + // acknowledged, so the producer keeps its dirt. + log::error!( + "external texture {id:?} is {width}x{height}, past D3D11's \ + {MAX_TEXTURE2D_DIMENSION}-pixel limit; skipping it" + ); + return Ok(false); + } let plan = plan_external_texture_update( self.external_textures.get(&id).map(|cached| cached.key), @@ -850,6 +898,8 @@ impl DirectXRenderer { sequence: 0, size: frame.size, }, + source: Arc::clone(source), + consumer: self.external_texture_consumer, }, ); } @@ -894,8 +944,9 @@ impl DirectXRenderer { // SAFETY: `destination` is clipped to the texture by // `dirty_regions`, and the bounds check above proves the // source rows for that box are inside `frame.bytes`, which - // stays alive for this call because the producer holds its - // lock for the duration of `with_frame`. + // stays alive for this call because `with_frame` clones the + // frame snapshot before the visit and the visitor's borrow + // is bounded by this closure. unsafe { devices.device_context.UpdateSubresource( &cached.texture, @@ -919,7 +970,7 @@ impl DirectXRenderer { let instance = ExternalTextureInstance { bounds: surface.bounds, content_mask: surface.content_mask.bounds, - opacity: 1.0, + opacity: surface.opacity, pad: 0, }; let devices = self.devices.as_ref().context("devices missing")?; @@ -941,7 +992,7 @@ impl DirectXRenderer { slice::from_ref(&view), slice::from_ref(&resources.viewport), slice::from_ref(&self.globals.global_params_buffer), - slice::from_ref(&self.globals.sampler), + slice::from_ref(&self.globals.external_texture_sampler), 0, 1, )?; @@ -1205,9 +1256,28 @@ impl DirectXGlobalElements { output }; + let external_texture_sampler = unsafe { + let desc = D3D11_SAMPLER_DESC { + Filter: D3D11_FILTER_MIN_MAG_MIP_LINEAR, + AddressU: D3D11_TEXTURE_ADDRESS_CLAMP, + AddressV: D3D11_TEXTURE_ADDRESS_CLAMP, + AddressW: D3D11_TEXTURE_ADDRESS_CLAMP, + MipLODBias: 0.0, + MaxAnisotropy: 1, + ComparisonFunc: D3D11_COMPARISON_ALWAYS, + BorderColor: [0.0; 4], + MinLOD: 0.0, + MaxLOD: D3D11_FLOAT32_MAX, + }; + let mut output = None; + device.CreateSamplerState(&desc, Some(&mut output))?; + output + }; + Ok(Self { global_params_buffer, sampler, + external_texture_sampler, }) } } From 458d821a99dd1d4883be435217dc212905ead1a6 Mon Sep 17 00:00:00 2001 From: soyboyscout Date: Sun, 20 Sep 2026 19:47:16 -0400 Subject: [PATCH 5/5] fix(gpui): close the second review pass on external textures The eight threads from the second review round: - `release_frame` now holds the state lock while it clears the frame, the spare allocation, the consumer table and the mirrored sequence, so a producer cannot publish between those steps. `state` is taken first and is the only lock ever held while acquiring another, which is the invariant the comment now states. - A partial upload is never acknowledged. Both backends report an `ExternalTextureUpload` outcome; a region rejected by the bounds checks leaves the cache key untouched and the frame unacknowledged, so the producer keeps the dirt and a later frame retries it. Direct3D still draws the last complete texture instead of skipping the surface, and its buffer-span rejection now logs like the wgpu one. - Dirty regions are validated against the frame's own dimensions and the byte span is computed with checked arithmetic on both backends, rather than trusting the caller's clamping. - A frame with no pixels to copy reports `Reused`, not `Complete`: it is not an upload and must not reach the upload counter. - The macOS CoreVideo path consumes the element opacity now: the instance struct carries it, the vertex shader forwards it, and the fragment shader multiplies the alpha by it, matching both other backends. - `PaintSurface::opacity` documents that both surface paths consume it, and the `item_top_and_content_height` doc no longer links a private method (rustdoc flagged it). --- crates/gpui/src/elements/list.rs | 4 +- crates/gpui/src/external_texture.rs | 26 +++-- crates/gpui/src/scene.rs | 7 +- crates/gpui_macos/src/metal_renderer.rs | 6 +- crates/gpui_macos/src/shaders.metal | 10 +- crates/gpui_wgpu/src/wgpu_renderer.rs | 93 ++++++++++++--- crates/gpui_windows/src/directx_renderer.rs | 120 +++++++++++++++----- 7 files changed, 207 insertions(+), 59 deletions(-) diff --git a/crates/gpui/src/elements/list.rs b/crates/gpui/src/elements/list.rs index 096c0f61457a90..1edcbd847df5db 100644 --- a/crates/gpui/src/elements/list.rs +++ b/crates/gpui/src/elements/list.rs @@ -692,8 +692,8 @@ impl ListState { /// /// Both numbers are in the list's own content space, padding included: /// [`List`] places the first item at `padding.top` and the content it - /// scrolls spans both edges, matching [`ListState::scroll`] and - /// [`ListState::is_scrolled_to_end`]. Note that + /// scrolls spans both edges, which is what + /// [`ListState::is_scrolled_to_end`] measures. Note that /// [`ListState::max_offset_for_scrollbar`] and /// [`ListState::scroll_px_offset_for_scrollbar`] still measure the items /// without padding, so a caller mixing the two is off by the padding. diff --git a/crates/gpui/src/external_texture.rs b/crates/gpui/src/external_texture.rs index 90e6820bd7b007..280131f0b657f5 100644 --- a/crates/gpui/src/external_texture.rs +++ b/crates/gpui/src/external_texture.rs @@ -505,19 +505,25 @@ impl ExternalTextureBuffer { /// Forget the frame without dropping the identity, so a hidden pane stops /// holding a framebuffer while keeping its renderer (PRD HID-01). pub fn release_frame(&self) { - { - let mut state = self.state.lock(); - state.size = Size::default(); - state.stride = 0; - state.format = None; - state.dirty.clear(); - state.generation += 1; - state.sequence = 0; - } - *self.frame.lock() = None; + // Everything happens under the state lock, which is also the lock + // `submit` holds while it publishes: a producer cannot slip a frame in + // between, which would either be erased by this call or leave + // `has_frame()` true with no frame behind it. + // + // Lock invariant: `state` is always taken first and is the only lock + // ever held while acquiring another; `consumers`, `spare` and `frame` + // are leaves that are never held while taking anything. + let mut state = self.state.lock(); + state.size = Size::default(); + state.stride = 0; + state.format = None; + state.dirty.clear(); + state.generation += 1; + state.sequence = 0; // The spare is a framebuffer as well, and this call exists to stop a // hidden pane holding one (PRD HID-01), so it goes too. *self.spare.lock() = Vec::new(); + *self.frame.lock() = None; // A renderer that draws this source again starts from a recreate and // registers itself again on its first upload. self.consumers.lock().clear(); diff --git a/crates/gpui/src/scene.rs b/crates/gpui/src/scene.rs index b3410309864648..c37f2026507f59 100644 --- a/crates/gpui/src/scene.rs +++ b/crates/gpui/src/scene.rs @@ -725,9 +725,10 @@ pub struct PaintSurface { /// Opacity of the element this surface was painted inside. /// /// A surface is drawn from its own texture, so the renderer cannot inherit - /// the sprite atlas's element opacity: without this an external texture - /// inside a faded ancestor would stay fully opaque. The CoreVideo path does - /// not consume it yet. + /// the sprite atlas's element opacity: without this a surface inside a + /// faded ancestor would stay fully opaque. Both surface paths consume it — + /// the CoreVideo shader multiplies its fragment alpha by it, and the + /// external-texture instance carries it to each backend's shader. pub opacity: f32, } diff --git a/crates/gpui_macos/src/metal_renderer.rs b/crates/gpui_macos/src/metal_renderer.rs index 0088075811e56b..1bdd52202783e6 100644 --- a/crates/gpui_macos/src/metal_renderer.rs +++ b/crates/gpui_macos/src/metal_renderer.rs @@ -1597,6 +1597,7 @@ impl MetalRenderer { SurfaceBounds { bounds: surface.bounds, content_mask: surface.content_mask, + opacity: surface.opacity, }, ); } @@ -1806,11 +1807,14 @@ pub struct PathSprite { pub bounds: Bounds, } -#[derive(Clone, Debug, Eq, PartialEq)] +#[derive(Clone, Debug, PartialEq)] #[repr(C)] pub struct SurfaceBounds { pub bounds: Bounds, pub content_mask: ContentMask, + /// Opacity of the element the surface was painted inside, so a video + /// surface fades with its ancestors instead of staying fully opaque. + pub opacity: f32, } #[cfg(any(test, feature = "test-support"))] diff --git a/crates/gpui_macos/src/shaders.metal b/crates/gpui_macos/src/shaders.metal index 2b52bb9ecc0476..f32118b091e37a 100644 --- a/crates/gpui_macos/src/shaders.metal +++ b/crates/gpui_macos/src/shaders.metal @@ -850,12 +850,14 @@ fragment float4 path_sprite_fragment( struct SurfaceVertexOutput { float4 position [[position]]; float2 texture_position; + float opacity; float clip_distance [[clip_distance]][4]; }; struct SurfaceFragmentInput { float4 position [[position]]; float2 texture_position; + float opacity; }; vertex SurfaceVertexOutput surface_vertex( @@ -878,6 +880,7 @@ vertex SurfaceVertexOutput surface_vertex( return SurfaceVertexOutput{ device_position, texture_position, + surface.opacity, {clip_distance.x, clip_distance.y, clip_distance.z, clip_distance.w}}; } @@ -896,7 +899,12 @@ fragment float4 surface_fragment(SurfaceFragmentInput input [[stage_in]], y_texture.sample(texture_sampler, input.texture_position).r, cb_cr_texture.sample(texture_sampler, input.texture_position).rg, 1.0); - return ycbcrToRGBTransform * ycbcr; + // The element's opacity, carried from the vertex stage: a surface painted + // inside a faded ancestor must fade with it, exactly as the external-texture + // backends do. + float4 color = ycbcrToRGBTransform * ycbcr; + color.a *= input.opacity; + return color; } float4 hsla_to_rgba(Hsla hsla) { diff --git a/crates/gpui_wgpu/src/wgpu_renderer.rs b/crates/gpui_wgpu/src/wgpu_renderer.rs index f5bc5829e183a1..bd9eace6f119de 100644 --- a/crates/gpui_wgpu/src/wgpu_renderer.rs +++ b/crates/gpui_wgpu/src/wgpu_renderer.rs @@ -50,6 +50,25 @@ struct ExternalTextureInstance { swap_red_blue: u32, } +/// What one external-texture upload managed to copy. +/// +/// A frame is only acknowledged when every region the producer reported reached +/// the GPU. Acknowledging a partial upload would let the producer drop dirty +/// regions whose pixels were never copied, and nothing would ever resend them. +#[derive(Copy, Clone, Debug, PartialEq, Eq)] +enum ExternalTextureUpload { + /// Nothing needed uploading: the frame had no pixels to copy. Not an + /// upload, so not acknowledged. + Reused, + /// Every region was copied. + Complete, + /// Not every region reached the GPU: the texture may be stale or only + /// partly updated. The producer keeps its dirty regions for a later frame. + Partial, + /// Nothing could be cached for this source. + Failed, +} + /// A GPU texture owned by one external source, kept across frames so a page /// that is not repainting costs no upload bandwidth (PERF-B04). struct CachedExternalTexture { @@ -1582,11 +1601,23 @@ impl WgpuRenderer { let plan = plan_external_texture_update(cached_key, &frame); if !matches!(plan, ExternalTextureUpdate::Reuse) { - if !self.upload_external_texture(source, &frame, plan) { - ok = false; - return; + match self.upload_external_texture(source, &frame, plan) { + ExternalTextureUpload::Complete => { + source + .mark_uploaded_for(self.external_texture_consumer, frame.sequence); + } + ExternalTextureUpload::Reused => {} + ExternalTextureUpload::Partial => { + // Deliberately unacknowledged: the pixels this frame + // could not copy are still dirty as far as the + // producer is concerned, so a later frame retries + // them instead of leaving stale pixels forever. + } + ExternalTextureUpload::Failed => { + ok = false; + return; + } } - source.mark_uploaded_for(self.external_texture_consumer, frame.sequence); } let instances = [ExternalTextureInstance { @@ -1623,14 +1654,18 @@ impl WgpuRenderer { true } - /// Create or refresh the cached texture for one external source. Returns - /// false only when the frame cannot be represented at all. + /// Create or refresh the cached texture for one external source. + /// + /// Returns [`ExternalTextureUpload::Failed`] only when the frame cannot be + /// cached at all; a region rejected as malformed yields + /// [`ExternalTextureUpload::Partial`], which the caller must not + /// acknowledge. fn upload_external_texture( &self, source: &Arc, frame: &gpui::ExternalFrameView<'_>, plan: ExternalTextureUpdate, - ) -> bool { + ) -> ExternalTextureUpload { let id = source.id(); let resources = self.resources(); // The context validated sampled/copy support for this format. Upload @@ -1639,7 +1674,9 @@ impl WgpuRenderer { let width = frame.size.width.0.max(0) as u32; let height = frame.size.height.0.max(0) as u32; if width == 0 || height == 0 { - return true; + // Nothing to copy. This is not an upload, so the caller must not + // count it as one. + return ExternalTextureUpload::Reused; } let mut textures = resources.external_textures.borrow_mut(); @@ -1677,7 +1714,7 @@ impl WgpuRenderer { } let Some(cached) = textures.get_mut(&id) else { - return false; + return ExternalTextureUpload::Failed; }; // A recreated texture has no pixels yet, so it takes the whole frame @@ -1691,6 +1728,10 @@ impl WgpuRenderer { frame.dirty_regions() }; + // A region the producer got wrong is dropped rather than turned into an + // out-of-bounds read or a validation panic, and the frame is then not + // acknowledged, so the dirt it covered comes back on a later frame. + let mut rejected = false; for region in regions { let x = region.origin.x.0.max(0) as u32; let y = region.origin.y.0.max(0) as u32; @@ -1699,17 +1740,36 @@ impl WgpuRenderer { if region_width == 0 || region_height == 0 { continue; } - let offset = y as usize * frame.stride + x as usize * 4; + // The region must fit the frame's own dimensions: `dirty_regions` + // clamps to them, but this function does not rely on its caller for + // a bound the GPU will enforce with a validation error. + if x.saturating_add(region_width) > width || y.saturating_add(region_height) > height { + log::error!( + "external texture {id:?} reported a dirty region outside its own frame" + ); + rejected = true; + continue; + } + let offset = (y as usize) + .checked_mul(frame.stride) + .and_then(|row| row.checked_add(x as usize * 4)); // `write_texture` reads `region_height` rows of `region_width * 4` // bytes at `frame.stride` pitch starting here. A slice shorter than // that is a wgpu validation panic, so the whole span is checked - // rather than just its first byte. - let span_end = - (y + region_height - 1) as usize * frame.stride + (x + region_width) as usize * 4; + // rather than just its first byte, with arithmetic that cannot wrap. + let span_end = ((y + region_height - 1) as usize) + .checked_mul(frame.stride) + .and_then(|end| end.checked_add((x + region_width) as usize * 4)); + let (Some(offset), Some(span_end)) = (offset, span_end) else { + log::error!("external texture {id:?} reported a region that overflows its stride"); + rejected = true; + continue; + }; if span_end > frame.bytes.len() { log::error!( "external texture {id:?} reported a dirty region outside its own buffer" ); + rejected = true; continue; } resources.queue.write_texture( @@ -1733,13 +1793,18 @@ impl WgpuRenderer { ); } + if rejected { + // The key stays where it was, so the next frame plans an upload + // again and the producer's dirt is still there to retry. + return ExternalTextureUpload::Partial; + } cached.key = ExternalTextureCacheKey { id, generation: frame.generation, sequence: frame.sequence, size: frame.size, }; - true + ExternalTextureUpload::Complete } fn draw_instances( diff --git a/crates/gpui_windows/src/directx_renderer.rs b/crates/gpui_windows/src/directx_renderer.rs index e6bb4a619b4371..3fc8e4e2e52487 100644 --- a/crates/gpui_windows/src/directx_renderer.rs +++ b/crates/gpui_windows/src/directx_renderer.rs @@ -116,6 +116,25 @@ struct ExternalTextureInstance { } /// A GPU texture owned by one external source, kept across frames. +/// What one external-texture upload managed to copy. +/// +/// A frame is only acknowledged when every region the producer reported reached +/// the GPU. Acknowledging a partial upload would let the producer drop dirty +/// regions whose pixels were never copied, and nothing would ever resend them. +#[derive(Copy, Clone, Debug, PartialEq, Eq)] +enum ExternalTextureUpload { + /// Nothing needed uploading: the cached texture already held this frame, or + /// the frame had no pixels to copy. Not an upload, so not acknowledged. + Reused, + /// Every region was copied. + Complete, + /// Not every region reached the GPU: the texture may be stale or only + /// partly updated. The producer keeps its dirty regions for a later frame. + Partial, +} + +/// A GPU texture owned by one external source, kept across frames so a page +/// that is not repainting costs no upload bandwidth (PERF-B04). struct CachedExternalTexture { texture: ID3D11Texture2D, view: Option, @@ -795,16 +814,15 @@ impl DirectXRenderer { return; } match self.draw_external_frame(surface, source, &frame) { - // Only an actual upload is acknowledged. Acknowledging a + // Only a complete upload is acknowledged. Acknowledging a // reuse would make `upload_count` report the compositing // rate, and PERF-B04 reads that counter to prove a settled - // page stops uploading. - Ok(uploaded) => { - if uploaded { - source - .mark_uploaded_for(self.external_texture_consumer, frame.sequence); - } + // page stops uploading; acknowledging a partial upload + // would let the producer drop pixels that never arrived. + Ok(ExternalTextureUpload::Complete) => { + source.mark_uploaded_for(self.external_texture_consumer, frame.sequence); } + Ok(ExternalTextureUpload::Reused | ExternalTextureUpload::Partial) => {} Err(error) => result = Err(error), } }); @@ -815,19 +833,22 @@ impl DirectXRenderer { /// Upload what changed and draw one external texture. /// - /// Returns whether pixels were actually sent to the GPU, so the caller can - /// acknowledge an upload and not a reuse. + /// The outcome tells the caller whether to acknowledge the frame: only + /// [`ExternalTextureUpload::Complete`] means pixels were copied, and only + /// every region the producer reported reaching the GPU counts. fn draw_external_frame( &mut self, surface: &PaintSurface, source: &Arc, frame: &ExternalFrameView<'_>, - ) -> Result { + ) -> Result { let id = source.id(); let width = frame.size.width.0.max(0) as u32; let height = frame.size.height.0.max(0) as u32; if width == 0 || height == 0 { - return Ok(false); + // Nothing to copy. This is not an upload, so the caller must not + // count it as one. + return Ok(ExternalTextureUpload::Reused); } if width > MAX_TEXTURE2D_DIMENSION || height > MAX_TEXTURE2D_DIMENSION { // A frame past the device's limit cannot be a texture at all, and @@ -838,7 +859,7 @@ impl DirectXRenderer { "external texture {id:?} is {width}x{height}, past D3D11's \ {MAX_TEXTURE2D_DIMENSION}-pixel limit; skipping it" ); - return Ok(false); + return Ok(ExternalTextureUpload::Partial); } let plan = plan_external_texture_update( @@ -904,6 +925,7 @@ impl DirectXRenderer { ); } + let mut rejected = false; if !matches!(plan, ExternalTextureUpdate::Reuse) { let cached = self .external_textures @@ -919,6 +941,10 @@ impl DirectXRenderer { } else { frame.dirty_regions() }; + // A region the producer got wrong is dropped rather than + // turned into an out-of-bounds read or an invalid box, and the + // frame is then not acknowledged, so the dirt it covered comes + // back on a later frame. for region in regions { let x = region.origin.x.0.max(0) as u32; let y = region.origin.y.0.max(0) as u32; @@ -927,10 +953,36 @@ impl DirectXRenderer { if region_width == 0 || region_height == 0 { continue; } - let offset = y as usize * frame.stride + x as usize * 4; - let last_row = (y + region_height - 1) as usize * frame.stride - + (x + region_width) as usize * 4; + // The region must fit the frame's own dimensions: + // `dirty_regions` clamps to them, but this function does not + // rely on its caller for a bound the device will reject. + if x.saturating_add(region_width) > width + || y.saturating_add(region_height) > height + { + log::error!( + "external texture {id:?} reported a dirty region outside its own frame" + ); + rejected = true; + continue; + } + let offset = (y as usize) + .checked_mul(frame.stride) + .and_then(|row| row.checked_add(x as usize * 4)); + let last_row = ((y + region_height - 1) as usize) + .checked_mul(frame.stride) + .and_then(|end| end.checked_add((x + region_width) as usize * 4)); + let (Some(offset), Some(last_row)) = (offset, last_row) else { + log::error!( + "external texture {id:?} reported a region that overflows its stride" + ); + rejected = true; + continue; + }; if last_row > frame.bytes.len() { + log::error!( + "external texture {id:?} reported a dirty region outside its own buffer" + ); + rejected = true; continue; } let destination = D3D11_BOX { @@ -941,12 +993,12 @@ impl DirectXRenderer { bottom: y + region_height, back: 1, }; - // SAFETY: `destination` is clipped to the texture by - // `dirty_regions`, and the bounds check above proves the - // source rows for that box are inside `frame.bytes`, which - // stays alive for this call because `with_frame` clones the - // frame snapshot before the visit and the visitor's borrow - // is bounded by this closure. + // SAFETY: `destination` is inside the texture (the frame + // dimensions were checked above) and the bounds check + // proves the source rows for that box are inside + // `frame.bytes`, which stays alive for this call because + // `with_frame` clones the frame snapshot before the visit + // and the visitor's borrow is bounded by this closure. unsafe { devices.device_context.UpdateSubresource( &cached.texture, @@ -958,12 +1010,18 @@ impl DirectXRenderer { ); } } - cached.key = ExternalTextureCacheKey { - id, - generation: frame.generation, - sequence: frame.sequence, - size: frame.size, - }; + if !rejected { + cached.key = ExternalTextureCacheKey { + id, + generation: frame.generation, + sequence: frame.sequence, + size: frame.size, + }; + } + // A rejected region leaves the key where it was, so the next + // frame plans an upload again and the producer's dirt is still + // there to retry. The draw below still runs, so the pane keeps + // showing the last complete texture instead of vanishing. } } @@ -996,7 +1054,13 @@ impl DirectXRenderer { 0, 1, )?; - Ok(!matches!(plan, ExternalTextureUpdate::Reuse)) + if rejected { + return Ok(ExternalTextureUpload::Partial); + } + if matches!(plan, ExternalTextureUpdate::Reuse) { + return Ok(ExternalTextureUpload::Reused); + } + Ok(ExternalTextureUpload::Complete) } pub(crate) fn gpu_specs(&self) -> Result {