script: Integrate animated image updates into ScriptThread event loop (#38941)

Instead of manually triggering `ScriptThread::update_the_rendering`,
have animated images trigger rendering updates via the `ScriptThread`
event loop. This should result in fewer calls to
`ScriptThread::update_the_rendering`.

Testing: This should not change behavior and is thus covered by existing
tests.

Signed-off-by: Martin Robinson <mrobinson@igalia.com>
Co-authored-by: Mukilan Thiyagarajan <mukilan@igalia.com>

components/script/animations.rs

@@ -76,10 +76,6 @@ impl Animations {
         self.pending_events.borrow_mut().clear();
     }
 
-    pub(crate) fn animations_present(&self) -> bool {
-        self.has_running_animations.get() || !self.pending_events.borrow().is_empty()
-    }
-
     // Mark all animations dirty, if they haven't been marked dirty since the
     // specified `current_timeline_value`. Returns true if animations were marked
     // dirty or false otherwise.

components/script/dom/document.rs

@@ -202,7 +202,7 @@ use crate::mime::{APPLICATION, CHARSET, MimeExt};
 use crate::network_listener::{NetworkListener, PreInvoke};
 use crate::realms::{AlreadyInRealm, InRealm, enter_realm};
 use crate::script_runtime::{CanGc, ScriptThreadEventCategory};
-use crate::script_thread::{ScriptThread, with_script_thread};
+use crate::script_thread::ScriptThread;
 use crate::stylesheet_set::StylesheetSetRef;
 use crate::task::NonSendTaskBox;
 use crate::task_source::TaskSourceName;
@@ -485,6 +485,8 @@ pub(crate) struct Document {
     /// List of all WebGL context IDs that need flushing.
     dirty_webgl_contexts:
         DomRefCell<HashMapTracedValues<WebGLContextId, Dom<WebGLRenderingContext>>>,
+    /// Whether or not animated images need to have their contents updated.
+    has_pending_animated_image_update: Cell<bool>,
     /// List of all WebGPU contexts.
     #[cfg(feature = "webgpu")]
     #[ignore_malloc_size_of = "Rc are hard"]
@@ -2646,6 +2648,9 @@ impl Document {
         if self.window().has_unhandled_resize_event() {
             return true;
         }
+        if self.has_pending_animated_image_update.get() {
+            return true;
+        }
 
         false
     }
@@ -2658,7 +2663,12 @@ impl Document {
     //
     // Returns the set of reflow phases run as a [`ReflowPhasesRun`].
     pub(crate) fn update_the_rendering(&self) -> ReflowPhasesRun {
-        self.update_animating_images();
+        if self.has_pending_animated_image_update.get() {
+            self.image_animation_manager
+                .borrow()
+                .update_active_frames(&self.window, self.current_animation_timeline_value());
+            self.has_pending_animated_image_update.set(false);
+        }
 
         // All dirty canvases are flushed before updating the rendering.
         #[cfg(feature = "webgpu")]
@@ -3352,6 +3362,7 @@ impl Document {
             media_controls: DomRefCell::new(HashMap::new()),
             dirty_2d_contexts: DomRefCell::new(HashMapTracedValues::new()),
             dirty_webgl_contexts: DomRefCell::new(HashMapTracedValues::new()),
+            has_pending_animated_image_update: Cell::new(false),
             #[cfg(feature = "webgpu")]
             webgpu_contexts: Rc::new(RefCell::new(HashMapTracedValues::new())),
             selection: MutNullableDom::new(None),
@@ -4144,7 +4155,9 @@ impl Document {
         self.animations
             .borrow()
             .do_post_reflow_update(&self.window, self.current_animation_timeline_value());
-        self.image_animation_manager().update_rooted_dom_nodes();
+        self.image_animation_manager
+            .borrow()
+            .update_rooted_dom_nodes(&self.window, self.current_animation_timeline_value());
     }
 
     pub(crate) fn cancel_animations_for_node(&self, node: &Node) {
@@ -4184,33 +4197,8 @@ impl Document {
         self.image_animation_manager.borrow()
     }
 
-    pub(crate) fn update_animating_images(&self) {
+    pub(crate) fn set_has_pending_animated_image_update(&self) {
-        let image_animation_manager = self.image_animation_manager.borrow();
+        self.has_pending_animated_image_update.set(true);
-        if !image_animation_manager.image_animations_present() {
-            return;
-        }
-        image_animation_manager
-            .update_active_frames(&self.window, self.current_animation_timeline_value());
-
-        if !self.animations().animations_present() {
-            let next_scheduled_time = image_animation_manager
-                .next_scheduled_time(self.current_animation_timeline_value());
-            // TODO: Once we have refresh signal from the compositor,
-            // we should get rid of timer for animated image update.
-            if let Some(next_scheduled_time) = next_scheduled_time {
-                self.schedule_image_animation_update(next_scheduled_time);
-            }
-        }
-    }
-
-    fn schedule_image_animation_update(&self, next_scheduled_time: f64) {
-        let callback = ImageAnimationUpdateCallback {
-            document: Trusted::new(self),
-        };
-        self.global().schedule_callback(
-            OneshotTimerCallback::ImageAnimationUpdate(callback),
-            Duration::from_secs_f64(next_scheduled_time),
-        );
     }
 
     /// <https://html.spec.whatwg.org/multipage/#shared-declarative-refresh-steps>
@@ -5919,24 +5907,6 @@ pub(crate) enum FocusEventType {
     Blur,  // Element lost focus. Doesn't bubble.
 }
 
-/// This is a temporary workaround to update animated images,
-/// we should get rid of this after we have refresh driver #3406
-#[derive(JSTraceable, MallocSizeOf)]
-pub(crate) struct ImageAnimationUpdateCallback {
-    /// The document.
-    #[ignore_malloc_size_of = "non-owning"]
-    document: Trusted<Document>,
-}
-
-impl ImageAnimationUpdateCallback {
-    pub(crate) fn invoke(self, can_gc: CanGc) {
-        with_script_thread(|script_thread| {
-            script_thread.set_needs_rendering_update();
-            script_thread.update_the_rendering(can_gc);
-        })
-    }
-}
-
 #[derive(JSTraceable, MallocSizeOf)]
 pub(crate) enum AnimationFrameCallback {
     DevtoolsFramerateTick {

components/script/image_animation.rs

@@ -2,7 +2,9 @@
  * 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::cell::Cell;
 use std::sync::Arc;
+use std::time::Duration;
 
 use compositing_traits::{ImageUpdate, SerializableImageData};
 use embedder_traits::UntrustedNodeAddress;
@@ -12,17 +14,21 @@ use layout_api::ImageAnimationState;
 use libc::c_void;
 use malloc_size_of::MallocSizeOf;
 use parking_lot::RwLock;
+use script_bindings::codegen::GenericBindings::WindowBinding::WindowMethods;
 use script_bindings::root::Dom;
 use style::dom::OpaqueNode;
+use timers::{TimerEventRequest, TimerId};
 use webrender_api::units::DeviceIntSize;
 use webrender_api::{ImageDescriptor, ImageDescriptorFlags, ImageFormat};
 
 use crate::dom::bindings::cell::DomRefCell;
+use crate::dom::bindings::refcounted::Trusted;
 use crate::dom::bindings::trace::NoTrace;
 use crate::dom::node::{Node, from_untrusted_node_address};
 use crate::dom::window::Window;
+use crate::script_thread::with_script_thread;
 
-#[derive(Clone, Debug, Default, JSTraceable)]
+#[derive(Clone, Default, JSTraceable)]
 #[cfg_attr(crown, crown::unrooted_must_root_lint::must_root)]
 pub struct ImageAnimationManager {
     #[no_trace]
@@ -32,6 +38,10 @@ pub struct ImageAnimationManager {
     ///
     /// TODO(mrobinson): This does not properly handle animating images that are in pseudo-elements.
     rooted_nodes: DomRefCell<FxHashMap<NoTrace<OpaqueNode>, Dom<Node>>>,
+
+    /// The [`TimerId`] of the currently scheduled animated image update callback.
+    #[no_trace]
+    callback_timer_id: Cell<Option<TimerId>>,
 }
 
 impl MallocSizeOf for ImageAnimationManager {
@@ -47,19 +57,19 @@ impl ImageAnimationManager {
         self.node_to_image_map.clone()
     }
 
-    pub(crate) fn next_scheduled_time(&self, now: f64) -> Option<f64> {
+    fn duration_to_next_frame(&self, now: f64) -> Option<Duration> {
         self.node_to_image_map
             .read()
             .values()
-            .map(|state| state.time_to_next_frame(now))
+            .map(|state| state.duration_to_next_frame(now))
-            .min_by(|a, b| a.partial_cmp(b).unwrap_or(std::cmp::Ordering::Equal))
+            .min()
-    }
-
-    pub(crate) fn image_animations_present(&self) -> bool {
-        !self.node_to_image_map.read().is_empty()
     }
 
     pub(crate) fn update_active_frames(&self, window: &Window, now: f64) {
+        if self.node_to_image_map.read().is_empty() {
+            return;
+        }
+
         let rooted_nodes = self.rooted_nodes.borrow();
         let updates = self
             .node_to_image_map
@@ -95,20 +105,26 @@ impl ImageAnimationManager {
             })
             .collect();
         window.compositor_api().update_images(updates);
+
+        self.maybe_schedule_animated_image_update_callback(window, now);
     }
+
     /// Ensure that all nodes with animating images are rooted and unroots any nodes that
     /// no longer have an animating image. This should be called immediately after a
     /// restyle, to ensure that these addresses are still valid.
     #[allow(unsafe_code)]
-    pub(crate) fn update_rooted_dom_nodes(&self) {
+    pub(crate) fn update_rooted_dom_nodes(&self, window: &Window, now: f64) {
         let mut rooted_nodes = self.rooted_nodes.borrow_mut();
         let node_to_image_map = self.node_to_image_map.read();
 
+        let mut added_node = false;
         for opaque_node in node_to_image_map.keys() {
             let opaque_node = *opaque_node;
             if rooted_nodes.contains_key(&NoTrace(opaque_node)) {
                 continue;
             }
+
+            added_node = true;
             let address = UntrustedNodeAddress(opaque_node.0 as *const c_void);
             unsafe {
                 rooted_nodes.insert(
@@ -118,6 +134,32 @@ impl ImageAnimationManager {
             };
         }
 
+        let length_before = rooted_nodes.len();
         rooted_nodes.retain(|node, _| node_to_image_map.contains_key(&node.0));
+
+        if added_node || length_before != rooted_nodes.len() {
+            self.maybe_schedule_animated_image_update_callback(window, now);
+        }
+    }
+
+    fn maybe_schedule_animated_image_update_callback(&self, window: &Window, now: f64) {
+        with_script_thread(|script_thread| {
+            if let Some(current_timer_id) = self.callback_timer_id.take() {
+                self.callback_timer_id.set(None);
+                script_thread.cancel_timer(current_timer_id);
+            }
+
+            if let Some(duration) = self.duration_to_next_frame(now) {
+                let trusted_window = Trusted::new(window);
+                let timer_id = script_thread.schedule_timer(TimerEventRequest {
+                    callback: Box::new(move || {
+                        let window = trusted_window.root();
+                        window.Document().set_has_pending_animated_image_update();
+                    }),
+                    duration,
+                });
+                self.callback_timer_id.set(Some(timer_id));
+            }
+        })
     }
 }

components/script/script_thread.rs

@@ -565,6 +565,12 @@ impl ScriptThread {
         self.timer_scheduler.borrow_mut().schedule_timer(request)
     }
 
+    /// Cancel a the [`TimerEventRequest`] for the given [`TimerId`] on this
+    /// [`ScriptThread`]'s [`TimerScheduler`].
+    pub(crate) fn cancel_timer(&self, timer_id: TimerId) {
+        self.timer_scheduler.borrow_mut().cancel_timer(timer_id)
+    }
+
     // https://html.spec.whatwg.org/multipage/#await-a-stable-state
     pub(crate) fn await_stable_state(task: Microtask) {
         with_script_thread(|script_thread| {

components/script/timers.rs

@@ -29,7 +29,7 @@ use crate::dom::bindings::reflector::{DomGlobal, DomObject};
 use crate::dom::bindings::root::{AsHandleValue, Dom};
 use crate::dom::bindings::str::DOMString;
 use crate::dom::csp::CspReporting;
-use crate::dom::document::{ImageAnimationUpdateCallback, RefreshRedirectDue};
+use crate::dom::document::RefreshRedirectDue;
 use crate::dom::eventsource::EventSourceTimeoutCallback;
 use crate::dom::globalscope::GlobalScope;
 #[cfg(feature = "testbinding")]
@@ -88,7 +88,6 @@ pub(crate) enum OneshotTimerCallback {
     #[cfg(feature = "testbinding")]
     TestBindingCallback(TestBindingCallback),
     RefreshRedirectDue(RefreshRedirectDue),
-    ImageAnimationUpdate(ImageAnimationUpdateCallback),
 }
 
 impl OneshotTimerCallback {
@@ -100,7 +99,6 @@ impl OneshotTimerCallback {
             #[cfg(feature = "testbinding")]
             OneshotTimerCallback::TestBindingCallback(callback) => callback.invoke(),
             OneshotTimerCallback::RefreshRedirectDue(callback) => callback.invoke(can_gc),
-            OneshotTimerCallback::ImageAnimationUpdate(callback) => callback.invoke(can_gc),
         }
     }
 }

components/shared/layout/lib.rs

@@ -16,6 +16,7 @@ use std::collections::HashMap;
 use std::sync::Arc;
 use std::sync::atomic::{AtomicIsize, AtomicU64, Ordering};
 use std::thread::JoinHandle;
+use std::time::Duration;
 
 use app_units::Au;
 use atomic_refcell::AtomicRefCell;
@@ -578,7 +579,7 @@ pub struct ImageAnimationState {
     #[ignore_malloc_size_of = "Arc is hard"]
     pub image: Arc<RasterImage>,
     pub active_frame: usize,
-    last_update_time: f64,
+    frame_start_time: f64,
 }
 
 impl ImageAnimationState {
@@ -586,7 +587,7 @@ impl ImageAnimationState {
         Self {
             image,
             active_frame: 0,
-            last_update_time,
+            frame_start_time: last_update_time,
         }
     }
 
@@ -594,15 +595,18 @@ impl ImageAnimationState {
         self.image.id
     }
 
-    pub fn time_to_next_frame(&self, now: f64) -> f64 {
+    pub fn duration_to_next_frame(&self, now: f64) -> Duration {
         let frame_delay = self
             .image
             .frames
             .get(self.active_frame)
             .expect("Image frame should always be valid")
             .delay
-            .map_or(0., |delay| delay.as_secs_f64());
+            .unwrap_or_default();
-        (frame_delay - now + self.last_update_time).max(0.0)
+
+        let time_since_frame_start = (now - self.frame_start_time).max(0.0) * 1000.0;
+        let time_since_frame_start = Duration::from_secs_f64(time_since_frame_start);
+        frame_delay - time_since_frame_start.min(frame_delay)
     }
 
     /// check whether image active frame need to be updated given current time,
@@ -613,7 +617,7 @@ impl ImageAnimationState {
             return false;
         }
         let image = &self.image;
-        let time_interval_since_last_update = now - self.last_update_time;
+        let time_interval_since_last_update = now - self.frame_start_time;
         let mut remain_time_interval = time_interval_since_last_update -
             image
                 .frames
@@ -637,7 +641,7 @@ impl ImageAnimationState {
             return false;
         }
         self.active_frame = next_active_frame_id;
-        self.last_update_time = now;
+        self.frame_start_time = now;
         true
     }
 }
@@ -698,18 +702,18 @@ mod test {
         let mut image_animation_state = ImageAnimationState::new(Arc::new(image), 0.0);
 
         assert_eq!(image_animation_state.active_frame, 0);
-        assert_eq!(image_animation_state.last_update_time, 0.0);
+        assert_eq!(image_animation_state.frame_start_time, 0.0);
         assert_eq!(
             image_animation_state.update_frame_for_animation_timeline_value(0.101),
             true
         );
         assert_eq!(image_animation_state.active_frame, 1);
-        assert_eq!(image_animation_state.last_update_time, 0.101);
+        assert_eq!(image_animation_state.frame_start_time, 0.101);
         assert_eq!(
             image_animation_state.update_frame_for_animation_timeline_value(0.116),
             false
         );
         assert_eq!(image_animation_state.active_frame, 1);
-        assert_eq!(image_animation_state.last_update_time, 0.101);
+        assert_eq!(image_animation_state.frame_start_time, 0.101);
     }
 }