fix(encode): trust the decoded signal spec for rate and channels by LeadcodeDev · Pull Request #236 · LeadcodeDev/rustmotion

LeadcodeDev · 2026-09-21T23:38:15Z

Severity Low, category correctness. Location: crates/rustmotion/src/encode/audio.rs:67

Impact

The interleaved samples appended to all_samples come from decoded.spec(), but the returned (sample_rate, channels) come from the container header with unwrap_or fallbacks. These diverge for real files: symphonia's ADTS AAC reader only sets params.with_channels(channels) when header.channels is Some (symphonia-codec-aac-0.5.5/src/adts.rs:139-141) — an ADTS stream with channel_config == 0 leaves codec_params.channels == None, so rustmotion assumes 2 while the decoder actually emits mono. to_stereo(&samples, 2) (line 205) then passes the mono buffer through untouched, so the track is half its real length and plays at double speed; mix_audio_tracks_segment sizes everything from that, and the muxed audio desyncs from video with no error anywhere. audio_analysis.rs:143-149 divides by the same wrong channels for the waveform/spectrum envelope, so the on-screen visualisation agrees with the broken mix.

Fix

Capture spec.rate and spec.channels.count() from the first decoded packet and return those, falling back to codec_params only when no packet ever decodes. Also bail out (or resample per-segment) if a later packet's spec differs from the first — today, chained streams get concatenated into one flat Vec under a single assumed layout.

Evidence the audit read

let sample_rate = track.codec_params.sample_rate.unwrap_or(44100);
    let channels = track
        .codec_params
        .channels
        .map(|c| c.count() as u32)
        .unwrap_or(2);
...
        let spec = *decoded.spec();
        let duration = decoded.capacity();
        let mut sample_buf = SampleBuffer::<f32>::new(duration as u64, spec);

Stacked on fix/encode-atempo-guard, which carries the previous finding of this workstream. GitHub shows only this finding's diff; merge in order.

Part of the September 2026 audit remediation chantier. Refs #220 (RM-34).

`remaining` never reaches 0.5 when it starts at 0.0 (0.0/0.5 == 0.0) or at any negative value (it diverges toward -inf). The loop never terminates and pushes a fresh `String` on every iteration, so rustmotion hangs while consuming memory until the OOM killer fires. Reached from a plain scenario JSON: `{"type":"video","src":"clip.mp4","playback_rate":0}` — `volume` defaults to 1.0 (`crates/rustmotion-components/src/video.rs:14` `fn default_volume() -> f32 { 1.0 }`), so `collect_videos_in_child` collects it (`v.volume > 0.0`, line 128), `collect_video_audio_tracks` calls `extract_audio_to_wav(..., occ.playback_rate)` (line 421), which calls `build_atempo_filter(0.0)` at line 333. `playback_rate` is an unvalidated `Option<f64>` (`crates/rustmotion-components/src/video.rs:26`); a repo-wide grep shows no validator constrains it. Both the ffmpeg render path (`encode_with_ffmpeg_hw_impl` line 432) and the native path (`mux.rs:34`) reach it. Refs #220

Triggering scenario: a scenario JSON declaring a `video` component with `style.width/height` of 1920x1080 inside a 60 s scene at 30 fps. `.output()` (line 234) collects ffmpeg's entire rawvideo stdout into one `Vec<u8>`: 1800 frames x 8.3 MB = 15 GB. `data` then stays borrowed for the whole copy loop, so the per-frame `.to_vec()` copies build a second 15 GB structure alongside it — 30 GB peak before `output.stdout` is dropped. The result is inserted into the process-global `VIDEO_FRAME_CACHE` (assets.rs:168), which has no eviction and no `clear` function at all. There is no size, duration, or resolution cap anywhere on this path, and the inputs are entirely scenario-controlled. Under `--watch` this compounds: changing the component's `style.width` mints a new `"src:WxH"` key while the old multi-GB entry is never freed. Secondary hazard on the same line: `width * height * 4` is plain u32 arithmetic, so a declared 65536x16384 wraps to 0 and `data.len() / frame_size` panics with a divide-by-zero. Refs #220

