From c1a70f4eb2a730f4a79bf4c916118145674d59fa Mon Sep 17 00:00:00 2001 From: Martin Robinson Date: Fri, 16 May 2025 20:26:59 +0200 Subject: [PATCH] compositor: Batch all pending scroll event updates into a single transaction (#36974) When multiple WebViews are updating scroll events, instead of taking the list of `WebView`s to avoid a double-borrow, batch scroll events into a single transaction. This should make processing slightly more efficient and avoids having to take the vector of WebViews. Testing: No behavior change here and this aspect of WebView interaction is untestable currently. Signed-off-by: Martin Robinson --- Cargo.lock | 1 + components/compositing/Cargo.toml | 1 + components/compositing/compositor.rs | 39 ++++++++++-- components/compositing/webview_renderer.rs | 72 ++++++++++++---------- 4 files changed, 75 insertions(+), 38 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 89ce8103608..04f5bc15092 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1103,6 +1103,7 @@ dependencies = [ "servo_allocator", "servo_config", "servo_geometry", + "smallvec", "stylo_traits", "surfman", "tracing", diff --git a/components/compositing/Cargo.toml b/components/compositing/Cargo.toml index 8a670a4b4a7..8e788e58734 100644 --- a/components/compositing/Cargo.toml +++ b/components/compositing/Cargo.toml @@ -38,6 +38,7 @@ script_traits = { workspace = true } servo_allocator = { path = "../allocator" } servo_config = { path = "../config" } servo_geometry = { path = "../geometry" } +smallvec = { workspace = true } stylo_traits = { workspace = true } tracing = { workspace = true, optional = true } webrender = { workspace = true } diff --git a/components/compositing/compositor.rs b/components/compositing/compositor.rs index b1669277ba1..74ada7b21c6 100644 --- a/components/compositing/compositor.rs +++ b/components/compositing/compositor.rs @@ -7,7 +7,6 @@ use std::collections::HashMap; use std::env; use std::fs::create_dir_all; use std::iter::once; -use std::mem::take; use std::rc::Rc; use std::sync::Arc; use std::time::{Duration, Instant, SystemTime, UNIX_EPOCH}; @@ -55,7 +54,7 @@ use webrender_api::{ use crate::InitialCompositorState; use crate::webview_manager::WebViewManager; -use crate::webview_renderer::{UnknownWebView, WebViewRenderer}; +use crate::webview_renderer::{PinchZoomResult, UnknownWebView, WebViewRenderer}; #[derive(Debug, PartialEq)] enum UnableToComposite { @@ -1668,11 +1667,39 @@ impl IOCompositor { if let Err(err) = self.rendering_context.make_current() { warn!("Failed to make the rendering context current: {:?}", err); } - let mut webview_renderers = take(&mut self.webview_renderers); - for webview_renderer in webview_renderers.iter_mut() { - webview_renderer.process_pending_scroll_events(self); + + let mut need_zoom = false; + let scroll_offset_updates: Vec<_> = self + .webview_renderers + .iter_mut() + .filter_map(|webview_renderer| { + let (zoom, scroll_result) = + webview_renderer.process_pending_scroll_and_pinch_zoom_events(); + need_zoom = need_zoom || (zoom == PinchZoomResult::DidPinchZoom); + scroll_result + }) + .collect(); + + if need_zoom || !scroll_offset_updates.is_empty() { + let mut transaction = Transaction::new(); + if need_zoom { + self.send_root_pipeline_display_list_in_transaction(&mut transaction); + } + for update in scroll_offset_updates { + let offset = LayoutVector2D::new(-update.offset.x, -update.offset.y); + transaction.set_scroll_offsets( + update.external_scroll_id, + vec![SampledScrollOffset { + offset, + generation: 0, + }], + ); + } + + self.generate_frame(&mut transaction, RenderReasons::APZ); + self.global.borrow_mut().send_transaction(transaction); } - self.webview_renderers = webview_renderers; + self.global.borrow().shutdown_state() != ShutdownState::FinishedShuttingDown } diff --git a/components/compositing/webview_renderer.rs b/components/compositing/webview_renderer.rs index a51dd5f8cda..0a6bdf9ae0a 100644 --- a/components/compositing/webview_renderer.rs +++ b/components/compositing/webview_renderer.rs @@ -20,15 +20,11 @@ use fnv::FnvHashSet; use log::{debug, warn}; use servo_geometry::DeviceIndependentPixel; use style_traits::{CSSPixel, PinchZoomFactor}; -use webrender::Transaction; use webrender_api::units::{ DeviceIntPoint, DeviceIntRect, DevicePixel, DevicePoint, DeviceRect, LayoutVector2D, }; -use webrender_api::{ - ExternalScrollId, HitTestFlags, RenderReasons, SampledScrollOffset, ScrollLocation, -}; +use webrender_api::{ExternalScrollId, HitTestFlags, ScrollLocation}; -use crate::IOCompositor; use crate::compositor::{PipelineDetails, ServoRenderer}; use crate::touch::{TouchHandler, TouchMoveAction, TouchMoveAllowed, TouchSequenceState}; @@ -55,6 +51,19 @@ enum ScrollZoomEvent { Scroll(ScrollEvent), } +#[derive(Clone, Copy, Debug, PartialEq)] +pub(crate) struct ScrollResult { + pub pipeline_id: PipelineId, + pub external_scroll_id: ExternalScrollId, + pub offset: LayoutVector2D, +} + +#[derive(PartialEq)] +pub(crate) enum PinchZoomResult { + DidPinchZoom, + DidNotPinchZoom, +} + /// A renderer for a libservo `WebView`. This is essentially the [`ServoRenderer`]'s interface to a /// libservo `WebView`, but the code here cannot depend on libservo in order to prevent circular /// dependencies, which is why we store a `dyn WebViewTrait` here instead of the `WebView` itself. @@ -737,9 +746,17 @@ impl WebViewRenderer { self.on_scroll_window_event(scroll_location, cursor) } - pub(crate) fn process_pending_scroll_events(&mut self, compositor: &mut IOCompositor) { + /// Process pending scroll events for this [`WebViewRenderer`]. Returns a tuple containing: + /// + /// - A boolean that is true if a zoom occurred. + /// - An optional [`ScrollResult`] if a scroll occurred. + /// + /// It is up to the caller to ensure that these events update the rendering appropriately. + pub(crate) fn process_pending_scroll_and_pinch_zoom_events( + &mut self, + ) -> (PinchZoomResult, Option) { if self.pending_scroll_zoom_events.is_empty() { - return; + return (PinchZoomResult::DidNotPinchZoom, None); } // Batch up all scroll events into one, or else we'll do way too much painting. @@ -790,37 +807,24 @@ impl WebViewRenderer { } } - let zoom_changed = - self.set_pinch_zoom_level(self.pinch_zoom_level().get() * combined_magnification); let scroll_result = combined_scroll_event.and_then(|combined_event| { self.scroll_node_at_device_point( combined_event.cursor.to_f32(), combined_event.scroll_location, ) }); - if !zoom_changed && scroll_result.is_none() { - return; + if let Some(scroll_result) = scroll_result { + self.send_scroll_positions_to_layout_for_pipeline(scroll_result.pipeline_id); } - let mut transaction = Transaction::new(); - if zoom_changed { - compositor.send_root_pipeline_display_list_in_transaction(&mut transaction); - } + let pinch_zoom_result = match self + .set_pinch_zoom_level(self.pinch_zoom_level().get() * combined_magnification) + { + true => PinchZoomResult::DidPinchZoom, + false => PinchZoomResult::DidNotPinchZoom, + }; - if let Some((pipeline_id, external_id, offset)) = scroll_result { - let offset = LayoutVector2D::new(-offset.x, -offset.y); - transaction.set_scroll_offsets( - external_id, - vec![SampledScrollOffset { - offset, - generation: 0, - }], - ); - self.send_scroll_positions_to_layout_for_pipeline(pipeline_id); - } - - compositor.generate_frame(&mut transaction, RenderReasons::APZ); - self.global.borrow_mut().send_transaction(transaction); + (pinch_zoom_result, scroll_result) } /// Perform a hit test at the given [`DevicePoint`] and apply the [`ScrollLocation`] @@ -831,7 +835,7 @@ impl WebViewRenderer { &mut self, cursor: DevicePoint, scroll_location: ScrollLocation, - ) -> Option<(PipelineId, ExternalScrollId, LayoutVector2D)> { + ) -> Option { let scroll_location = match scroll_location { ScrollLocation::Delta(delta) => { let device_pixels_per_page = self.device_pixels_per_page_pixel(); @@ -871,8 +875,12 @@ impl WebViewRenderer { let scroll_result = pipeline_details .scroll_tree .scroll_node_or_ancestor(scroll_tree_node, scroll_location); - if let Some((external_id, offset)) = scroll_result { - return Some((*pipeline_id, external_id, offset)); + if let Some((external_scroll_id, offset)) = scroll_result { + return Some(ScrollResult { + pipeline_id: *pipeline_id, + external_scroll_id, + offset, + }); } } }