Auto merge of #25822 - jdm:delay-reftest-async-render, r=emilio

Delay reftest screenshot while WR frame is rendering

This PR addresses the theory that #24726 occurs when WR is performing an async frame render and the reftest screenshot decides it's time to synchronously read the framebuffer. If there have not been any completed frames rendered yet, that would yield the page background colour.

The changes in this PR introduce an additional layer of synchronization - the compositor stores an AtomicBool value that indicates whether we know that a WR frame has started rendering, which is set to true when an IPC request from layout that submits a new display list is received. This bool is set to false when WR notifies us that a frame has been rendered. The screenshot code refuses to take a screenshot if the bool is true, causing us to delay taking a screenshot until there is no frame pending.

components/compositing/compositor.rs

@@ -39,6 +39,8 @@ use std::fs::{create_dir_all, File};
 use std::io::Write;
 use std::num::NonZeroU32;
 use std::rc::Rc;
+use std::sync::atomic::{AtomicBool, Ordering};
+use std::sync::Arc;
 use style_traits::viewport::ViewportConstraints;
 use style_traits::{CSSPixel, DevicePixel, PinchZoomFactor};
 use time::{now, precise_time_ns, precise_time_s};
@@ -208,6 +210,11 @@ 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
+    /// taken before the render is complete will not reflect the
+    /// most up to date rendering.
+    waiting_on_pending_frame: Arc<AtomicBool>,
 }
 
 #[derive(Clone, Copy)]
@@ -322,6 +329,7 @@ impl<Window: WindowMethods + ?Sized> IOCompositor<Window> {
             is_running_problem_test,
             exit_after_load,
             convert_mouse_to_touch,
+            waiting_on_pending_frame: state.pending_wr_frame,
         }
     }
 
@@ -425,6 +433,7 @@ impl<Window: WindowMethods + ?Sized> IOCompositor<Window> {
             },
 
             (Msg::Recomposite(reason), ShutdownState::NotShuttingDown) => {
+                self.waiting_on_pending_frame.store(false, Ordering::SeqCst);
                 self.composition_request = CompositionRequest::CompositeNow(reason)
             },
 
@@ -455,7 +464,7 @@ impl<Window: WindowMethods + ?Sized> IOCompositor<Window> {
                     self.ready_to_save_state,
                     ReadyState::WaitingForConstellationReply
                 );