Triggering scenario: `rustmotion render deck.json -o out.mp4 --watch` left open for an afternoon, with an `image` component pointing at `logo.png`. The author re-exports `logo.png`. `ASSET_CACHE` is keyed on the path alone (image.rs:41-54) with no mtime or size component, and in the incremental branch `clear_asset_cache()` only runs when the video config hash changes (resolution/fps) — editing scene content never clears it. Every subsequent re-render silently re-uses the stale decoded bitmap; the author sees the old logo indefinitely with nothing on stderr. The adjacent audio path solved exactly this and documents why (`encode/audio_analysis.rs:29-39`: fingerprint on len+mtime because 'someone re-exports a mix while the studio is open') — the image/GIF/video caches never got the same treatment. Memory side: `clear_asset_cache` touches only `ASSET_CACHE`; `GIF_CACHE`, `VIDEO_FRAME_CACHE` and `AUDIO_ANALYSIS_CACHE` have no clear function at all, so a long `--watch` session that touches several asset variants accumulates every decode for the process lifetime. Refs #220

Refs #220

Two parsers for the same syntax have drifted. Executed: `style="padding: 24px 48px; margin: 0 auto; grid-template-columns: repeat(3, 1fr)"` transpiles to `"padding":"24px 48px"`, `"margin":"0 auto"`, `"grid-template-columns":["repeat(3,","1fr)"]`. All three deserialize successfully — `Edges::Uniform(LengthPercentage::String)` and `GridTrack::Length(LengthPercentage::String)` are untagged string variants — and then core's `parse_length` (rustmotion-core/src/css/units.rs:125) only understands a single number+unit token, so `parse_length_or_warn` (units.rs:262) falls back to `Px(0.0)`: zero padding, zero margin, two zero-width grid tracks. The only signal is an `eprintln!` warning; `validate` exits 0 and the render is wrong. Per-side padding is entirely unexpressible from HTML (`padding-top:32` is rejected by `CssStyle`'s `deny_unknown_fields`, `padding:32px 48px` silently becomes 0), and the skill rule html-css-mental-model.md:97 only documents the JSON object form, which inline `style=""` cannot produce. Refs #220

…#290) `render_frame` panics on failure (`.expect("render frame")`, `.expect("rgba matches dimensions")`, `.expect("encode jpeg")`). Because the panic happens in the scoped child thread and is consumed by `join().unwrap_or_default()`, the `catch_unwind` wrapper in `view.rs::serve_or_render` never fires — so `fail_ledger().record_failure(key)` is unreachable on that path and the whole retry-budget mechanism is dead code for the on-demand asset handler. Instead the empty `Vec` is treated as a successful render: `frame_cache().insert(key, rendered.clone(), ...)` caches zero bytes, and the responder replies `200 image/jpeg` with an empty body. The canvas shows a permanently broken image for that (generation, frame, scale) and the cache guarantees it is never re-attempted. (The prefetch worker path calls `render_frame` directly and is correctly fenced.) Refs #220

`src` is the `video` component's scenario field, unfiltered. `info.rs:385-391` documents this explicitly ("video's ffmpeg-backed path only reaches one incidentally by handing the string straight to ffmpeg's own demuxer") — so the network reach is acknowledged as accidental, not designed. ffmpeg is invoked without `-protocol_whitelist`, which means the scenario selects from ffmpeg's entire built-in protocol set: `src: "http://169.254.169.254/latest/meta-data/"` or `http://127.0.0.1:8500/...` makes the renderer issue that request (SSRF from an LLM-authored scenario, and a reliable internal port-scan oracle via the exit status printed at preload.rs:252-258). The same unfiltered string reaches `preload.rs:221-222` and `encode/video_audio.rs:326-327`. Secondary risk: playlist/subtitle demuxers that dereference nested `file:` URLs would render local file content into the output video. There is no timeout on the subprocess either, so a slow remote URL stalls the render. Refs #220

