Merge pull request #1092 from vangel76/fix-1083-beat-marker-audio-pause

Markers: adding one during playback no longer stops the audio (#1083, #1085)

crates/engine/src/tests/disk_cache.rs

@@ -136,3 +136,62 @@ fn footage_changes_change_the_salt() {
     assert_ne!(k3, k2);
     assert_ne!(set_proxy(&mut s, false), k3);
 }
+
+/// Marker edits keep the disk key (the sound of a playing preview must not wait for frames to
+/// render again, #1083); an expression that reads markers makes them count.
+#[test]
+fn markers_keep_the_content_key_unless_an_expression_reads_them() {
+    let mut s = Session::default();
+    s.execute("comp.new", json!({"name": "M", "width": 8, "height": 8, "frameRate": 30, "duration": 2})).unwrap();
+    let cid = s.active_comp_id().unwrap();
+    let l = s.execute("layer.newSolid", json!({"color": "#ff0000"})).unwrap()["layer"].as_u64().unwrap();
+    let key = |s: &Session| effectcraft_render::disk_cache::comp_content_key(&s.project, cid);
+    let k0 = key(&s);
+    s.execute("comp.addMarker", json!({"time": 0.5, "comment": "beat"})).unwrap();
+    s.execute("markers.set", json!({"layer": l, "new": true, "time": 0.25})).unwrap();
+    assert_eq!(key(&s), k0, "markers");
+    s.execute("prop.setExpression", json!({"layer": l, "path": "transform/opacity", "expression": "thisComp.marker.numKeys"})).unwrap();
+    let k1 = key(&s);
+    assert_ne!(k1, k0);
+    s.execute("comp.addMarker", json!({"time": 1.0})).unwrap();
+    assert_ne!(key(&s), k1, "a marker read by an expression");
+}
+
+/// A comp marker alone (no layer marker beside it) keeps the key too.
+#[test]
+fn a_lone_comp_marker_keeps_the_content_key() {
+    let mut s = Session::default();
+    s.execute("comp.new", json!({"name": "M", "width": 8, "height": 8, "frameRate": 30, "duration": 2})).unwrap();
+    let cid = s.active_comp_id().unwrap();
+    s.execute("layer.newSolid", json!({"color": "#ff0000"})).unwrap();
+    let key = |s: &Session| effectcraft_render::disk_cache::comp_content_key(&s.project, cid);
+    let k0 = key(&s);
+    s.execute("comp.addMarker", json!({"time": 0.5, "comment": "beat"})).unwrap();
+    assert_eq!(key(&s), k0);
+}
+
+/// A protected marker with a duration retimes a stretched precomp layer (Responsive Design), so
+/// changing it changes the key of the comp that nests it; a plain marker beside it does not.
+#[test]
+fn protected_markers_with_a_duration_change_the_content_key() {
+    let mut s = Session::default();
+    s.execute("comp.new", json!({"name": "Inner", "width": 8, "height": 8, "frameRate": 30, "duration": 4})).unwrap();
+    let inner = s.active_comp_id().unwrap();
+    s.execute("layer.newSolid", json!({"color": "#ff0000"})).unwrap();
+    s.execute("comp.new", json!({"name": "Outer", "width": 8, "height": 8, "frameRate": 30, "duration": 10})).unwrap();
+    let outer = s.active_comp_id().unwrap();
+    let l = s.execute("layer.addItem", json!({"item": inner.0})).unwrap()["layer"].as_u64().unwrap();
+    s.execute("layer.select", json!({"layers": [l]})).unwrap();
+    s.execute("layer.timeStretch", json!({"percent": 200})).unwrap();
+    let key = |s: &Session| effectcraft_render::disk_cache::comp_content_key(&s.project, outer);
+    let k0 = key(&s);
+    s.execute("comp.addMarker", json!({"comp": inner.0, "time": 2.0, "comment": "beat"})).unwrap();
+    assert_eq!(key(&s), k0, "a plain marker");
+    s.execute("comp.addMarker", json!({"comp": inner.0, "time": 1.0})).unwrap();
+    let index = s.project.comp(inner).unwrap().markers.iter().position(|m| m.time.seconds() == 1.0).unwrap();
+    s.execute("markers.set", json!({"comp": inner.0, "index": index, "duration": 1.0, "protected": true})).unwrap();
+    let k1 = key(&s);
+    assert_ne!(k1, k0, "a protected marker with a duration");
+    s.execute("markers.set", json!({"comp": inner.0, "index": index, "duration": 2.0})).unwrap();
+    assert_ne!(key(&s), k1, "its duration");
+}

crates/project/src/comp.rs

@@ -275,15 +275,35 @@ impl Comp {
     }
 }
 
