From 71bfd2d13f4f8c895f43934205c6eb6b12011e80 Mon Sep 17 00:00:00 2001 From: Martin Robinson Date: Wed, 12 Feb 2025 09:25:58 +0100 Subject: [PATCH] libservo: Don't bounce ready-to-present frame notifications to the Constellation (#35369) Instead of telling the Constellation to tell the embedder that new frames are ready, have the compositor tell the embedder directly. This should reduce frame latency. Now, after processing compositor updates, run any pending `WebView::new_frame_ready` delegate methods. This change also removes the `refresh` call from the Java interface as that was the only other place that the compositor was rendering the WebRender scene outside of event looping spinning. This `refresh` call was completely unused. Signed-off-by: Martin Robinson --- components/compositing/compositor.rs | 26 ++++++++++--------- components/constellation/constellation.rs | 10 ------- components/constellation/tracing.rs | 2 -- components/servo/lib.rs | 19 +++++++++----- components/servo/webview.rs | 4 --- .../shared/compositing/constellation_msg.rs | 3 --- components/shared/embedder/lib.rs | 3 --- ports/servoshell/egl/android.rs | 9 ------- ports/servoshell/egl/app_state.rs | 7 ----- .../java/org/servo/servoview/JNIServo.java | 2 -- .../main/java/org/servo/servoview/Servo.java | 4 --- 11 files changed, 26 insertions(+), 63 deletions(-) diff --git a/components/compositing/compositor.rs b/components/compositing/compositor.rs index 6aa3e74518f..9d9c917672e 100644 --- a/components/compositing/compositor.rs +++ b/components/compositing/compositor.rs @@ -3,6 +3,7 @@ * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ use std::cell::OnceCell; +use std::collections::hash_set::Iter; use std::collections::HashMap; use std::env; use std::fs::{create_dir_all, File}; @@ -212,8 +213,9 @@ pub struct IOCompositor { /// The number of frames pending to receive from WebRender. pending_frames: usize, - /// Waiting for external code to call present. - waiting_on_present: bool, + /// 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. @@ -403,7 +405,7 @@ impl IOCompositor { exit_after_load, convert_mouse_to_touch, pending_frames: 0, - waiting_on_present: false, + webviews_waiting_on_present: Default::default(), last_animation_tick: Instant::now(), version_string, }; @@ -424,6 +426,10 @@ impl IOCompositor { } } + pub fn webviews_waiting_on_present(&self) -> Iter<'_, WebViewId> { + self.webviews_waiting_on_present.iter() + } + fn update_cursor(&mut self, result: CompositorHitTestResult) { let cursor = match result.cursor { Some(cursor) if cursor != self.cursor => cursor, @@ -1976,7 +1982,7 @@ impl IOCompositor { target: CompositeTarget, page_rect: Option>, ) -> Result, UnableToComposite> { - if self.waiting_on_present { + if !self.webviews_waiting_on_present.is_empty() { debug!("tried to composite while waiting on present"); return Err(UnableToComposite::NotReadyToPaintImage( NotReadyToPaint::WaitingOnConstellation, @@ -2146,15 +2152,11 @@ impl IOCompositor { 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.waiting_on_present = true; - let webview_ids = self.webviews.painting_order().map(|(&id, _)| id); - let msg = ConstellationMsg::ReadyToPresent(webview_ids.collect()); - if let Err(e) = self.constellation_chan.send(msg) { - warn!("Sending event to constellation failed ({:?}).", e); - } - + self.webviews_waiting_on_present + .extend(self.webviews.painting_order().map(|(&id, _)| id)); self.composition_request = CompositionRequest::NoCompositingNecessary; self.process_animations(true); @@ -2245,7 +2247,7 @@ impl IOCompositor { let _span = tracing::trace_span!("Compositor Present Surface", servo_profiling = true).entered(); self.rendering_context.present(); - self.waiting_on_present = false; + self.webviews_waiting_on_present.clear(); } fn composite_if_necessary(&mut self, reason: CompositingReason) { diff --git a/components/constellation/constellation.rs b/components/constellation/constellation.rs index f26a91290bc..ed782036d3f 100644 --- a/components/constellation/constellation.rs +++ b/components/constellation/constellation.rs @@ -1459,16 +1459,6 @@ where FromCompositorMsg::SetWebViewThrottled(webview_id, throttled) => { self.set_webview_throttled(webview_id, throttled); }, - FromCompositorMsg::ReadyToPresent(webview_ids) => { - #[cfg(feature = "tracing")] - let _span = tracing::trace_span!( - "FromCompositorMsg::ReadyToPresent", - servo_profiling = true, - ) - .entered(); - self.embedder_proxy - .send(EmbedderMsg::ReadyToPresent(webview_ids)); - }, FromCompositorMsg::Gamepad(gamepad_event) => { self.handle_gamepad_msg(gamepad_event); }, diff --git a/components/constellation/tracing.rs b/components/constellation/tracing.rs index 419ee311d7f..97896bef813 100644 --- a/components/constellation/tracing.rs +++ b/components/constellation/tracing.rs @@ -90,7 +90,6 @@ mod from_compositor { Self::MediaSessionAction(_) => target!("MediaSessionAction"), Self::SetWebViewThrottled(_, _) => target!("SetWebViewThrottled"), Self::IMEDismissed => target!("IMEDismissed"), - Self::ReadyToPresent(..) => target!("ReadyToPresent"), Self::Gamepad(..) => target!("Gamepad"), Self::Clipboard(..) => target!("Clipboard"), } @@ -248,7 +247,6 @@ mod from_script { Self::MediaSessionEvent(..) => target_variant!("MediaSessionEvent"), Self::OnDevtoolsStarted(..) => target_variant!("OnDevtoolsStarted"), Self::RequestDevtoolsConnection(..) => target_variant!("RequestDevtoolsConnection"), - Self::ReadyToPresent(..) => target_variant!("ReadyToPresent"), Self::EventDelivered(..) => target_variant!("EventDelivered"), Self::PlayGamepadHapticEffect(..) => target_variant!("PlayGamepadHapticEffect"), Self::StopGamepadHapticEffect(..) => target_variant!("StopGamepadHapticEffect"), diff --git a/components/servo/lib.rs b/components/servo/lib.rs index d533c392fbd..84aa5bffd1f 100644 --- a/components/servo/lib.rs +++ b/components/servo/lib.rs @@ -643,6 +643,7 @@ impl Servo { } self.compositor.borrow_mut().perform_updates(); + self.send_new_frame_ready_messages(); if self.compositor.borrow().shutdown_state == ShutdownState::FinishedShuttingDown { return false; @@ -651,6 +652,17 @@ impl Servo { true } + fn send_new_frame_ready_messages(&self) { + for webview in self + .compositor + .borrow() + .webviews_waiting_on_present() + .filter_map(|id| self.get_webview_handle(*id)) + { + webview.delegate().notify_new_frame_ready(webview); + } + } + pub fn pinch_zoom_level(&self) -> f32 { self.compositor.borrow_mut().pinch_zoom_level().get() } @@ -987,13 +999,6 @@ impl Servo { }, ); }, - EmbedderMsg::ReadyToPresent(webview_ids) => { - for webview_id in webview_ids { - if let Some(webview) = self.get_webview_handle(webview_id) { - webview.delegate().notify_new_frame_ready(webview); - } - } - }, EmbedderMsg::EventDelivered(webview_id, event) => { if let Some(webview) = self.get_webview_handle(webview_id) { webview.delegate().notify_event_delivered(webview, event); diff --git a/components/servo/webview.rs b/components/servo/webview.rs index 491590defb3..677913ed7d5 100644 --- a/components/servo/webview.rs +++ b/components/servo/webview.rs @@ -441,10 +441,6 @@ impl WebView { self.inner().compositor.borrow_mut().capture_webrender(); } - pub fn composite(&self) { - self.inner().compositor.borrow_mut().composite(); - } - pub fn toggle_sampling_profiler(&self, rate: Duration, max_duration: Duration) { self.inner() .constellation_proxy diff --git a/components/shared/compositing/constellation_msg.rs b/components/shared/compositing/constellation_msg.rs index 102a65dad19..a794d7b3044 100644 --- a/components/shared/compositing/constellation_msg.rs +++ b/components/shared/compositing/constellation_msg.rs @@ -84,8 +84,6 @@ pub enum ConstellationMsg { SetWebViewThrottled(TopLevelBrowsingContextId, bool), /// Virtual keyboard was dismissed IMEDismissed, - /// Notify the embedder that it needs to present a new frame. - ReadyToPresent(Vec), /// Gamepad state has changed Gamepad(GamepadEvent), /// Inform the constellation of a clipboard event. @@ -133,7 +131,6 @@ impl ConstellationMsg { SetWebViewThrottled(..) => "SetWebViewThrottled", IMEDismissed => "IMEDismissed", ClearCache => "ClearCache", - ReadyToPresent(..) => "ReadyToPresent", Gamepad(..) => "Gamepad", Clipboard(..) => "Clipboard", } diff --git a/components/shared/embedder/lib.rs b/components/shared/embedder/lib.rs index fbb2534e97d..7201b150f8a 100644 --- a/components/shared/embedder/lib.rs +++ b/components/shared/embedder/lib.rs @@ -250,8 +250,6 @@ pub enum EmbedderMsg { OnDevtoolsStarted(Result, String), /// Ask the user to allow a devtools client to connect. RequestDevtoolsConnection(IpcSender), - /// Notify the embedder that it needs to present a new frame. - ReadyToPresent(Vec), /// The given event was delivered to a pipeline in the given browser. EventDelivered(WebViewId, CompositorEventVariant), /// Request to play a haptic effect on a connected gamepad. @@ -314,7 +312,6 @@ impl Debug for EmbedderMsg { EmbedderMsg::OnDevtoolsStarted(..) => write!(f, "OnDevtoolsStarted"), EmbedderMsg::RequestDevtoolsConnection(..) => write!(f, "RequestDevtoolsConnection"), EmbedderMsg::ShowContextMenu(..) => write!(f, "ShowContextMenu"), - EmbedderMsg::ReadyToPresent(..) => write!(f, "ReadyToPresent"), EmbedderMsg::EventDelivered(..) => write!(f, "HitTestedEvent"), EmbedderMsg::PlayGamepadHapticEffect(..) => write!(f, "PlayGamepadHapticEffect"), EmbedderMsg::StopGamepadHapticEffect(..) => write!(f, "StopGamepadHapticEffect"), diff --git a/ports/servoshell/egl/android.rs b/ports/servoshell/egl/android.rs index c3711a4a5b6..3b9721fb1f5 100644 --- a/ports/servoshell/egl/android.rs +++ b/ports/servoshell/egl/android.rs @@ -222,15 +222,6 @@ pub extern "C" fn Java_org_servo_servoview_JNIServo_stop<'local>( call(&mut env, |s| s.stop()); } -#[no_mangle] -pub extern "C" fn Java_org_servo_servoview_JNIServo_refresh<'local>( - mut env: JNIEnv<'local>, - _class: JClass<'local>, -) { - debug!("refresh"); - call(&mut env, |s| s.refresh()); -} - #[no_mangle] pub extern "C" fn Java_org_servo_servoview_JNIServo_goBack<'local>( mut env: JNIEnv<'local>, diff --git a/ports/servoshell/egl/app_state.rs b/ports/servoshell/egl/app_state.rs index 7cec82951a5..0d426ddad20 100644 --- a/ports/servoshell/egl/app_state.rs +++ b/ports/servoshell/egl/app_state.rs @@ -401,13 +401,6 @@ impl RunningAppState { self.perform_updates(); } - /// Redraw the page. - pub fn refresh(&self) { - info!("refresh"); - self.active_webview().composite(); - self.perform_updates(); - } - /// Stop loading the page. pub fn stop(&self) { warn!("TODO can't stop won't stop"); diff --git a/support/android/apk/servoview/src/main/java/org/servo/servoview/JNIServo.java b/support/android/apk/servoview/src/main/java/org/servo/servoview/JNIServo.java index 1acfb0414b5..2ce46fde2fc 100644 --- a/support/android/apk/servoview/src/main/java/org/servo/servoview/JNIServo.java +++ b/support/android/apk/servoview/src/main/java/org/servo/servoview/JNIServo.java @@ -35,8 +35,6 @@ public class JNIServo { public native void stop(); - public native void refresh(); - public native void goBack(); public native void goForward(); diff --git a/support/android/apk/servoview/src/main/java/org/servo/servoview/Servo.java b/support/android/apk/servoview/src/main/java/org/servo/servoview/Servo.java index dfdf81ddc9b..9687278c0bf 100644 --- a/support/android/apk/servoview/src/main/java/org/servo/servoview/Servo.java +++ b/support/android/apk/servoview/src/main/java/org/servo/servoview/Servo.java @@ -87,10 +87,6 @@ public class Servo { mRunCallback.inGLThread(() -> mJNI.resize(coords)); } - public void refresh() { - mRunCallback.inGLThread(() -> mJNI.refresh()); - } - public void reload() { mRunCallback.inGLThread(() -> mJNI.reload()); }