box_builder.rs:496 calls `rustmotion_core::css::cascade::inherit_from(parent_css, &mut css)` and paint_pass.rs:479 passes `&node.css` into `dispatch`. `LegacyPaintDispatcher` (the only real `PaintDispatcher`; the other is `NoopDispatcher`) binds it to `_css` and drops it, and `Painter::paint_content` (painter.rs:60-66) has no `CssStyle` parameter at all — so a painter structurally cannot see the cascaded style. Every painter resolves typography from its own declared struct field instead (text.rs:523 `self.style.color_str_or("#FFFFFF")`). All twelve properties `inherit_from` propagates (color, font-family, font-size, font-weight, font-style, line-height, letter-spacing, text-align, white-space, overflow-wrap, visibility, text-decoration) are paint-only: taffy_bridge.rs reads none of them (its own header says "Paint properties (color, background, transform...)" are excluded), paint_pass.rs contains zero references to `visibility`/`text_decoration`/`text_align`/`font_family`/`white_space`, and `component_intrinsic` (box_builder.rs:639) builds measurers from `&child.component`, not from the cascaded `css`. Net effect: `{"type":"card", "style":{"color":"#ff0000"}}` wrapping a `text` with no colour of its own renders white, not red — the documented CLAUDE.md feature "cascade.rs — Héritage color/font-* parent → enfant" has no observable effect. The regression test `card_color_cascades_to_text_child_with_no_color_of_its_own` (box_builder.rs:3066) passes because it only asserts on the intermediate `text_box.css.color` field, never on painted output. Refs #220

For `angle_deg = 180` (the default, CSS "to bottom"): rad = 0, sin_a = 0, cos_a = -1, so p0 = (cx, cy + h/2) = bottom edge and p1 = top edge. Skia places `colors[0]` at p0, so the first stop lands at the *bottom* where CSS puts it at the top. Same 180-degree offset at every angle (90 -> first stop on the right, CSS says left). The repo's own skill doc contradicts the rendered result: `.claude/skills/rustmotion/rules/glassmorphism.md:114` prescribes `"gradient-border": { "colors": ["#FFFFFF50", "#FFFFFF08"], "width": 1.5, "angle": 180 }` and describes it at line 118 as "la lumière qui accroche le haut de la carte" — but this function puts the bright stop at the bottom. `paint_gradient_border` (line 1375) shares the function, so both `background` gradients and `gradient-border` are affected. Note the engine also carries a second, incompatible convention for `Fill::Gradient` in rustmotion-components/src/shape.rs:78 (angle 0 = left-to-right, math convention). Refs #220

`spring_value` is reached per animated property, per node, per frame (`box_builder.rs:538` -> `resolve_props_for_effects` -> `resolve_animation_value_full`, animator.rs:997). When `SpringConfig::duration` is set, each of those calls runs a full coarse scan plus 40 bisection steps, with `steps` clamped to [2000, 20000]. For the shipped defaults (stiffness 100, mass 1 -> omega 10, period 0.628 s) `desired_steps` is 2292, so one animated property costs ~2333 `exp`/`sin`/`cos` evaluations per frame to recompute a value that depends only on (damping, stiffness, mass, rest_threshold) — completely invariant across frames. A 30 s/60 fps render with 20 nodes carrying two spring-duration properties each burns roughly 165 million redundant solver evaluations. The function's own doc comment acknowledges it is "called on every `spring_value` sample when `duration` is set". Refs #220

) `loaded.raw` is captured AFTER `rustmotion::assets::rebase_relative_paths(&mut json_value, dir)` runs (crates/rustmotion/src/cli/commands/validation.rs:194-196), which rewrites every `src`/`track` string that names an existing file next to the scenario into a canonicalized ABSOLUTE path (crates/rustmotion/src/assets.rs:62-78). `refuse_fix` (validate.rs:81-109) only guards HTML, `config`/`$`, `include`, and `for-each`/`use` — nothing stops this. So `rustmotion validate --fix scene.json` on a scenario containing `"src": "assets/logo.png"` silently overwrites the source with `"src": "/Users/alice/proj/assets/logo.png"`, which no longer resolves on any other machine or in CI, and pollutes the diff with paths the author never typed. This is the same class of unfaithful write-back that `FixRefusal` exists to prevent. Secondary hazard from the same line: `rustmotion-studio` enables serde_json's `preserve_order` (crates/rustmotion-studio/Cargo.toml:21) while `crates/rustmotion/Cargo.toml:29` does not, so under resolver-2 feature unification a workspace build preserves key order but a standalone `cargo install rustmotion` build alphabetises every object in the rewritten file. Refs #220