+/// A protected time marker with a duration sets the comp's timing (Responsive Design), so it is
+/// part of the pixels even where plain markers are not.
+fn is_timing_marker(m: &Marker) -> bool {
+    m.protected && m.duration > Tick::ZERO
+}
+
+fn timing_markers(m: &[Marker]) -> impl Iterator<Item = (Tick, Tick)> + '_ {
+    m.iter().filter(|m| is_timing_marker(m)).map(|m| (m.time, m.duration))
+}
+
 impl Comp {
     /// Draws the same pixels as `o`: equal but for its layers' Audio, Lock and Shy switches, Hide
     /// Shy Layers and the viewer's Resolution.
     pub fn same_pixels(&self, o: &Comp) -> bool {
-        self.same_settings(o) && self.layers.len() == o.layers.len() && self.layers.iter().zip(&o.layers).all(|(a, b)| a.same_pixels(b))
+        self.same_pixels_with(o, true)
     }
 
-    /// [`Self::same_pixels`] but for the layers.
-    fn same_settings(&self, o: &Comp) -> bool {
+    /// [`Self::same_pixels`], with the comp's and its layers' markers counting only when
+    /// `with_markers`: only expressions read them, so while none does, adding or moving a marker
+    /// (even during playback) leaves every rendered frame valid. Protected time markers with a
+    /// duration set the timing (Responsive Design), so those always count.
+    pub fn same_pixels_with(&self, o: &Comp, with_markers: bool) -> bool {
+        self.same_settings(o, with_markers)
+            && self.layers.len() == o.layers.len()
+            && self.layers.iter().zip(&o.layers).all(|(a, b)| a.same_pixels_with(b, with_markers))
+    }
+
+    /// [`Self::same_pixels_with`] but for the layers.
+    fn same_settings(&self, o: &Comp, with_markers: bool) -> bool {
         // Every field (no `..`: a new field must be considered here).
         let Comp {
             width,
@@ -321,7 +341,7 @@ impl Comp {
             && *display_start == o.display_start
             && *background == o.background
             && *work_area == o.work_area
-            && *markers == o.markers
+            && (if with_markers { *markers == o.markers } else { timing_markers(markers).eq(timing_markers(&o.markers)) })
             && *shutter_angle == o.shutter_angle
             && *shutter_phase == o.shutter_phase
             && *motion_blur_samples == o.motion_blur_samples
@@ -356,7 +376,7 @@ impl Comp {
             })
         }
         // Solo hides every other layer at every frame.
-        if !self.same_settings(o) || self.any_solo() != o.any_solo() {
+        if !self.same_settings(o, true) || self.any_solo() != o.any_solo() {
             return None;
         }
         let common = |a: &Comp, b: &Comp| a.layers.iter().map(|l| l.id).filter(|id| b.layer(*id).is_some()).collect::<Vec<_>>();
@@ -393,34 +413,33 @@ impl Comp {
         self.layers.iter().any(|l| l.switches.solo && l.source.is_av())
     }
 
-    /// Draws the same pixels as `o` once plain markers are ignored: they change no pixels. Protected
-    /// time markers with a duration set the timing (Responsive Design), so those still count.
-    pub fn same_pixels_ignoring_markers(&self, o: &Comp) -> bool {
-        let protected =
-            |m: &[Marker]| -> Vec<(Tick, Tick)> { m.iter().filter(|m| m.protected && m.duration > Tick::ZERO).map(|m| (m.time, m.duration)).collect() };
-        if protected(&self.markers) != protected(&o.markers) {
-            return false;
-        }
-        let mut c = self.clone();
-        c.markers = o.markers.clone();
-        for (a, b) in c.layers.iter_mut().zip(&o.layers) {
-            a.markers = b.markers.clone();
-            a.markers_locked = b.markers_locked;
-        }
-        c.same_pixels(o)
-    }
-
     /// The comp with those switches and the Resolution at their defaults (borrowed when they
     /// already are), for keys that hash the whole comp.
     pub fn pixel_form(&self) -> std::borrow::Cow<'_, Comp> {
-        if !self.hide_shy && self.resolution.is_auto() && self.layers.iter().all(|l| l.switches == l.switches.pixels()) {
+        self.pixel_form_with(true)
+    }
+
+    /// [`Self::pixel_form`], also without the comp's plain markers and its layers' markers when not
+    /// `with_markers` (see [`Self::same_pixels_with`] and [`Project::reads_markers`]).
+    pub fn pixel_form_with(&self, with_markers: bool) -> std::borrow::Cow<'_, Comp> {
+        if !self.hide_shy
+            && self.resolution.is_auto()
+            && self.layers.iter().all(|l| l.switches == l.switches.pixels())
+            && (with_markers || (self.markers.iter().all(is_timing_marker) && self.layers.iter().all(|l| l.markers.is_empty())))
+        {
             return std::borrow::Cow::Borrowed(self);
         }
         let mut c = self.clone();
         c.hide_shy = false;
         c.resolution = Resolution::Auto;
+        if !with_markers {
+            c.markers.retain(is_timing_marker);
+        }
         for l in &mut c.layers {
             l.switches = l.switches.pixels();
+            if !with_markers {
+                l.markers.clear();
+            }
         }
         std::borrow::Cow::Owned(c)
     }

crates/project/src/item.rs

@@ -98,6 +98,21 @@ impl Default for Project {
 }
 
 impl Project {
+    /// Some layer has an expression, which can read markers (`marker.key(1)`, `numKeys`,
+    /// `nearestKey`...). Only then do the markers of comps and layers affect what is rendered, so
+    /// frame caches ignore them otherwise.
+    pub fn reads_markers(&self) -> bool {
+        // Called for every frame identity and disk-cache key: stops at the first expression and
+        // builds no paths (`PropGroup::walk` formats one per node).
+        fn any_expr(g: &crate::PropGroup) -> bool {
+            g.children.iter().any(|c| match c {
+                crate::Node::Prop(p) => p.expr.is_some(),
+                crate::Node::Group(sub) => any_expr(sub),
+            })
+        }
+        self.comps().any(|(_, c)| c.layers.iter().any(|l| any_expr(&l.props)))
+    }
+
     /// Allocate an id (items, layers and property uids share one counter).
     pub fn alloc(&mut self) -> u64 {
         let id = self.next_id;

crates/project/src/layer.rs

@@ -195,6 +195,12 @@ impl Switches {
 impl Layer {
     /// Draws the same pixels as `o`: equal but for the Audio, Lock and Shy switches.
     pub fn same_pixels(&self, o: &Layer) -> bool {
+        self.same_pixels_with(o, true)
+    }
+
+    /// [`Self::same_pixels`], with the markers counting only when `with_markers` (they are read by
+    /// expressions alone, so a project without any can edit them freely).
+    pub fn same_pixels_with(&self, o: &Layer, with_markers: bool) -> bool {
         // Every field (no `..`: a new field must be considered here); the property tree last.
         let Layer {
             id,
@@ -232,7 +238,7 @@ impl Layer {
             && *preserve_transparency == o.preserve_transparency
             && *track_matte == o.track_matte
             && *parent == o.parent
-            && *markers == o.markers
+            && (!with_markers || *markers == o.markers)
             && *markers_locked == o.markers_locked
             && *auto_orient == o.auto_orient
             && *environment == o.environment

crates/render/src/disk_cache.rs

@@ -903,7 +903,8 @@ pub fn footage_salt_with(project: &effectcraft_project::Project, virtual_id: Opt
 /// Content key of a composition's pixels: project render settings, the comp and everything it
 /// uses (nested comps, footage, solids, their proxies and Use Proxy switches, footage and proxy
 /// files' size and time). With expressions in play (which can read any comp) the whole project
-/// counts. Switches that don't change pixels (Audio, Lock, Shy, Hide Shy Layers) don't count.
+/// counts. Switches that don't change pixels (Audio, Lock, Shy, Hide Shy Layers) don't count, and
+/// neither do markers unless an expression mentions them.
 pub fn comp_content_key(project: &effectcraft_project::Project, comp: effectcraft_project::ItemId) -> u128 {
     comp_content_key_with(project, comp, None)
 }
@@ -942,13 +943,15 @@ fn comp_content_key_for_renderer(
             }
         }
     }
+    // Markers are pixels only to the expressions that read them (#1083).
+    let markers = project.reads_markers();
     let ids: Vec<effectcraft_project::ItemId> = if expressions { project.items.keys().copied().collect() } else { seen.into_iter().collect() };
     for id in ids {
         let Some(it) = project.item(id) else { continue };
         h.write_u64(id.0);
         h.write(it.name.as_bytes());
         match &it.kind {
-            ItemKind::Comp(c) => hash_debug(&mut h, &c.pixel_form()),
+            ItemKind::Comp(c) => hash_debug(&mut h, &c.pixel_form_with(markers)),
             k => hash_debug(&mut h, k),
         }
         if let ItemKind::Footage(f) = &it.kind {

crates/ui-egui/src/frames.rs

@@ -317,23 +317,6 @@ pub fn comp_content(project: &Project, comp: ItemId) -> u64 {
     comp_content_with(project, comp, project, |_, c| c)
 }
 
-/// Whether two versions of a comp draw the same pixels. Markers only count when an expression
-/// could read them (`markers_count`).
-fn same_pixels(a: &Comp, b: &Comp, markers_count: bool) -> bool {
-    if markers_count { a.same_pixels(b) } else { a.same_pixels_ignoring_markers(b) }
-}
-
-/// Whether a layer anywhere in the project has an expression, which can read markers.
-fn project_has_expressions(project: &Project) -> bool {
-    fn has_expression(g: &PropGroup) -> bool {
-        g.children.iter().any(|c| match c {
-            Node::Prop(p) => p.expr.is_some(),
-            Node::Group(sub) => has_expression(sub),
-        })
-    }
-    project.items.values().any(|i| matches!(&i.kind, ItemKind::Comp(c) if c.layers.iter().any(|l| has_expression(&l.props))))
-}
-
 /// [`comp_content`] with each comp's version selected for both its sources and its address.
 /// Sources removed by undo are read from `base`.
 fn comp_content_with<'a>(project: &'a Project, comp: ItemId, base: &'a Project, version: impl Fn(ItemId, &'a Arc<Comp>) -> &'a Arc<Comp>) -> u64 {
@@ -762,12 +745,12 @@ impl Frames {
     fn identity(&self, project: &Arc<Project>, comp: ItemId) -> Identity {
         let plain = comp_content(project, comp);
         let Ok(mut bases) = self.bases.lock() else { return (plain, None) };
-        // Markers can only change pixels through expressions, which read them.
-        let markers_count = project_has_expressions(project);
+        // Markers are pixels only to the expressions that read them (#1083).
+        let markers = project.reads_markers();
         if let Some((base_id, base)) = bases.get(&comp.0)
             && *base_id != plain
             && comp_content_with(project, comp, project, |id, c| match base.item(id).map(|i| &i.kind) {
-                Some(ItemKind::Comp(b)) if Arc::ptr_eq(b, c) || same_pixels(b, c, markers_count) => b,
+                Some(ItemKind::Comp(b)) if Arc::ptr_eq(b, c) || b.same_pixels_with(c, markers) => b,
                 _ => c,
             }) == *base_id
         {
@@ -779,7 +762,7 @@ impl Frames {
         if let Some((base_id, base)) = bases.get(&comp.0)
             && let (Some(old), Some(new)) = (base.comp(comp), project.comp(comp))
             && comp_content_with(project, comp, base, |id, c| match base.item(id).map(|i| &i.kind) {
-                Some(ItemKind::Comp(b)) if id == comp || Arc::ptr_eq(b, c) || same_pixels(b, c, markers_count) => b,
+                Some(ItemKind::Comp(b)) if id == comp || Arc::ptr_eq(b, c) || b.same_pixels_with(c, markers) => b,
                 _ => c,
             }) == *base_id
             && let Some((s, e)) = new.changed_span(old)

crates/ui-egui/src/frames/tests.rs

@@ -895,33 +895,59 @@ fn adding_a_source_does_not_hide_changes_to_existing_dependencies() {
     }
 }
 
+/// Adding, moving or deleting a marker (Num* while a preview plays) keeps the cached frames:
+/// the sound would otherwise stop at the next frame that has to be rendered again (#1083).
+/// Only expressions that read markers make them part of the pixels.
 #[test]
-fn markers_keep_the_frames() {
+fn markers_keep_the_frames_unless_an_expression_reads_them() {
     use effectcraft_engine::Session;
     use serde_json::json;
     let mut s = Session::default();
     let id = |v: serde_json::Value, k: &str| v[k].as_u64().unwrap();
-    let outer = ItemId(id(s.execute("comp.new", json!({"name": "Outer", "width": 8, "height": 8, "duration": 1})).unwrap(), "comp"));
-    let solid = id(s.execute("layer.newSolid", json!({"comp": outer.0, "color": "#ff0000"})).unwrap(), "layer");
+    let inner = ItemId(id(s.execute("comp.new", json!({"name": "Inner", "width": 8, "height": 8, "duration": 2})).unwrap(), "comp"));
+    let solid = id(s.execute("layer.newSolid", json!({"comp": inner.0, "color": "#ff0000"})).unwrap(), "layer");
+    let outer = ItemId(id(s.execute("comp.new", json!({"name": "Outer", "width": 8, "height": 8, "duration": 2})).unwrap(), "comp"));
+    id(s.execute("layer.addItem", json!({"comp": outer.0, "item": inner.0})).unwrap(), "layer");
     let f = Frames::default();
-    let at = |s: &Session| f.content_of(&s.project, s.revision, outer);
-    let o0 = at(&s);
-    s.execute("layer.addMarker", json!({"comp": outer.0, "layers": [solid], "time": 0.5, "comment": "beat"})).unwrap();
-    assert_eq!(at(&s), o0, "a marker changes no pixels, so it keeps the frames");
+    let at = |s: &Session, c: ItemId| f.content_of(&s.project, s.revision, c);
+    let (o0, i0) = (at(&s, outer), at(&s, inner));
+    s.execute("comp.addMarker", json!({"comp": inner.0, "time": 0.5, "comment": "beat"})).unwrap();
+    s.execute("comp.addMarker", json!({"comp": outer.0, "time": 1.0})).unwrap();
+    s.execute("comp.addMarker", json!({"comp": outer.0, "time": 1.5})).unwrap();
+    assert_eq!((at(&s, outer), at(&s, inner)), (o0, i0), "comp markers");
+    s.execute("markers.set", json!({"comp": inner.0, "layer": solid, "new": true, "time": 0.25})).unwrap();
+    assert_eq!((at(&s, outer), at(&s, inner)), (o0, i0), "layer marker");
+    // An expression reading markers makes them count again.
+    s.execute("prop.setExpression", json!({"comp": inner.0, "layer": solid, "path": "transform/opacity", "expression": "thisComp.marker.numKeys"})).unwrap();
+    let (o1, i1) = (at(&s, outer), at(&s, inner));
+    assert_ne!(o1, o0);
+    s.execute("comp.addMarker", json!({"comp": inner.0, "time": 1.0})).unwrap();
+    assert_ne!(at(&s, inner), i1, "marker read by an expression");
+    assert_ne!(at(&s, outer), o1, "marker read by an expression in a nested comp");
 }
 
+/// A protected time marker with a duration sets the comp's timing (Responsive Design), so it
+/// changes the frames even though plain markers do not; and any expression, which can read
+/// markers, makes every marker count.
 #[test]
-fn markers_count_when_an_expression_can_read_them() {
+fn timing_markers_and_expressions_still_change_the_frames() {
     use effectcraft_engine::Session;
     use serde_json::json;
     let mut s = Session::default();
     let id = |v: serde_json::Value, k: &str| v[k].as_u64().unwrap();
     let outer = ItemId(id(s.execute("comp.new", json!({"name": "Outer", "width": 8, "height": 8, "duration": 1})).unwrap(), "comp"));
     let solid = id(s.execute("layer.newSolid", json!({"comp": outer.0, "color": "#ff0000"})).unwrap(), "layer");
-    s.execute("prop.setExpression", json!({"comp": outer.0, "layer": solid, "path": "transform/opacity", "expression": "1"})).unwrap();
     let f = Frames::default();
     let at = |s: &Session| f.content_of(&s.project, s.revision, outer);
     let o0 = at(&s);
     s.execute("layer.addMarker", json!({"comp": outer.0, "layers": [solid], "time": 0.5, "comment": "beat"})).unwrap();
-    assert_ne!(at(&s), o0, "an expression can read the marker, so the frames change");
+    assert_eq!(at(&s), o0, "a plain marker changes no pixels, so it keeps the frames");
+    s.execute("comp.addMarker", json!({"comp": outer.0, "time": 0.25})).unwrap();
+    s.execute("markers.set", json!({"comp": outer.0, "index": 0, "duration": 0.25, "protected": true})).unwrap();
+    let o1 = at(&s);
+    assert_ne!(o1, o0, "a protected time marker with a duration sets the timing");
+    s.execute("prop.setExpression", json!({"comp": outer.0, "layer": solid, "path": "transform/opacity", "expression": "1"})).unwrap();
+    let o2 = at(&s);
+    s.execute("layer.addMarker", json!({"comp": outer.0, "layers": [solid], "time": 0.75})).unwrap();
+    assert_ne!(at(&s), o2, "an expression can read the marker, so the frames change");
 }

crates/ui-egui/tests/it/ui_audio_scrub.rs

@@ -651,3 +651,31 @@ fn startup_gives_up_on_a_first_frame_that_never_caches() {
     assert!(fake.0.lock().unwrap().0.is_none(), "no sound started and dropped");
     assert!(h.state().playback.playing && h.state().playback.audio_held, "it goes on as a stalled preview");
 }
+
+/// Adding markers while a preview sounds (Num*, one per beat) keeps the sound going (#1083): a
+/// marker changes no pixels, so no cached frame is dropped and no frame has to be rendered again
+/// (which would stop the sound until the frames ahead are cached).
+#[test]
+fn adding_markers_does_not_stop_the_sound() {
+    let fake = Fake::default();
+    let (app, cid) = setup(true, &fake);
+    // Everything is cached before the preview starts, so the sound starts at once.
+    let mut h = Harness::builder().with_size(egui::vec2(1400.0, 900.0)).with_step_dt(1.0 / 31.0).build_eframe(|_| app);
+    h.run_steps(2);
+    let deadline = std::time::Instant::now() + std::time::Duration::from_secs(60);
+    let now = h.ctx.input(|i| i.time);
+    h.state_mut().play(now);
+    while h.state().audio.is_none() && std::time::Instant::now() < deadline {
+        h.step();
+        std::thread::sleep(std::time::Duration::from_millis(1));
+    }
+    assert!(h.state().audio.is_some(), "the sound started");
+    for beat in 0..6 {
+        let t = 0.1 + 0.05 * beat as f64;
+        h.state_mut().session.execute("comp.addMarker", json!({"comp": cid.0, "time": t, "comment": "beat"})).unwrap();
+        h.step();
+        let app = h.state();
+        assert!(app.playback.playing && app.audio.is_some() && !app.playback.audio_held, "marker {beat}: still playing with sound");
+    }
+    assert_eq!(h.state().session.project.comp(cid).unwrap().markers.len(), 6);
+}