From bf1ebfb6fe1c018e7166f3c33bce299147dc0400 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=9B=B7=E7=94=B5=E8=8A=BD=E8=A1=A3?= Date: Tue, 12 May 2026 23:42:52 -0400 Subject: [PATCH] Hold hardware live mode until BGRA presentation --- .../ely_app/src/services/iosurface_metal.rs | 17 ++-- crates/ely_app/src/services/servo_live.rs | 73 ++++++++++++---- crates/ely_app/src/shell/web_surface_frame.rs | 83 +++++++++---------- crates/ely_app/src/shell/web_surface_tests.rs | 22 +++++ crates/ely_app/src/shell/web_surface_view.rs | 11 +-- 5 files changed, 136 insertions(+), 70 deletions(-) diff --git a/crates/ely_app/src/services/iosurface_metal.rs b/crates/ely_app/src/services/iosurface_metal.rs index 93ad8ba..cc65b6e 100644 --- a/crates/ely_app/src/services/iosurface_metal.rs +++ b/crates/ely_app/src/services/iosurface_metal.rs @@ -1,13 +1,12 @@ //! macOS-only import of cross-process IOSurface handles into -//! `CVPixelBuffer`s suitable for GPUI's `Surface` element. +//! `CVPixelBuffer`s that preserve the sidecar's IOSurface identity. //! //! `T10.4` originally imported the IOSurface into an `MTLTexture` -//! directly, but GPUI 0.2.2 already speaks `CVPixelBuffer` end-to-end -//! through `Window::paint_surface` / `elements::surface::Surface`. Its -//! internal Blade Metal renderer takes care of building the Metal -//! texture, so a parallel MTLTexture cache here would be wasted work. -//! The cache now hands the renderer the CVPixelBuffer GPUI already -//! knows how to render. +//! directly. GPUI 0.2.2 exposes `Window::paint_surface` / +//! `elements::surface::Surface` for `CVPixelBuffer`, and that public +//! path is wired for NV12 video frames. Servo's hardware renderer +//! publishes BGRA IOSurfaces, so this cache stays as verified +//! cross-process plumbing until the presenter accepts BGRA surfaces. //! //! Lifetime contract: //! @@ -211,7 +210,9 @@ mod tests { assert!(mach_port != 0, "IOSurfaceCreateMachPort must yield a real port"); let surface_id: u64 = 0xDEAD_BEEFu64; - cache.import(mach_port, surface_id).expect("local IOSurface must round-trip into a CVPixelBuffer"); + cache + .import(mach_port, surface_id) + .expect("local IOSurface must round-trip into a CVPixelBuffer"); let pixel_buffer = cache .pixel_buffer_for(surface_id) diff --git a/crates/ely_app/src/services/servo_live.rs b/crates/ely_app/src/services/servo_live.rs index f5cc5a1..94b857c 100644 --- a/crates/ely_app/src/services/servo_live.rs +++ b/crates/ely_app/src/services/servo_live.rs @@ -10,10 +10,11 @@ use std::{ /// (default — bit-identical to pre-flag builds) and `hardware` (real /// GPU adapter via the vendored `HardwareOffscreenContext`; requires /// the sidecar binary to be compiled with the `hardware-render` -/// feature). Anything else is silently dropped and the sidecar -/// defaults to software so a typo'd value never blocks the browser -/// from starting; the sidecar's own arg parser still errors loudly on -/// an unrecognised value when set explicitly via the flag. +/// feature and a GPUI BGRA surface presenter). Anything else is +/// silently dropped and the sidecar defaults to software so a typo'd +/// value never blocks the browser from starting; the sidecar's own +/// arg parser still errors loudly on an unrecognised value when set +/// explicitly via the flag. const RENDERING_CONTEXT_ENV: &str = "ELY_SERVO_RENDERING_CONTEXT"; use ely_domain::SitePermissionDecision; @@ -265,10 +266,10 @@ pub(crate) struct ServoLiveFrame { #[cfg(all(test, feature = "live-site-smoke"))] sample_hash: u64, rgba_bytes: Vec, - /// Hardware-path companion: when present, the renderer can hand - /// the underlying IOSurface straight to GPUI's Metal pipeline via - /// `gpui::surface(...)` and skip the RGBA upload entirely. Always - /// `None` on the software path and on non-macOS hosts. + /// Hardware-path companion: the imported IOSurface published by + /// the sidecar. GPUI 0.2.2 presents `surface(...)` through its + /// NV12 video path, so the current BGRA Servo surface stays as + /// observability plumbing until a BGRA presenter lands. #[cfg(target_os = "macos")] pixel_buffer: Option, } @@ -294,8 +295,9 @@ impl ServoLiveFrame { } /// Returns the imported `CVPixelBuffer` matching the frame's - /// current hardware surface, if any. The renderer hands this to - /// `gpui::surface(...)` to skip the RGBA→texture upload path. + /// current hardware surface, if any. The renderer keeps this as + /// wire-path evidence while GPUI's public `surface(...)` element + /// remains NV12-only. #[cfg(target_os = "macos")] #[must_use] pub fn pixel_buffer(&self) -> Option<&CVPixelBuffer> { @@ -412,9 +414,52 @@ pub(crate) enum ServoLiveError { /// truth for what values are valid — we just gate which ones we /// forward. fn rendering_context_from_env() -> Option<&'static str> { - match env::var(RENDERING_CONTEXT_ENV).ok()?.to_lowercase().as_str() { - "software" => Some("software"), - "hardware" => Some("hardware"), - _ => None, + let raw = env::var(RENDERING_CONTEXT_ENV).ok()?; + match rendering_context_selection(raw.as_str()) { + RenderingContextSelection::Forward(value) => Some(value), + RenderingContextSelection::HoldHardware => { + tracing::warn!( + target: "ely::servo::iosurface", + "hardware rendering context requested; GPUI 0.2.2 surface presenter accepts NV12 CVPixelBuffers; Servo publishes BGRA IOSurfaces; using software rendering context", + ); + None + } + RenderingContextSelection::Ignore => None, + } +} + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum RenderingContextSelection { + Forward(&'static str), + HoldHardware, + Ignore, +} + +fn rendering_context_selection(raw: &str) -> RenderingContextSelection { + match raw.to_lowercase().as_str() { + "software" => RenderingContextSelection::Forward("software"), + "hardware" => RenderingContextSelection::HoldHardware, + _ => RenderingContextSelection::Ignore, + } +} + +#[cfg(test)] +mod tests { + use super::{RenderingContextSelection, rendering_context_selection}; + + #[test] + fn hardware_env_is_held_until_gpui_can_present_bgra_surfaces() { + assert_eq!( + rendering_context_selection("hardware"), + RenderingContextSelection::HoldHardware + ); + } + + #[test] + fn software_env_still_forwards_to_the_sidecar() { + assert_eq!( + rendering_context_selection("software"), + RenderingContextSelection::Forward("software"), + ); } } diff --git a/crates/ely_app/src/shell/web_surface_frame.rs b/crates/ely_app/src/shell/web_surface_frame.rs index d98bdc6..e284fa1 100644 --- a/crates/ely_app/src/shell/web_surface_frame.rs +++ b/crates/ely_app/src/shell/web_surface_frame.rs @@ -57,15 +57,14 @@ pub(super) struct WebSurfaceFrame { content_pixel_count: u64, #[cfg(all(test, feature = "live-site-smoke"))] sample_hash: u64, - /// Software-path image. `None` whenever the sidecar took the - /// hardware shortcut and dropped the RGBA payload from the - /// wire — the IOSurface in `pixel_buffer` is the source of truth - /// for that frame. + /// Software-path image. Current GPUI builds require this for every + /// ready web frame because BGRA IOSurface presentation is still + /// held at the protocol boundary. pub(super) image: Option>, - /// Hardware-path companion: when present, the view samples the - /// IOSurface through GPUI's Metal pipeline via `gpui::surface(...)` - /// instead of uploading the RGBA bytes again. Always `None` on - /// the software path; `image` is the source of truth there. + /// Hardware-path companion imported from the sidecar. GPUI 0.2.2's + /// public `surface(...)` presenter accepts NV12 video buffers, and + /// Servo publishes BGRA IOSurfaces; this remains observability + /// state until a BGRA presenter is available. #[cfg(target_os = "macos")] pub(super) pixel_buffer: Option, } @@ -103,27 +102,23 @@ impl WebSurfaceFrame { } fn from_parts(parts: WebSurfaceFrameParts) -> Result { - // Hardware path frames arrive with `rgba_bytes` empty — the - // sidecar dropped the 8 MB payload from the wire and the - // receiver samples the IOSurface directly. In that case the - // RGBA hash + LAST_FRAME_IMAGE dedup are skipped entirely. - let image = if parts.rgba_bytes.is_empty() { - None - } else { - // Servo's `read_pixels(gl::RGBA, gl::UNSIGNED_BYTE)` - // writes R-G-B-A in memory order. GPUI's `RenderImage` is - // documented as "in BGRA format" and uploads via - // `MTLPixelFormat::BGRA8Unorm`, which reads B-G-R-A. Hand - // the bytes across unchanged and the Metal sampler treats - // R as B (and vice versa) — every coloured pixel renders - // with R and B swapped. Swap once here so the rest of the - // pipeline (dedup hash, image buffer, GPU upload) all - // operate on the same BGRA representation. - let mut bytes = parts.rgba_bytes; - swap_red_blue_in_place(&mut bytes); - let bytes_hash = rgba_hash(&bytes); - Some(resolve_render_image(parts.width, parts.height, bytes, bytes_hash)?) - }; + if parts.rgba_bytes.is_empty() { + return Err(WebSurfaceError::MissingRenderablePayload); + } + + // Servo's `read_pixels(gl::RGBA, gl::UNSIGNED_BYTE)` writes + // R-G-B-A in memory order. GPUI's `RenderImage` is documented + // as "in BGRA format" and uploads via + // `MTLPixelFormat::BGRA8Unorm`, which reads B-G-R-A. Hand the bytes across + // unchanged and the Metal sampler treats R as B (and vice + // versa) — every coloured pixel renders with R and B swapped. + // Swap once here so the rest of the pipeline (dedup hash, + // image buffer, GPU upload) all operate on the same BGRA + // representation. + let mut bytes = parts.rgba_bytes; + swap_red_blue_in_place(&mut bytes); + let bytes_hash = rgba_hash(&bytes); + let image = Some(resolve_render_image(parts.width, parts.height, bytes, bytes_hash)?); Ok(Self { requested_url: parts.requested_url, @@ -245,6 +240,10 @@ struct WebSurfaceFrameParts { pub(super) enum WebSurfaceError { #[error("invalid servo frame buffer for {width}x{height}")] InvalidFrameBuffer { width: u32, height: u32 }, + #[error( + "servo live frame did not include renderable pixels; BGRA IOSurface presentation is unavailable in GPUI 0.2.2" + )] + MissingRenderablePayload, } /// Swap byte 0 and byte 2 of every 4-byte pixel, converting Servo's @@ -268,20 +267,18 @@ fn resolve_render_image( rgba_bytes: Vec, bytes_hash: u64, ) -> Result, WebSurfaceError> { - LAST_FRAME_IMAGE.with( - |cache| -> Result, WebSurfaceError> { - let mut cache = cache.borrow_mut(); - if let Some((cached_hash, cached_image)) = cache.as_ref() { - if *cached_hash == bytes_hash { - return Ok(cached_image.clone()); - } + LAST_FRAME_IMAGE.with(|cache| -> Result, WebSurfaceError> { + let mut cache = cache.borrow_mut(); + if let Some((cached_hash, cached_image)) = cache.as_ref() { + if *cached_hash == bytes_hash { + return Ok(cached_image.clone()); } + } - let image_buffer = ImageBuffer::, _>::from_raw(width, height, rgba_bytes) - .ok_or(WebSurfaceError::InvalidFrameBuffer { width, height })?; - let new_image = Arc::new(RenderImage::new([image::Frame::new(image_buffer)])); - *cache = Some((bytes_hash, new_image.clone())); - Ok(new_image) - }, - ) + let image_buffer = ImageBuffer::, _>::from_raw(width, height, rgba_bytes) + .ok_or(WebSurfaceError::InvalidFrameBuffer { width, height })?; + let new_image = Arc::new(RenderImage::new([image::Frame::new(image_buffer)])); + *cache = Some((bytes_hash, new_image.clone())); + Ok(new_image) + }) } diff --git a/crates/ely_app/src/shell/web_surface_tests.rs b/crates/ely_app/src/shell/web_surface_tests.rs index c42639e..cabc32a 100644 --- a/crates/ely_app/src/shell/web_surface_tests.rs +++ b/crates/ely_app/src/shell/web_surface_tests.rs @@ -383,6 +383,28 @@ fn live_frame_swaps_red_and_blue_bytes_for_gpui_bgra() -> Result<(), Box Bounds { Bounds::new(point(px(0.0), px(0.0)), size(px(640.0), px(480.0))) } diff --git a/crates/ely_app/src/shell/web_surface_view.rs b/crates/ely_app/src/shell/web_surface_view.rs index 52b83ca..ffca44b 100644 --- a/crates/ely_app/src/shell/web_surface_view.rs +++ b/crates/ely_app/src/shell/web_surface_view.rs @@ -12,7 +12,7 @@ pub(super) fn render_ready_web_surface( tab: &BrowserTab, state_entity: Entity, ) -> AnyElement { - // T14: the `gpui::surface(...)` hardware path is disabled. + // T14: the `gpui::surface(...)` hardware path is held. // // GPUI 0.2.2's Blade Metal renderer hard-asserts that any // CVPixelBuffer handed to `surface(...)` is NV12 YUV @@ -35,10 +35,11 @@ pub(super) fn render_ready_web_surface( img(ImageSource::Render(image.clone())).size_full().object_fit(ObjectFit::Fill), ); } - // Both image variants empty: the sidecar should always publish - // RGBA while the hardware path is disabled, but a blank canvas is - // the honest user-facing fallback if it ever does not. - render_web_surface(tab, state_entity, div().size_full()) + render_web_surface( + tab, + state_entity, + error_page("Web surface frame did not include renderable pixels."), + ) } pub(super) fn render_loading_web_surface(