`bbox` here is `raw_bbox = bbox_of(layout)` (geometry.rs:272, 376-383), i.e. `layout.width` — the BORDER box. But `LegacyPaintDispatcher::dispatch` (crates/rustmotion-components/src/legacy_dispatch.rs) hands every non-codeblock painter a synthetic `BoxLayout { width: cw, height: ch, border/padding: zero }` taken from `layout.content_box()` and translates the canvas to the content-box origin. So `Text::paint` wraps and draws inside `cw`, not `layout.width`. A `white-space: nowrap` text with `style.padding: "0 24px"` in a 300px box has `cw = 252`; a natural line width of 280px paints 28px past the box's right edge, yet `280 > 300` is false and nothing is reported. The sibling check `check_content_overflows_box` (geometry.rs:848) does this correctly via `layout.content_box()` — the two checks disagree about which box text lives in, and the flagship `unwrappable_text_overflow` is the one that is wrong. The gap equals `padding.left + padding.right + border.left + border.right`. Refs #220

`decoder.width()/height()` come straight from the GIF logical-screen descriptor (2 bytes each, max 65535). A ~200-byte crafted GIF declaring 65535×65535 makes line 129 request a single 17.2 GB allocation before a single pixel is decoded. Worse, the `while` loop then pushes a *full-canvas clone* per animation frame (line 141) with no frame-count limit — a 4000×4000 canvas with 500 frames is 32 TB of `Vec<u8>` — and the result is inserted into the global `gif_cache()` (`renderer/assets.rs:32`) which is never evicted. A scenario whose `gif` src points at such a file aborts the render process (Rust allocation failure = abort, not a catchable error), which is a denial of service on any batch/CI renderer fed third-party scenarios or media. Refs #220

`resolve_name_template` walks the template as raw bytes and casts each one to `char`, which in Rust is a Latin-1 reinterpretation of the byte value. Substituted `{field}` values are fine (they go through `push_str` at line 71); the literal text around them is not. I confirmed the behaviour by compiling the same loop standalone: `résumé-{id}.mp4` yields `rÃ©sumÃ©-VAL.mp4` and `动画-{id}.mp4` yields `å¨ç»-VAL.mp4`. So `rustmotion batch --name-template "résumé-{id}.mp4"` writes files literally named `rÃ©sumÃ©-abc.mp4` to disk, and the same corruption lands in the `[ok] <path>` progress lines and in every failure message (`item N: ...`). A localisation batch — the exact use case `{lang}/{id}.mp4` in the module doc is built for — is where a non-ASCII template is most likely. None of the five tests in `name_template_tests` (lines 448-495) uses a non-ASCII template. Refs #220

Refs #220

Every non-decorative node is timed by `PaintWindow::contains` — `self.end.is_none_or(|e| t < e)` (rustmotion-core/src/engine/box_tree.rs:43), a half-open window — and gets its effects from `box_builder::effective_effects` (legacy_dispatch.rs:110), which folds in `timeline` steps and `style.transition` keyframes. This second path uses an inclusive `time > e` and calls `a.animation_effects()` raw. `Particle` declares `timeline: Vec<TimelineStep>` (particle.rs:57), so a particle's timeline steps and transition keyframes are silently dropped, and its `end_at` keeps it visible for one extra frame — but only in a `world` view, since this is the sole call site (scene.rs:1130). The same particle in a `slide` view behaves correctly. A component whose behaviour depends on which view type contains it is invisible to tests written against either one. Refs #220

#250) `cfg.color` is a free-form `String` on `HeropatternConfig` (crates/rustmotion-core/src/schema/background.rs:216, `#[serde(default = "default_hero_color")]`) with no validation anywhere — `validate_animated_bg` (crates/rustmotion/src/include.rs:267-273) checks only that the pattern *name* resolves. The template lands inside a double-quoted attribute (`fill="{{color}}"`), so a colour containing `"` closes the attribute and injects arbitrary markup into the document usvg then parses. usvg 0.46's `Options::default()` keeps the default `ImageHrefResolver`, whose string resolver I read at usvg-0.46.0/src/parser/image.rs:85-112: it calls `std::fs::read(&path)` on any href that `path.exists()` accepts — with `resources_dir: None` an absolute path resolves to itself — so an injected `<image href="/absolute/path.png">` is decoded and painted into the frame. The impact is bounded (a scenario can already read a local image legitimately via `{"type":"image","src":"/abs/path.png"}`, and there is no network resolver), so this is a hardening gap rather than a privilege escalation. The quieter half is availability: any malformed splice makes `Tree::from_data` fail and line 601 `return`s, so the background silently does not render with no diagnostic at all. Refs #220