-                if is_ready {
+                if is_ready && !self.waiting_on_pending_frame.load(Ordering::SeqCst) {
                     self.ready_to_save_state = ReadyState::ReadyToSaveImage;
                     if self.is_running_problem_test {
                         println!("ready to save image!");

components/compositing/compositor_thread.rs

@@ -17,6 +17,8 @@ use profile_traits::mem;
 use profile_traits::time;
 use script_traits::{AnimationState, EventResult, MouseButton, MouseEventType};
 use std::fmt::{Debug, Error, Formatter};
+use std::sync::atomic::AtomicBool;
+use std::sync::Arc;
 use style_traits::viewport::ViewportConstraints;
 use style_traits::CSSPixel;
 use webrender_api;
@@ -167,4 +169,5 @@ pub struct InitialCompositorState {
     pub webrender_api: webrender_api::RenderApi,
     pub webvr_heartbeats: Vec<Box<dyn WebVRMainThreadHeartbeat>>,
     pub webxr_main_thread: webxr::MainThreadRegistry,
+    pub pending_wr_frame: Arc<AtomicBool>,
 }

components/constellation/constellation.rs

@@ -166,6 +166,7 @@ use std::marker::PhantomData;
 use std::mem::replace;
 use std::process;
 use std::rc::{Rc, Weak};
+use std::sync::atomic::{AtomicBool, Ordering};
 use std::sync::Arc;
 use std::thread;
 use style_traits::viewport::ViewportConstraints;
@@ -546,6 +547,9 @@ pub struct InitialConstellationState {
 
     /// Mechanism to force the compositor to process events.
     pub event_loop_waker: Option<Box<dyn EventLoopWaker>>,
+
+    /// A flag share with the compositor to indicate that a WR frame is in progress.
+    pub pending_wr_frame: Arc<AtomicBool>,
 }
 
 /// Data needed for webdriver
@@ -720,12 +724,17 @@ enum WebrenderMsg {
 
 /// Accept messages from content processes that need to be relayed to the WebRender
 /// instance in the parent process.
-fn handle_webrender_message(webrender_api: &webrender_api::RenderApi, msg: WebrenderMsg) {
+fn handle_webrender_message(
+    pending_wr_frame: &AtomicBool,
+    webrender_api: &webrender_api::RenderApi,
+    msg: WebrenderMsg,
+) {
     match msg {
         WebrenderMsg::Layout(script_traits::WebrenderMsg::SendInitialTransaction(
             doc,
             pipeline,
         )) => {
+            pending_wr_frame.store(true, Ordering::SeqCst);
             let mut txn = webrender_api::Transaction::new();
             txn.set_display_list(
                 webrender_api::Epoch(0),
@@ -757,6 +766,7 @@ fn handle_webrender_message(webrender_api: &webrender_api::RenderApi, msg: Webre
             data,
             descriptor,
         )) => {
+            pending_wr_frame.store(true, Ordering::SeqCst);
             let mut txn = webrender_api::Transaction::new();
             txn.set_display_list(
                 epoch,
@@ -881,10 +891,12 @@ where
                     ipc::channel().expect("ipc channel failure");
 
                 let webrender_api = state.webrender_api_sender.create_api();
+                let pending_wr_frame_clone = state.pending_wr_frame.clone();
                 ROUTER.add_route(
                     webrender_ipc_receiver.to_opaque(),
                     Box::new(move |message| {
                         handle_webrender_message(
+                            &pending_wr_frame_clone,
                             &webrender_api,
                             WebrenderMsg::Layout(message.to().expect("conversion failure")),
                         )
@@ -892,10 +904,12 @@ where
                 );
 
                 let webrender_api = state.webrender_api_sender.create_api();
+                let pending_wr_frame_clone = state.pending_wr_frame.clone();
                 ROUTER.add_route(
                     webrender_image_ipc_receiver.to_opaque(),
                     Box::new(move |message| {
                         handle_webrender_message(
+                            &pending_wr_frame_clone,
                             &webrender_api,
                             WebrenderMsg::Net(message.to().expect("conversion failure")),
                         )

components/servo/lib.rs

@@ -115,6 +115,7 @@ use std::borrow::Cow;
 use std::cmp::max;
 use std::path::PathBuf;
 use std::rc::Rc;
+use std::sync::atomic::AtomicBool;
 use std::sync::{Arc, Mutex};
 #[cfg(not(target_os = "windows"))]
 use surfman::platform::default::device::Device as HWDevice;
@@ -505,6 +506,8 @@ where
             device_pixel_ratio: Scale::new(device_pixel_ratio),
         };
 
+        let pending_wr_frame = Arc::new(AtomicBool::new(false));
+
         // Create the constellation, which maintains the engine
         // pipelines, including the script and layout threads, as well
         // as the navigation context.
@@ -527,6 +530,7 @@ where
             glplayer_threads,
             event_loop_waker,
             window_size,
+            pending_wr_frame.clone(),
         );
 
         // Send the constellation's swmanager sender to service worker manager thread
@@ -553,6 +557,7 @@ where
                 webrender_api,
                 webvr_heartbeats,
                 webxr_main_thread,
+                pending_wr_frame,
             },
             opts.output_file.clone(),
             opts.is_running_problem_test,
@@ -865,6 +870,7 @@ fn create_constellation(
     glplayer_threads: Option<GLPlayerThreads>,
     event_loop_waker: Option<Box<dyn EventLoopWaker>>,
     initial_window_size: WindowSizeData,
+    pending_wr_frame: Arc<AtomicBool>,
 ) -> (Sender<ConstellationMsg>, SWManagerSenders) {
     // Global configuration options, parsed from the command line.
     let opts = opts::get();
@@ -907,6 +913,7 @@ fn create_constellation(
         glplayer_threads,
         player_context,
         event_loop_waker,
+        pending_wr_frame,
     };
 
     let (canvas_chan, ipc_canvas_chan) = canvas::canvas_paint_thread::CanvasPaintThread::start();