compositor: Improve the way we wait for frames (#31523)

* compositor: Improve the way we wait for frames

In the newest version of WebRender it will be harder to make the
distinction between frame queued for scrolling and other kinds of
pending frames. This change makes it so that we queue frames for both
kinds of changes the same way and keeps a counting of pending frames.
This is conceptually a lot simpler.

In addition, do queue a composite even when recomposite isn't necessary
for a WebRender frame when there are active requestAnimationFrame
callbacks. Doing a composite is what triggers the callbacks to actually
run in the script thread! I believe this was a bug, but the WebRender
upgrade made it much more obvious.

These changes are in preparation for the WebRender upgrade.

* Remove spurious println

components/compositing/compositor.rs

@@ -13,9 +13,8 @@ use std::time::{SystemTime, UNIX_EPOCH};
 
 use canvas::canvas_paint_thread::ImageUpdate;
 use compositing_traits::{
-    CanvasToCompositorMsg, CompositingReason, CompositionPipeline, CompositorMsg,
+    CanvasToCompositorMsg, CompositionPipeline, CompositorMsg, CompositorReceiver,
-    CompositorReceiver, ConstellationMsg, FontToCompositorMsg, ForwardedToCompositorMsg,
+    ConstellationMsg, FontToCompositorMsg, ForwardedToCompositorMsg, SendableFrameTree,
-    SendableFrameTree,
 };
 use crossbeam_channel::Sender;
 use embedder_traits::Cursor;
@@ -182,9 +181,6 @@ pub struct IOCompositor<Window: WindowMethods + ?Sized> {
     /// Pending scroll/zoom events.
     pending_scroll_zoom_events: Vec<ScrollZoomEvent>,
 
-    /// Whether we're waiting on a recomposite after dispatching a scroll.
-    waiting_for_results_of_scroll: bool,
-
     /// Used by the logic that determines when it is safe to output an
     /// image for the reftest framework.
     ready_to_save_state: ReadyState,
@@ -244,10 +240,8 @@ pub struct IOCompositor<Window: WindowMethods + ?Sized> {
     /// True to translate mouse input into touch events.
     convert_mouse_to_touch: bool,
 
-    /// True if a WR frame render has been requested. Screenshots
+    /// The number of frames pending to receive from WebRender.
-    /// taken before the render is complete will not reflect the
+    pending_frames: usize,
-    /// most up to date rendering.
-    waiting_on_pending_frame: bool,
 
     /// Waiting for external code to call present.
     waiting_on_present: bool,
@@ -272,6 +266,23 @@ 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,
+}
+
 #[derive(Debug, PartialEq)]
 enum CompositionRequest {
     NoCompositingNecessary,
@@ -387,7 +398,6 @@ impl<Window: WindowMethods + ?Sized> IOCompositor<Window> {
             composition_request: CompositionRequest::NoCompositingNecessary,
             touch_handler: TouchHandler::new(),
             pending_scroll_zoom_events: Vec::new(),
-            waiting_for_results_of_scroll: false,
             composite_target,
             shutdown_state: ShutdownState::NotShuttingDown,
             page_zoom: Scale::new(1.0),
@@ -415,7 +425,7 @@ impl<Window: WindowMethods + ?Sized> IOCompositor<Window> {
             is_running_problem_test,
             exit_after_load,
             convert_mouse_to_touch,
-            waiting_on_pending_frame: false,
+            pending_frames: 0,
             waiting_on_present: false,
         }
     }
@@ -529,6 +539,10 @@ impl<Window: WindowMethods + ?Sized> IOCompositor<Window> {
     }
 
     fn handle_browser_message(&mut self, msg: CompositorMsg) -> bool {
+        if matches!(msg, CompositorMsg::NewWebRenderFrameReady(..)) {
+            self.pending_frames -= 1;
+        }
+
         match (msg, self.shutdown_state) {
             (_, ShutdownState::FinishedShuttingDown) => {
                 error!("compositor shouldn't be handling messages after shutting down");
@@ -552,11 +566,6 @@ impl<Window: WindowMethods + ?Sized> IOCompositor<Window> {
                 self.send_scroll_positions_to_layout_for_pipeline(&frame_tree.pipeline.id);
             },
 
-            (CompositorMsg::Recomposite(reason), ShutdownState::NotShuttingDown) => {
-                self.waiting_on_pending_frame = false;
-                self.composition_request = CompositionRequest::CompositeNow(reason)
-            },
-
             (CompositorMsg::TouchEventProcessed(result), ShutdownState::NotShuttingDown) => {
                 self.touch_handler.on_event_processed(result);
             },
@@ -577,8 +586,7 @@ impl<Window: WindowMethods + ?Sized> IOCompositor<Window> {
                     self.ready_to_save_state,
                     ReadyState::WaitingForConstellationReply
                 );
-                if is_ready && !self.waiting_on_pending_frame && !self.waiting_for_results_of_scroll
+                if is_ready && self.pending_frames == 0 {
-                {
                     self.ready_to_save_state = ReadyState::ReadyToSaveImage;
                     if self.is_running_problem_test {
                         println!("ready to save image!");
@@ -607,17 +615,19 @@ impl<Window: WindowMethods + ?Sized> IOCompositor<Window> {
             },
 
             (
-                CompositorMsg::NewScrollFrameReady(recomposite_needed),
+                CompositorMsg::NewWebRenderFrameReady(recomposite_needed),
                 ShutdownState::NotShuttingDown,
             ) => {
-                self.waiting_for_results_of_scroll = false;
+                if recomposite_needed {
                     if let Some(result) = self.hit_test_at_device_point(self.cursor_pos) {
                         self.update_cursor(result);
                     }
-                if recomposite_needed {
+                    self.composition_request =
-                    self.composition_request = CompositionRequest::CompositeNow(
+                        CompositionRequest::CompositeNow(CompositingReason::NewWebRenderFrame);
-                        CompositingReason::NewWebRenderScrollFrame,
+                }
-                    );
+
+                if recomposite_needed || self.animation_callbacks_active() {
+                    self.composite_if_necessary(CompositingReason::NewWebRenderFrame)
                 }
             },
 
@@ -700,7 +710,6 @@ impl<Window: WindowMethods + ?Sized> IOCompositor<Window> {
             ForwardedToCompositorMsg::Layout(
                 script_traits::ScriptToCompositorMsg::SendInitialTransaction(pipeline),
             ) => {
-                self.waiting_on_pending_frame = true;
                 let mut txn = Transaction::new();
                 txn.set_display_list(
                     WebRenderEpoch(0),
@@ -710,6 +719,7 @@ impl<Window: WindowMethods + ?Sized> IOCompositor<Window> {
                     false,
                 );
 
+                self.generate_frame(&mut txn);
                 self.webrender_api
                     .send_transaction(self.webrender_document, txn);
             },
@@ -717,11 +727,9 @@ impl<Window: WindowMethods + ?Sized> IOCompositor<Window> {
             ForwardedToCompositorMsg::Layout(
                 script_traits::ScriptToCompositorMsg::SendScrollNode(point, scroll_id),
             ) => {
-                self.waiting_for_results_of_scroll = true;
-
                 let mut txn = Transaction::new();
                 txn.scroll_node_with_id(point, scroll_id, ScrollClamping::NoClamping);
-                txn.generate_frame(0);
+                self.generate_frame(&mut txn);
                 self.webrender_api
                     .send_transaction(self.webrender_document, txn);
             },
@@ -738,8 +746,6 @@ impl<Window: WindowMethods + ?Sized> IOCompositor<Window> {
                     _ => return warn!("Could not recieve WebRender display list."),
                 };
 
-                self.waiting_on_pending_frame = true;
-
                 let pipeline_id = display_list_info.pipeline_id;
                 let details = self.pipeline_details(PipelineId::from_webrender(pipeline_id));
                 details.most_recent_display_list_epoch = Some(display_list_info.epoch);
@@ -757,7 +763,7 @@ impl<Window: WindowMethods + ?Sized> IOCompositor<Window> {
                     ),
                     true,
                 );
-                txn.generate_frame(0);
+                self.generate_frame(&mut txn);
                 self.webrender_api
                     .send_transaction(self.webrender_document, txn);
             },
@@ -879,6 +885,12 @@ impl<Window: WindowMethods + ?Sized> IOCompositor<Window> {
         }
     }
 
+    /// Queue a new frame in the transaction and increase the pending frames count.
+    fn generate_frame(&mut self, transaction: &mut Transaction) {
+        self.pending_frames += 1;
+        transaction.generate_frame(0);
+    }
+
     /// Sets or unsets the animations-running flag for the given pipeline, and schedules a
     /// recomposite if necessary.
     fn change_running_animations_state(
@@ -1011,7 +1023,7 @@ impl<Window: WindowMethods + ?Sized> IOCompositor<Window> {
 
         let mut txn = Transaction::new();
         self.set_root_content_pipeline_handling_pinch_zoom(&mut txn);
-        txn.generate_frame(0);
+        self.generate_frame(&mut txn);
         self.webrender_api
             .send_transaction(self.webrender_document, txn);
 
@@ -1449,10 +1461,9 @@ impl<Window: WindowMethods + ?Sized> IOCompositor<Window> {
             let scroll_origin = LayoutPoint::new(-offset.x, -offset.y);
             transaction.scroll_node_with_id(scroll_origin, external_id, ScrollClamping::NoClamping);
             self.send_scroll_positions_to_layout_for_pipeline(&pipeline_id);
-            self.waiting_for_results_of_scroll = true
         }
 
-        transaction.generate_frame(0);
+        self.generate_frame(&mut transaction);
         self.webrender_api
             .send_transaction(self.webrender_document, transaction);
     }
@@ -1630,6 +1641,13 @@ impl<Window: WindowMethods + ?Sized> IOCompositor<Window> {
         false
     }
 
+    /// Returns true if any animation callbacks (ie `requestAnimationFrame`) are waiting for a response.
+    fn animation_callbacks_active(&self) -> bool {
+        self.pipeline_details
+            .values()
+            .any(|details| details.animation_callbacks_running)
+    }
+
     /// Query the constellation to see if the current compositor
     /// output matches the current frame tree output, and if the
     /// associated script threads are idle.
@@ -1930,7 +1948,6 @@ impl<Window: WindowMethods + ?Sized> IOCompositor<Window> {
         self.composition_request = CompositionRequest::NoCompositingNecessary;
 
         self.process_animations();
-        self.waiting_for_results_of_scroll = false;
 
         Ok(rv)
     }
@@ -2013,8 +2030,12 @@ impl<Window: WindowMethods + ?Sized> IOCompositor<Window> {
         let mut found_recomposite_msg = false;
         while let Some(msg) = self.port.try_recv_compositor_msg() {
             match msg {
-                CompositorMsg::Recomposite(_) if found_recomposite_msg => {},
+                CompositorMsg::NewWebRenderFrameReady(_) if found_recomposite_msg => {
-                CompositorMsg::Recomposite(_) => {
+                    // Only take one of duplicate NewWebRendeFrameReady messages, but do subtract
+                    // one frame from the pending frames.
+                    self.pending_frames -= 1;
+                },
+                CompositorMsg::NewWebRenderFrameReady(_) => {
                     found_recomposite_msg = true;
                     compositor_messages.push(msg)
                 },
@@ -2054,7 +2075,7 @@ impl<Window: WindowMethods + ?Sized> IOCompositor<Window> {
         // The WebXR thread may make a different context current
         let _ = self.rendering_context.make_gl_context_current();
 
-        if !self.pending_scroll_zoom_events.is_empty() && !self.waiting_for_results_of_scroll {
+        if !self.pending_scroll_zoom_events.is_empty() {
             self.process_pending_scroll_events()
         }
         self.shutdown_state != ShutdownState::FinishedShuttingDown
@@ -2068,7 +2089,7 @@ impl<Window: WindowMethods + ?Sized> IOCompositor<Window> {
         while self.shutdown_state != ShutdownState::ShuttingDown {
             let msg = self.port.recv_compositor_msg();
             let need_recomposite = match msg {
-                CompositorMsg::Recomposite(_) => true,
+                CompositorMsg::NewWebRenderFrameReady(_) => true,
                 _ => false,
             };
             let keep_going = self.handle_browser_message(msg);
@@ -2113,7 +2134,7 @@ impl<Window: WindowMethods + ?Sized> IOCompositor<Window> {
         self.webrender.set_debug_flags(flags);
 
         let mut txn = Transaction::new();
-        txn.generate_frame(0);
+        self.generate_frame(&mut txn);
         self.webrender_api
             .send_transaction(self.webrender_document, txn);
     }

components/servo/lib.rs

@@ -33,8 +33,8 @@ use canvas_traits::webgl::WebGLThreads;
 use compositing::windowing::{EmbedderEvent, EmbedderMethods, WindowMethods};
 use compositing::{CompositeTarget, IOCompositor, InitialCompositorState, ShutdownState};
 use compositing_traits::{
-    CanvasToCompositorMsg, CompositingReason, CompositorMsg, CompositorProxy, CompositorReceiver,
+    CanvasToCompositorMsg, CompositorMsg, CompositorProxy, CompositorReceiver, ConstellationMsg,
-    ConstellationMsg, FontToCompositorMsg, ForwardedToCompositorMsg,
+    FontToCompositorMsg, ForwardedToCompositorMsg,
 };
 #[cfg(all(
     not(target_os = "windows"),
@@ -198,26 +198,17 @@ impl webrender_api::RenderNotifier for RenderNotifier {
         Box::new(RenderNotifier::new(self.compositor_proxy.clone()))
     }
 
-    fn wake_up(&self, composite_needed: bool) {
+    fn wake_up(&self, _composite_needed: bool) {}
-        if composite_needed {
-            self.compositor_proxy
-                .recomposite(CompositingReason::NewWebRenderFrame);
-        }
-    }
 
     fn new_frame_ready(
         &self,
         _document_id: DocumentId,
-        scrolled: bool,
+        _scrolled: bool,
         composite_needed: bool,
         _render_time_ns: Option<u64>,
     ) {
-        if scrolled {
         self.compositor_proxy
-                .send(CompositorMsg::NewScrollFrameReady(composite_needed));
+            .send(CompositorMsg::NewWebRenderFrameReady(composite_needed));
-        } else {
-            self.wake_up(true);
-        }
     }
 }
 

components/shared/compositing/lib.rs

@@ -27,33 +27,6 @@ use style_traits::CSSPixel;
 use webrender_api::units::{DeviceIntPoint, DeviceIntSize};
 use webrender_api::{self, FontInstanceKey, FontKey, ImageKey};
 
-/// Why we performed a composite. This is used for debugging.
-#[derive(Clone, Copy, Debug, PartialEq)]
-pub enum CompositingReason {
-    /// We hit the delayed composition timeout. (See `delayed_composition.rs`.)
-    DelayedCompositeTimeout,
-    /// The window has been scrolled and we're starting the first recomposite.
-    Scroll,
-    /// A scroll has continued and we need to recomposite again.
-    ContinueScroll,
-    /// We're performing the single composite in headless mode.
-    Headless,
-    /// We're performing a composite to run an animation.
-    Animation,
-    /// A new frame tree has been loaded.
-    NewFrameTree,
-    /// New painted buffers have been received.
-    NewPaintedBuffers,
-    /// The window has been zoomed.
-    Zoom,
-    /// A new WebRender frame has arrived.
-    NewWebRenderFrame,
-    /// WebRender has processed a scroll event and has generated a new frame.
-    NewWebRenderScrollFrame,
-    /// The window has been resized and will need to be synchronously repainted.
-    Resize,
-}
-
 /// Sends messages to the compositor.
 pub struct CompositorProxy {
     pub sender: Sender<CompositorMsg>,
@@ -92,12 +65,6 @@ impl CompositorReceiver {
     }
 }
 
-impl CompositorProxy {
-    pub fn recomposite(&self, reason: CompositingReason) {
-        self.send(CompositorMsg::Recomposite(reason));
-    }
-}
-
 /// Messages from (or via) the constellation thread to the compositor.
 pub enum CompositorMsg {
     /// Informs the compositor that the constellation has completed shutdown.
@@ -108,8 +75,6 @@ pub enum CompositorMsg {
     ChangeRunningAnimationsState(PipelineId, AnimationState),
     /// Replaces the current frame tree, typically called during main frame navigation.
     SetFrameTree(SendableFrameTree),
-    /// Composite.
-    Recomposite(CompositingReason),
     /// Script has handled a touch event, and either prevented or allowed default actions.
     TouchEventProcessed(EventResult),
     /// Composite to a PNG file and return the Image over a passed channel.
@@ -118,9 +83,9 @@ pub enum CompositorMsg {
     IsReadyToSaveImageReply(bool),
     /// Pipeline visibility changed
     PipelineVisibilityChanged(PipelineId, bool),
-    /// WebRender has successfully processed a scroll. The boolean specifies whether a composite is
+    /// WebRender has produced a new frame. This message informs the compositor that
-    /// needed.
+    /// the frame is ready, so that it may trigger a recomposite.
-    NewScrollFrameReady(bool),
+    NewWebRenderFrameReady(bool /* composite_needed */),
     /// A pipeline was shut down.
     // This message acts as a synchronization point between the constellation,
     // when it shuts down a pipeline, to the compositor; when the compositor
@@ -193,13 +158,12 @@ impl Debug for CompositorMsg {
                 write!(f, "ChangeRunningAnimationsState({:?})", state)
             },
             CompositorMsg::SetFrameTree(..) => write!(f, "SetFrameTree"),
-            CompositorMsg::Recomposite(..) => write!(f, "Recomposite"),
             CompositorMsg::TouchEventProcessed(..) => write!(f, "TouchEventProcessed"),
             CompositorMsg::CreatePng(..) => write!(f, "CreatePng"),
             CompositorMsg::IsReadyToSaveImageReply(..) => write!(f, "IsReadyToSaveImageReply"),
             CompositorMsg::PipelineVisibilityChanged(..) => write!(f, "PipelineVisibilityChanged"),
             CompositorMsg::PipelineExited(..) => write!(f, "PipelineExited"),
-            CompositorMsg::NewScrollFrameReady(..) => write!(f, "NewScrollFrameReady"),
+            CompositorMsg::NewWebRenderFrameReady(..) => write!(f, "NewWebRenderFrameReady"),
             CompositorMsg::Dispatch(..) => write!(f, "Dispatch"),
             CompositorMsg::PendingPaintMetric(..) => write!(f, "PendingPaintMetric"),
             CompositorMsg::LoadComplete(..) => write!(f, "LoadComplete"),