The `TagKind::Text` arm never calls `element_to_value` on its children, so the whole guard layer (`StyleElementUnsupported`, `UnsupportedNativeElement`, `TagKind::Ignored`) is unreachable below `p/span/h1..h6/strong/em/label`. Executed against the real crate: `<p>Real <script>var secret = 1; alert(2);</script></p>` transpiles to `{"content":"Real var secret = 1; alert(2);","type":"text"}` — the JS source is rendered on screen; `<h1>Title<style>h1{color:#0f0}</style></h1>` gives `{"content":"Titleh1{color:#0f0}"}`; `<p>cap<img src="hero.png"></p>`, `<h1>t<svg>…</svg></h1>` and `<span>n=<rm-counter from="0" to="100"></rm-counter></span>` drop the img/svg/counter entirely and exit 0. This is a live regression of "constat 1" and "constat 3", which tests/silent_loss_regressions.rs asserts are fixed — the fix only ever covered the `Container` path. Refs #220

All four write-back entry points (`set_inline_style`, `set_text_content`, `set_attribute`, `remove_inline_style`) re-serialize only the `<rustmotion>` element, and rustmotion-studio writes that string straight over the source file (editor/inspector.rs:2168 -> `write_and_note(path, &updated)`). Executed: a 281-byte document with `<!DOCTYPE html>`, `<html><head><meta charset="utf-8"></head><body>`, a leading `` comment and a trailing comment becomes 150 bytes — doctype, head, meta, body and both outer comments gone — after one font-size tweak in the inspector. The transpiler happily accepts such documents (`find_element` is a document-wide DFS), so this is a supported authoring form being destroyed. The docstring's "formatting is normalized" does not cover deleting the surrounding document. Secondarily, `serialize_element` (lib.rs:488-497) does `let _ = serialize(...)` and `String::from_utf8(buf).unwrap_or_default()`, so a serializer failure would write an empty file rather than surface an error. Refs #220

`MAX_EXPANSION_DEPTH` bounds how deeply directives may nest, not how many nodes they produce, and nesting one `for-each` inside another's `template` is explicitly supported (`use_template_can_contain_a_nested_for_each`, expand.rs:1005). Each nesting level costs exactly +1 depth (resolve_entry recurses with `depth + 1` per produced node, while `walk_children` forwards `depth` unchanged), so up to 64 levels are legal and the output is the product of the array lengths. Four nested levels of 50 elements is 6.25M nodes from a file under 1 KB; eight levels of 10 is 10^8. Each iteration does a full `Value` deep-clone of the template plus a `substitute` walk, so the process hangs and OOMs during `expand_directives` — before validation, before any render, and with no diagnostic. An LLM-authored or third-party scenario reaches this through the documented public syntax; `--no-validate` is not needed. Refs #220

Any scenario handed to `rustmotion render`/`validate` can make the user's machine issue GETs to hosts of the author's choosing, with no host/scheme allowlist, no redirect policy, no timeout set on the call, and no `--offline` switch to refuse. On a developer laptop or a CI runner this reaches loopback services, RFC1918 addresses and cloud link-local metadata (169.254.169.254), and it doubles as a silent beacon that fires merely from *validating* a file (validation::load goes through the same `resolve_includes`). Fetching remote includes is clearly deliberate design, so the finding is the absence of any control around it, not the feature. Refs #220

