From e5c9a0365db39eaba109132b8d545ef4732ed16e Mon Sep 17 00:00:00 2001 From: Martin Robinson Date: Wed, 19 Feb 2025 11:35:56 +0100 Subject: [PATCH] libservo: Rework and clarify the rendering model of the `WebView` (#35522) Make the rendering model of the `WebView` clearer: 1. `WebViewDelegate::notify_new_frame_ready()` indicates that the WebView has become dirty and needs to be repainted. 2. `WebView::paint()` asks Servo to paint the contents of the `WebView` into the `RenderingContext`. 3. `RenderingContext::present()` does a buffer swap if the `RenderingContext` is actually double-buffered. This is documented and all in-tree embedders are updated to work with this new model. Signed-off-by: Martin Robinson Co-authored-by: Ngo Iok Ui (Wu Yu Wei) Co-authored-by: Mukilan Thiyagarajan --- Cargo.lock | 1 + components/compositing/Cargo.toml | 1 + components/compositing/compositor.rs | 164 +++++++-------------- components/servo/examples/winit_minimal.rs | 17 ++- components/servo/lib.rs | 16 +- components/servo/webview.rs | 35 ++++- ports/servoshell/desktop/app.rs | 20 +-- ports/servoshell/desktop/app_state.rs | 29 ++-- ports/servoshell/desktop/minibrowser.rs | 2 + ports/servoshell/egl/app_state.rs | 4 +- 10 files changed, 129 insertions(+), 160 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7612454954a..63a78eb5008 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1035,6 +1035,7 @@ name = "compositing" version = "0.0.1" dependencies = [ "base", + "bitflags 2.8.0", "compositing_traits", "crossbeam-channel", "embedder_traits", diff --git a/components/compositing/Cargo.toml b/components/compositing/Cargo.toml index 4382036accc..955ceead971 100644 --- a/components/compositing/Cargo.toml +++ b/components/compositing/Cargo.toml @@ -19,6 +19,7 @@ webxr = ["dep:webxr"] [dependencies] base = { workspace = true } +bitflags = { workspace = true } compositing_traits = { workspace = true } crossbeam-channel = { workspace = true } embedder_traits = { workspace = true } diff --git a/components/compositing/compositor.rs b/components/compositing/compositor.rs index 54527b72de6..cd05dda82e7 100644 --- a/components/compositing/compositor.rs +++ b/components/compositing/compositor.rs @@ -2,7 +2,7 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ -use std::collections::hash_set::Iter; +use std::cell::Cell; use std::collections::HashMap; use std::env; use std::fs::{create_dir_all, File}; @@ -15,6 +15,7 @@ use std::time::{Duration, Instant, SystemTime, UNIX_EPOCH}; use base::cross_process_instant::CrossProcessInstant; use base::id::{PipelineId, TopLevelBrowsingContextId, WebViewId}; use base::{Epoch, WebRenderEpochToU16}; +use bitflags::bitflags; use compositing_traits::{ CompositionPipeline, CompositorMsg, CompositorReceiver, ConstellationMsg, SendableFrameTree, }; @@ -122,8 +123,8 @@ pub struct IOCompositor { /// The type of composition to perform composite_target: CompositeTarget, - /// Tracks whether we should composite this frame. - composition_request: CompositionRequest, + /// Tracks whether or not the view needs to be repainted. + needs_repaint: Cell, /// Tracks whether we are in the process of shutting down, or have shut down and should close /// the compositor. @@ -198,10 +199,6 @@ pub struct IOCompositor { /// The number of frames pending to receive from WebRender. pending_frames: usize, - /// A list of [`WebViewId`]s of WebViews that have new frames ready that are waiting to - /// be presented. - webviews_waiting_on_present: FnvHashSet, - /// The [`Instant`] of the last animation tick, used to avoid flooding the Constellation and /// ScriptThread with a deluge of animation ticks. last_animation_tick: Instant, @@ -230,27 +227,21 @@ enum ScrollZoomEvent { Scroll(ScrollEvent), } -/// Why we performed a composite. This is used for debugging. -/// -/// TODO: It would be good to have a bit more precision here about why a composite -/// was originally triggered, but that would require tracking the reason when a -/// frame is queued in WebRender and then remembering when the frame is ready. -#[derive(Clone, Copy, Debug, PartialEq)] -enum CompositingReason { - /// We're performing the single composite in headless mode. - Headless, - /// We're performing a composite to run an animation. - Animation, - /// A new WebRender frame has arrived. - NewWebRenderFrame, - /// The window has been resized and will need to be synchronously repainted. - Resize, -} +/// Why we need to be repainted. This is used for debugging. +#[derive(Clone, Copy, Default)] +struct RepaintReason(u8); -#[derive(Debug, PartialEq)] -enum CompositionRequest { - NoCompositingNecessary, - CompositeNow(CompositingReason), +bitflags! { + impl RepaintReason: u8 { + /// We're performing the single repaint in headless mode. + const ReadyForScreenshot = 1 << 0; + /// We're performing a repaint to run an animation. + const ChangedAnimationState = 1 << 1; + /// A new WebRender frame has arrived. + const NewWebRenderFrame = 1 << 2; + /// The window has been resized and will need to be synchronously repainted. + const Resize = 1 << 3; + } } #[derive(Clone, Copy, Debug, PartialEq)] @@ -355,7 +346,7 @@ impl IOCompositor { port: state.receiver, webviews: WebViewManager::default(), pipeline_details: HashMap::new(), - composition_request: CompositionRequest::NoCompositingNecessary, + needs_repaint: Cell::default(), touch_handler: TouchHandler::new(), pending_scroll_zoom_events: Vec::new(), composite_target, @@ -383,7 +374,6 @@ impl IOCompositor { exit_after_load, convert_mouse_to_touch, pending_frames: 0, - webviews_waiting_on_present: Default::default(), last_animation_tick: Instant::now(), version_string, }; @@ -404,8 +394,14 @@ impl IOCompositor { } } - pub fn webviews_waiting_on_present(&self) -> Iter<'_, WebViewId> { - self.webviews_waiting_on_present.iter() + fn set_needs_repaint(&self, reason: RepaintReason) { + let mut needs_repaint = self.needs_repaint.get(); + needs_repaint.insert(reason); + self.needs_repaint.set(needs_repaint); + } + + pub fn needs_repaint(&self) -> bool { + !self.needs_repaint.get().is_empty() } fn update_cursor(&mut self, result: &CompositorHitTestResult) { @@ -433,14 +429,12 @@ impl IOCompositor { } } - pub fn maybe_start_shutting_down(&mut self) { - if self.shutdown_state == ShutdownState::NotShuttingDown { - debug!("Shutting down the constellation for WindowEvent::Quit"); - self.start_shutting_down(); + pub fn start_shutting_down(&mut self) { + if self.shutdown_state != ShutdownState::NotShuttingDown { + warn!("Requested shutdown while already shutting down"); + return; } - } - fn start_shutting_down(&mut self) { debug!("Compositor sending Exit message to Constellation"); if let Err(e) = self.constellation_chan.send(ConstellationMsg::Exit) { warn!("Sending exit message to constellation failed ({:?}).", e); @@ -525,7 +519,7 @@ impl IOCompositor { } else { self.ready_to_save_state = ReadyState::Unknown; } - self.composite_if_necessary(CompositingReason::Headless); + self.set_needs_repaint(RepaintReason::ReadyForScreenshot); }, CompositorMsg::SetThrottled(pipeline_id, throttled) => { @@ -549,7 +543,7 @@ impl IOCompositor { } if recomposite_needed || self.animation_callbacks_active() { - self.composite_if_necessary(CompositingReason::NewWebRenderFrame) + self.set_needs_repaint(RepaintReason::NewWebRenderFrame); } }, @@ -558,7 +552,7 @@ impl IOCompositor { if matches!(self.composite_target, CompositeTarget::PngFile(_)) || self.exit_after_load { - self.composite_if_necessary(CompositingReason::Headless); + self.set_needs_repaint(RepaintReason::ReadyForScreenshot); } }, @@ -903,7 +897,7 @@ impl IOCompositor { let throttled = self.pipeline_details(pipeline_id).throttled; self.pipeline_details(pipeline_id).animations_running = true; if !throttled { - self.composite_if_necessary(CompositingReason::Animation); + self.set_needs_repaint(RepaintReason::ChangedAnimationState); } }, AnimationState::AnimationCallbacksPresent => { @@ -1297,7 +1291,7 @@ impl IOCompositor { } self.update_after_zoom_or_hidpi_change(); - self.composite_if_necessary(CompositingReason::Resize); + self.set_needs_repaint(RepaintReason::Resize); true } @@ -1953,18 +1947,21 @@ impl IOCompositor { } pub fn composite(&mut self) { - match self.composite_specific_target(self.composite_target.clone(), None) { - Ok(_) => { - if matches!(self.composite_target, CompositeTarget::PngFile(_)) || - self.exit_after_load - { - println!("Shutting down the Constellation after generating an output file or exit flag specified"); - self.start_shutting_down(); - } - }, - Err(error) => { - trace!("Unable to composite: {error:?}"); - }, + if let Err(error) = self.composite_specific_target(self.composite_target.clone(), None) { + warn!("Unable to composite: {error:?}"); + return; + } + + // We've painted the default target, which means that from the embedder's perspective, + // the scene no longer needs to be repainted. + self.needs_repaint.set(RepaintReason::empty()); + + // Queue up any subsequent paints for animations. + self.process_animations(true); + + if matches!(self.composite_target, CompositeTarget::PngFile(_)) || self.exit_after_load { + println!("Shutting down the Constellation after generating an output file or exit flag specified"); + self.start_shutting_down(); } } @@ -2090,19 +2087,6 @@ impl IOCompositor { }, }; - #[cfg(feature = "tracing")] - let _span = - tracing::trace_span!("ConstellationMsg::ReadyToPresent", servo_profiling = true) - .entered(); - - // Notify embedder that servo is ready to present. - // Embedder should call `present` to tell compositor to continue rendering. - self.webviews_waiting_on_present - .extend(self.webviews.painting_order().map(|(&id, _)| id)); - self.composition_request = CompositionRequest::NoCompositingNecessary; - - self.process_animations(true); - Ok(rv) } @@ -2172,31 +2156,13 @@ impl IOCompositor { } } - #[cfg_attr( - feature = "tracing", - tracing::instrument(skip_all, fields(servo_profiling = true), level = "trace") - )] - pub fn present(&mut self) { - #[cfg(feature = "tracing")] - let _span = - tracing::trace_span!("Compositor Present Surface", servo_profiling = true).entered(); - self.rendering_context.present(); - self.webviews_waiting_on_present.clear(); - } - - fn composite_if_necessary(&mut self, reason: CompositingReason) { - trace!( - "Will schedule a composite {reason:?}. Previously was {:?}", - self.composition_request - ); - self.composition_request = CompositionRequest::CompositeNow(reason) - } - fn clear_background(&self) { let gl = &self.webrender_gl; self.assert_gl_framebuffer_complete(); - // Set the viewport background based on prefs. + // Always clear the entire RenderingContext, regardless of how many WebViews there are + // or where they are positioned. This is so WebView actually clears even before the + // first WebView is ready. let color = servo_config::pref!(shell_background_color_rgba); gl.clear_color( color[0] as f32, @@ -2204,22 +2170,7 @@ impl IOCompositor { color[2] as f32, color[3] as f32, ); - - // Clear the viewport rect of each top-level browsing context. - for (_, webview) in self.webviews.painting_order() { - let rect = self.embedder_coordinates.flip_rect(&webview.rect.to_i32()); - gl.scissor( - rect.min.x, - rect.min.y, - rect.size().width, - rect.size().height, - ); - gl.enable(gleam::gl::SCISSOR_TEST); - gl.clear(gleam::gl::COLOR_BUFFER_BIT); - gl.disable(gleam::gl::SCISSOR_TEST); - } - - self.assert_gl_framebuffer_complete(); + gl.clear(gleam::gl::COLOR_BUFFER_BIT); } #[track_caller] @@ -2288,11 +2239,6 @@ impl IOCompositor { self.zoom_action = false; } - match self.composition_request { - CompositionRequest::NoCompositingNecessary => {}, - CompositionRequest::CompositeNow(_) => self.composite(), - } - #[cfg(feature = "webxr")] // Run the WebXR main thread self.webxr_main_thread.run_one_frame(); diff --git a/components/servo/examples/winit_minimal.rs b/components/servo/examples/winit_minimal.rs index 2b15acc8e02..388b9a6b749 100644 --- a/components/servo/examples/winit_minimal.rs +++ b/components/servo/examples/winit_minimal.rs @@ -8,7 +8,7 @@ use std::rc::Rc; use compositing::windowing::{AnimationState, EmbedderMethods, WindowMethods}; use euclid::{Point2D, Scale, Size2D}; -use servo::{RenderingContext, Servo, TouchEventAction, WebView, WindowRenderingContext}; +use servo::{RenderingContext, Servo, TouchEventType, WebView, WindowRenderingContext}; use servo_geometry::DeviceIndependentPixel; use tracing::warn; use url::Url; @@ -44,6 +44,7 @@ fn main() -> Result<(), Box> { struct AppState { window_delegate: Rc, servo: Servo, + rendering_context: Rc, webviews: RefCell>, } @@ -93,9 +94,10 @@ impl ApplicationHandler for App { .expect("Failed to create winit Window"); let window_handle = window.window_handle().expect("Failed to get window handle"); - let rendering_context = + let rendering_context = Rc::new( WindowRenderingContext::new(display_handle, window_handle, &window.inner_size()) - .expect("Could not create RenderingContext for window."); + .expect("Could not create RenderingContext for window."), + ); let window_delegate = Rc::new(WindowDelegate::new(window)); let _ = rendering_context.make_current(); @@ -103,7 +105,7 @@ impl ApplicationHandler for App { let servo = Servo::new( Default::default(), Default::default(), - Rc::new(rendering_context), + rendering_context.clone(), Box::new(EmbedderDelegate { waker: waker.clone(), }), @@ -116,6 +118,7 @@ impl ApplicationHandler for App { let app_state = Rc::new(AppState { window_delegate, servo, + rendering_context, webviews: Default::default(), }); @@ -152,8 +155,8 @@ impl ApplicationHandler for App { }, WindowEvent::RedrawRequested => { if let Self::Running(state) = self { - state.webviews.borrow().last().unwrap().paint_immediately(); - state.servo.present(); + state.webviews.borrow().last().unwrap().paint(); + state.rendering_context.present(); } }, WindowEvent::MouseWheel { delta, .. } => { @@ -170,7 +173,7 @@ impl ApplicationHandler for App { webview.notify_scroll_event( ScrollLocation::Delta(moved_by), DeviceIntPoint::new(10, 10), - TouchEventAction::Down, + TouchEventType::Down, ); } } diff --git a/components/servo/lib.rs b/components/servo/lib.rs index 30164692c9d..6e783152f8f 100644 --- a/components/servo/lib.rs +++ b/components/servo/lib.rs @@ -658,11 +658,15 @@ impl Servo { } fn send_new_frame_ready_messages(&self) { + if !self.compositor.borrow().needs_repaint() { + return; + } + for webview in self - .compositor + .webviews .borrow() - .webviews_waiting_on_present() - .filter_map(|id| self.get_webview_handle(*id)) + .values() + .filter_map(WebView::from_weak_handle) { webview.delegate().notify_new_frame_ready(webview); } @@ -686,7 +690,7 @@ impl Servo { } pub fn start_shutting_down(&self) { - self.compositor.borrow_mut().maybe_start_shutting_down(); + self.compositor.borrow_mut().start_shutting_down(); } pub fn allow_navigation_response(&self, pipeline_id: PipelineId, allow: bool) { @@ -703,10 +707,6 @@ impl Servo { self.compositor.borrow_mut().deinit(); } - pub fn present(&self) { - self.compositor.borrow_mut().present(); - } - pub fn new_webview(&self, url: url::Url) -> WebView { let webview = WebView::new(&self.constellation_proxy, self.compositor.clone()); self.webviews diff --git a/components/servo/webview.rs b/components/servo/webview.rs index f7264607d37..6261f8205b3 100644 --- a/components/servo/webview.rs +++ b/components/servo/webview.rs @@ -23,6 +23,33 @@ use crate::clipboard_delegate::{ClipboardDelegate, DefaultClipboardDelegate}; use crate::webview_delegate::{DefaultWebViewDelegate, WebViewDelegate}; use crate::ConstellationProxy; +/// A handle to a Servo webview. If you clone this handle, it does not create a new webview, +/// but instead creates a new handle to the webview. Once the last handle is dropped, Servo +/// considers that the webview has closed and will clean up all associated resources related +/// to this webview. +/// +/// ## Rendering Model +/// +/// Every [`WebView`] has a [`RenderingContext`](crate::RenderingContext). The embedder manages when +/// the contents of the [`WebView`] paint to the [`RenderingContext`](crate::RenderingContext). When +/// a [`WebView`] needs to be painted, for instance, because its contents have changed, Servo will +/// call [`WebViewDelegate::notify_new_frame_ready`] in order to signal that it is time to repaint +/// the [`WebView`] using [`WebView::paint`]. +/// +/// An example of how this flow might work is: +/// +/// 1. [`WebViewDelegate::notify_new_frame_ready`] is called. The applications triggers a request +/// to repaint the window that contains this [`WebView`]. +/// 2. During window repainting, the application calls [`WebView::paint`] and the contents of the +/// [`RenderingContext`][crate::RenderingContext] are updated. +/// 3. If the [`RenderingContext`][crate::RenderingContext] is double-buffered, the +/// application then calls [`crate::RenderingContext::present()`] in order to swap the back buffer +/// to the front, finally displaying the updated [`WebView`] contents. +/// +/// In cases where the [`WebView`] contents have not been updated, but a repaint is necessary, for +/// instance when repainting a window due to damage, an application may simply perform the final two +/// steps and Servo will repaint even without first calling the +/// [`WebViewDelegate::notify_new_frame_ready`] method. #[derive(Clone)] pub struct WebView(Rc>); @@ -63,12 +90,6 @@ impl Drop for WebViewInner { } } -/// Handle for a webview. -/// -/// - The webview exists for exactly as long as there are WebView handles -/// (FIXME: this is not true yet; webviews can still close of their own volition) -/// - All methods are infallible; if the constellation dies, the embedder finds out when calling -/// [Servo::handle_events](crate::Servo::handle_events) impl WebView { pub(crate) fn new( constellation_proxy: &ConstellationProxy, @@ -415,7 +436,7 @@ impl WebView { .send(ConstellationMsg::SendError(Some(self.id()), message)); } - pub fn paint_immediately(&self) { + pub fn paint(&self) { self.inner().compositor.borrow_mut().composite(); } } diff --git a/ports/servoshell/desktop/app.rs b/ports/servoshell/desktop/app.rs index aa54b0a3b7c..54a49caceb3 100644 --- a/ports/servoshell/desktop/app.rs +++ b/ports/servoshell/desktop/app.rs @@ -58,7 +58,6 @@ pub(crate) enum PumpResult { Shutdown, Continue { need_update: bool, - new_servo_frame: bool, need_window_redraw: bool, }, } @@ -195,23 +194,15 @@ impl App { }, PumpResult::Continue { need_update: update, - new_servo_frame, need_window_redraw, } => { - // A new Servo frame is ready, so swap the buffer on our `RenderingContext`. In headed mode - // this won't immediately update the widget surface, because we render to an offscreen - // `RenderingContext`. - if new_servo_frame { - state.servo().present(); - } - let updated = match (update, &mut self.minibrowser) { (true, Some(minibrowser)) => minibrowser.update_webview_data(state), _ => false, }; // If in headed mode, request a winit redraw event, so we can paint the minibrowser. - if updated || need_window_redraw || new_servo_frame { + if updated || need_window_redraw { if let Some(window) = window.winit_window() { window.request_redraw(); } @@ -247,14 +238,7 @@ impl App { state.shutdown(); self.state = AppState::ShuttingDown; }, - PumpResult::Continue { - new_servo_frame, .. - } => { - if new_servo_frame { - // In headless mode, we present directly. - state.servo().present(); - } - }, + PumpResult::Continue { .. } => state.repaint_servo_if_necessary(), } !matches!(self.state, AppState::ShuttingDown) diff --git a/ports/servoshell/desktop/app_state.rs b/ports/servoshell/desktop/app_state.rs index 848c78b8dd4..9ef17477c95 100644 --- a/ports/servoshell/desktop/app_state.rs +++ b/ports/servoshell/desktop/app_state.rs @@ -73,8 +73,9 @@ pub struct RunningAppStateInner { /// Whether or not the application interface needs to be updated. need_update: bool, - /// Whether or not the application needs to be redrawn. - new_servo_frame_ready: bool, + /// Whether or not Servo needs to repaint its display. Currently this is global + /// because every `WebView` shares a `RenderingContext`. + need_repaint: bool, } impl Drop for RunningAppState { @@ -101,7 +102,7 @@ impl RunningAppState { window, gamepad_support: GamepadSupport::maybe_new(), need_update: false, - new_servo_frame_ready: false, + need_repaint: false, }), } } @@ -124,6 +125,19 @@ impl RunningAppState { &self.servo } + pub(crate) fn repaint_servo_if_necessary(&self) { + if !self.inner().need_repaint { + return; + } + let Some(webview) = self.focused_webview() else { + return; + }; + + webview.paint(); + self.inner().window.rendering_context().present(); + self.inner_mut().need_repaint = false; + } + /// Spins the internal application event loop. /// /// - Notifies Servo about incoming gamepad events @@ -138,15 +152,12 @@ impl RunningAppState { } // Delegate handlers may have asked us to present or update compositor contents. - let new_servo_frame = std::mem::replace(&mut self.inner_mut().new_servo_frame_ready, false); - let need_update = std::mem::replace(&mut self.inner_mut().need_update, false); - // Currently, egui-file-dialog dialogs need to be constantly redrawn or animations aren't fluid. - let need_window_redraw = new_servo_frame || self.has_active_dialog(); + let need_window_redraw = self.inner().need_repaint || self.has_active_dialog(); + let need_update = std::mem::replace(&mut self.inner_mut().need_update, false); PumpResult::Continue { need_update, - new_servo_frame, need_window_redraw, } } @@ -484,7 +495,7 @@ impl WebViewDelegate for RunningAppState { } fn notify_new_frame_ready(&self, _webview: servo::WebView) { - self.inner_mut().new_servo_frame_ready = true; + self.inner_mut().need_repaint = true; } fn play_gamepad_haptic_effect( diff --git a/ports/servoshell/desktop/minibrowser.rs b/ports/servoshell/desktop/minibrowser.rs index bc5cb9710cb..d8b223aca45 100644 --- a/ports/servoshell/desktop/minibrowser.rs +++ b/ports/servoshell/desktop/minibrowser.rs @@ -403,6 +403,8 @@ impl Minibrowser { ); } + state.repaint_servo_if_necessary(); + if let Some(render_to_parent) = rendering_context.render_to_parent_callback() { ui.painter().add(PaintCallback { rect, diff --git a/ports/servoshell/egl/app_state.rs b/ports/servoshell/egl/app_state.rs index 651a593a22c..3317b673be2 100644 --- a/ports/servoshell/egl/app_state.rs +++ b/ports/servoshell/egl/app_state.rs @@ -680,8 +680,8 @@ impl RunningAppState { pub fn present_if_needed(&self) { if self.inner().need_present { self.inner_mut().need_present = false; - self.active_webview().paint_immediately(); - self.servo.present(); + self.active_webview().paint(); + self.rendering_context.present(); } } }