`TableIntrinsic::compute_width` (intrinsic.rs:911-949) measures every header and every cell, keeps a per-column max (`col_widths[i] = col_widths[i].max(w + cell_padding * 2.0)`) and returns the **sum**. The painter then throws that per-column distribution away and gives each column `total_w / col_count`. For an uneven table (e.g. headers `["ID", "Description of the incident"]`, natural widths 60px and 400px → intrinsic 460px) the painter allocates 230px per column; the 400px column's text is drawn from `align_text_x` at a position computed against a 230px cell and there is no per-cell clip (table.rs:203-262 draws each cell with `draw_text_with_fallback` and only advances `col_x += cw`), so the text overlaps the neighbouring column or spills past the table's right edge. With `column_align: "right"` it spills backwards into the previous column. The measurer's own doc comment (intrinsic.rs:884-888) asserts the opposite: "Natural size formula (matches the painter exactly) ... otherwise each column gets `max(header_text_width + 2 × cell_padding, min_col_width)`" — the painter implements no such rule, and `compute_width` has no `min_col_width` either. Refs #220

The interleaved samples appended to `all_samples` come from `decoded.spec()`, but the returned `(sample_rate, channels)` come from the container header with `unwrap_or` fallbacks. These diverge for real files: symphonia's ADTS AAC reader only sets `params.with_channels(channels)` when `header.channels` is `Some` (symphonia-codec-aac-0.5.5/src/adts.rs:139-141) — an ADTS stream with `channel_config == 0` leaves `codec_params.channels == None`, so rustmotion assumes 2 while the decoder actually emits mono. `to_stereo(&samples, 2)` (line 205) then passes the mono buffer through untouched, so the track is half its real length and plays at double speed; `mix_audio_tracks_segment` sizes everything from that, and the muxed audio desyncs from video with no error anywhere. `audio_analysis.rs:143-149` divides by the same wrong `channels` for the waveform/spectrum envelope, so the on-screen visualisation agrees with the broken mix. Refs #220

LeadcodeDev · 2026-09-22T09:11:07Z

Closed and reopened as a fresh pull request: GitHub registered this one as part of a stack, and a stacked pull request cannot be merged through either the GraphQL or the REST merge endpoint. The finding itself is unchanged.

LeadcodeDev added the bug Something isn't working label Sep 21, 2026

LeadcodeDev self-assigned this Sep 21, 2026

LeadcodeDev force-pushed the fix/audio-signal-spec branch from 24a7db8 to a48727d Compare September 21, 2026 23:40

LeadcodeDev mentioned this pull request Sep 21, 2026

chantier: September 2026 audit remediation — 53 findings, one PR each #220

Closed

53 tasks

LeadcodeDev force-pushed the fix/audio-signal-spec branch from aefd76d to c94fb86 Compare September 22, 2026 06:10

LeadcodeDev added this pull request to stack #280 September 22, 2026 06:40

LeadcodeDev force-pushed the fix/audio-signal-spec branch from c94fb86 to d8f814e Compare September 22, 2026 08:34

LeadcodeDev force-pushed the fix/audio-signal-spec branch from d8f814e to 43a3840 Compare September 22, 2026 08:43

LeadcodeDev force-pushed the fix/audio-signal-spec branch from 43a3840 to bf89598 Compare September 22, 2026 08:59

LeadcodeDev added 12 commits September 22, 2026 10:59

fix(video): loop the picture with the audio (#245)

9890365

Refs #220

fix(video): treat decoded frames as unpremultiplied (#247)

7298fd8

Refs #220

LeadcodeDev force-pushed the fix/audio-signal-spec branch from bf89598 to af73683 Compare September 22, 2026 09:04

LeadcodeDev added 2 commits September 22, 2026 11:05

LeadcodeDev added 10 commits September 22, 2026 11:05

fix(video): honour the fit property (#244)

4ac841d

Refs #220

fix(video): stop the picture at trim_end (#246)

41689b8

Refs #220

LeadcodeDev force-pushed the fix/audio-signal-spec branch from af73683 to a190eb8 Compare September 22, 2026 09:08

LeadcodeDev closed this Sep 22, 2026

Navigation Menu

Repository navigation

fix(encode): trust the decoded signal spec for rate and channels - #236

fix(encode): trust the decoded signal spec for rate and channels#236
LeadcodeDev wants to merge 25 commits into
fix/encode-atempo-guardfrom
fix/audio-signal-spec

LeadcodeDev commented Sep 21, 2026

Uh oh!

LeadcodeDev commented Sep 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

LeadcodeDev commented Sep 21, 2026

Impact

Fix

Evidence the audit read

Uh oh!

LeadcodeDev commented Sep 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant