Skip to content

Commit 6b8e2d9

Browse files
committed
fix(assets): make icon preloading work at all, and asset failures visible
Three defects that all end the same way: a hole in the video and no diagnostic. - Icon preloading could never work. The painter keyed its cache on the oversampled render size, the preloader on the target size — for a 40x40 icon one wrote `80x80` and the other read `40x40`, so the keys could not collide at any size. The mechanism meant to stop N render threads hitting the network in parallel added a wasted download and rasterization, then let every thread fetch anyway. The preloader also rasterized without the oversample, so fixing the key alone would have turned a useless preload into a harmful one: the painter would finally find a bitmap, at half the resolution. Both sites now go through one `icon_cache_key`, which applies the oversample and builds the key — the drift is no longer expressible. - Icons had no disk cache and failed silently at every level. There is now a cache on disk, following the pattern `google_fonts.rs` already uses in this crate, and an unresolvable icon fails the preload instead of leaving a gap. - Video frame extraction failed silently too: a missing or failing ffmpeg produced an entirely empty video with no warning. PR #151 had already hardened the *audio* extraction of embedded videos in this same crate, with an availability probe and a warning emitted once; that discipline is now extended to the frame path, which had been left behind. Warnings added here are deduplicated: `prepare_scene` runs per frame, so an unguarded one would print over a thousand times on a 1200-frame render.
1 parent 3331a5b commit 6b8e2d9

4 files changed

Lines changed: 552 additions & 18 deletions

File tree

crates/rustmotion-components/src/icon.rs

Lines changed: 120 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ use skia_safe::{Canvas, ColorType, ImageInfo, Paint, Rect, SamplingOptions};
55
use rustmotion_core::css::CssStyle;
66
use rustmotion_core::engine::animator::AnimatedProperties;
77
use rustmotion_core::engine::layout_pass::BoxLayout;
8-
use rustmotion_core::engine::renderer::{asset_cache, fetch_icon_svg};
8+
use rustmotion_core::engine::renderer::{asset_cache, fetch_icon_svg, icon_cache_key};
99
use rustmotion_core::schema::TimelineStep;
1010
use rustmotion_core::traits::{PaintCtx, Painter, TimingConfig};
1111

@@ -38,22 +38,36 @@ impl Painter for Icon {
3838
_ctx: &PaintCtx,
3939
) {
4040
let color = self.style.color_str_or("#FFFFFF");
41-
// Oversample 2× so the rasterized SVG stays crisp under sub-pixel
42-
// positioning and minor scale animations. Skia's high-quality
43-
// sampling downscales to layout size without softening edges.
44-
const OVERSAMPLE: u32 = 2;
4541
let target_w = (layout.width as u32).max(1);
4642
let target_h = (layout.height as u32).max(1);
47-
let render_w = target_w * OVERSAMPLE;
48-
let render_h = target_h * OVERSAMPLE;
49-
50-
let cache_key = format!("icon:{}:{}:{}x{}", self.icon, color, render_w, render_h);
43+
// Oversampling (crisp edges under sub-pixel positioning / scale
44+
// animation) and the cache key are computed together by
45+
// `icon_cache_key` — see its doc for why that matters (issue #166):
46+
// this used to be a local `const OVERSAMPLE` here plus an
47+
// independent `format!` in `preload.rs`'s prefetcher, and the two
48+
// could never agree.
49+
let (render_w, render_h, cache_key) = icon_cache_key(&self.icon, color, target_w, target_h);
5150

5251
let cache = asset_cache();
5352
let img = if let Some(cached) = cache.get(&cache_key) {
5453
cached.clone()
5554
} else {
5655
let Ok(svg_data) = fetch_icon_svg(&self.icon, color, render_w, render_h) else {
56+
// Preload (issue #167 item 2) already hard-fails when an
57+
// icon cannot be resolved via disk cache or network, so this
58+
// branch is defense in depth (paint_content can run without
59+
// a preceding prefetch, or the layout-derived target size
60+
// here can differ from the preloader's style-based
61+
// estimate, producing a genuine cache miss). Guarded so a
62+
// single offline/typo'd icon does not spam once per frame
63+
// over a render that can be 1000+ frames long.
64+
if crate::warn_once_for(&format!("icon-fetch-failed:{}", self.icon)) {
65+
eprintln!(
66+
"Warning: icon '{}' could not be loaded (checked the disk cache and \
67+
the network) — nothing will be painted for it.",
68+
self.icon
69+
);
70+
}
5771
return;
5872
};
5973

@@ -100,3 +114,100 @@ impl Painter for Icon {
100114
);
101115
}
102116
}
117+
118+
#[cfg(test)]
119+
mod tests {
120+
use super::*;
121+
122+
fn base_ctx() -> PaintCtx {
123+
PaintCtx {
124+
time: 0.0,
125+
scene_duration: 1.0,
126+
frame_index: 0,
127+
fps: 30,
128+
video_width: 100,
129+
video_height: 100,
130+
stagger_offset: 0.0,
131+
}
132+
}
133+
134+
fn solid_image() -> skia_safe::Image {
135+
let px = [255u8, 0, 255, 255];
136+
let mut data = Vec::with_capacity(4 * 4);
137+
for _ in 0..4 {
138+
data.extend_from_slice(&px);
139+
}
140+
let img_info = ImageInfo::new(
141+
(2, 2),
142+
ColorType::RGBA8888,
143+
skia_safe::AlphaType::Premul,
144+
None,
145+
);
146+
let skia_data = skia_safe::Data::new_copy(&data);
147+
skia_safe::images::raster_from_data(&img_info, skia_data, 2 * 4).expect("sentinel image")
148+
}
149+
150+
/// Regression for issue #166: proves the painter's cache-key formula is
151+
/// literally `icon_cache_key`. Pre-populate `asset_cache()` under the
152+
/// exact key `preload.rs`'s prefetcher now computes for a 40×40 target,
153+
/// then confirm the painter finds and paints it — instead of falling
154+
/// through to `fetch_icon_svg` for a nonsense icon id (which pre-fix, on
155+
/// a key mismatch, is exactly what would have happened every time).
156+
#[test]
157+
fn painter_finds_the_entry_preload_would_have_written() {
158+
let icon = Icon {
159+
icon: "test-suite:icon-cache-key-agreement".to_string(),
160+
timing: Default::default(),
161+
style: CssStyle::default(),
162+
timeline: Vec::new(),
163+
stagger: None,
164+
};
165+
let target_w = 40u32;
166+
let target_h = 40u32;
167+
let color = icon.style.color_str_or("#FFFFFF");
168+
let (_, _, key) = icon_cache_key(&icon.icon, color, target_w, target_h);
169+
170+
asset_cache().insert(key.clone(), solid_image());
171+
172+
let layout = BoxLayout {
173+
width: target_w as f32,
174+
height: target_h as f32,
175+
..Default::default()
176+
};
177+
let ctx = base_ctx();
178+
let props = AnimatedProperties::default();
179+
let mut surface =
180+
skia_safe::surfaces::raster_n32_premul((target_w as i32, target_h as i32)).unwrap();
181+
{
182+
let canvas = surface.canvas();
183+
icon.paint_content(canvas, &layout, &props, &ctx);
184+
}
185+
186+
// Cleanup so this entry does not leak into other tests sharing the
187+
// process-global asset_cache.
188+
asset_cache().remove(&key);
189+
190+
let snapshot = surface.image_snapshot();
191+
let info = ImageInfo::new(
192+
(target_w as i32, target_h as i32),
193+
ColorType::RGBA8888,
194+
skia_safe::AlphaType::Premul,
195+
None,
196+
);
197+
let mut buf = vec![0u8; (target_w * target_h * 4) as usize];
198+
let ok = snapshot.read_pixels(
199+
&info,
200+
&mut buf,
201+
(target_w * 4) as usize,
202+
skia_safe::IPoint::new(0, 0),
203+
skia_safe::image::CachingHint::Disallow,
204+
);
205+
assert!(ok, "pixel read should succeed");
206+
let has_ink = buf.chunks(4).any(|px| px[3] > 0);
207+
assert!(
208+
has_ink,
209+
"painter must have found and painted the cache entry preload.rs would have \
210+
written under the same key — if the keys disagree, nothing paints"
211+
);
212+
}
213+
}

crates/rustmotion-components/src/video.rs

Lines changed: 93 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -82,8 +82,26 @@ impl Painter for Video {
8282
}
8383
}
8484

85-
let Ok(frame_data) = extract_video_frame(&self.src, source_time, width, height) else {
86-
return;
85+
let frame_data = match extract_video_frame(&self.src, source_time, width, height) {
86+
Ok(data) => data,
87+
Err(e) => {
88+
// Item 3 (issue #167): decoding failures (ffmpeg missing, or
89+
// this specific frame failing) used to be a silent `return`
90+
// — a video component would render entirely blank with no
91+
// trace anywhere. `paint_content` runs once per frame, so
92+
// the warning is deduplicated per `src` via `warn_once_for`
93+
// (the same guard `lib.rs` already uses for exactly this
94+
// per-frame-call-site problem) instead of printing the same
95+
// line a thousand times over a render.
96+
if crate::warn_once_for(&format!("video-frame:{}", self.src)) {
97+
eprintln!(
98+
"Warning: video '{}' could not be decoded: {e}. This component will \
99+
render nothing for the remainder of the video.",
100+
self.src
101+
);
102+
}
103+
return;
104+
}
87105
};
88106
let skia_data = skia_safe::Data::new_copy(&frame_data);
89107
if let Some(img) = skia_safe::Image::from_encoded(skia_data) {
@@ -93,3 +111,76 @@ impl Painter for Video {
93111
}
94112
}
95113
}
114+
115+
#[cfg(test)]
116+
mod tests {
117+
use super::*;
118+
use rustmotion_core::engine::animator::AnimatedProperties;
119+
use rustmotion_core::engine::layout_pass::BoxLayout;
120+
use rustmotion_core::traits::PaintCtx;
121+
122+
fn base_ctx() -> PaintCtx {
123+
PaintCtx {
124+
time: 0.0,
125+
scene_duration: 1.0,
126+
frame_index: 0,
127+
fps: 30,
128+
video_width: 100,
129+
video_height: 100,
130+
stagger_offset: 0.0,
131+
}
132+
}
133+
134+
/// A failed frame extraction (bad src, or no ffmpeg) must be reported,
135+
/// not swallowed. Pre-fix, `paint_content` never calls `warn_once_for`
136+
/// on this path at all, so the slot for this exact src stays unclaimed
137+
/// ("first sighting" == true) forever — this is the observable half of
138+
/// total silence we can assert on without capturing stderr.
139+
#[test]
140+
fn a_failed_frame_extraction_must_claim_its_warn_once_slot() {
141+
let missing_src = std::env::temp_dir().join(format!(
142+
"rustmotion-video-test-missing-{}-{}.mp4",
143+
std::process::id(),
144+
std::time::SystemTime::now()
145+
.duration_since(std::time::UNIX_EPOCH)
146+
.unwrap()
147+
.as_nanos(),
148+
));
149+
let _ = std::fs::remove_file(&missing_src);
150+
let src_str = missing_src.to_str().unwrap().to_string();
151+
152+
let video = Video {
153+
src: src_str.clone(),
154+
trim_start: None,
155+
trim_end: None,
156+
playback_rate: None,
157+
fit: Default::default(),
158+
volume: 1.0,
159+
loop_video: None,
160+
timing: Default::default(),
161+
style: CssStyle::default(),
162+
timeline: Vec::new(),
163+
stagger: None,
164+
};
165+
let layout = BoxLayout {
166+
width: 40.0,
167+
height: 40.0,
168+
..Default::default()
169+
};
170+
let ctx = base_ctx();
171+
let props = AnimatedProperties::default();
172+
let mut surface = skia_safe::surfaces::raster_n32_premul((40, 40)).unwrap();
173+
{
174+
let canvas = surface.canvas();
175+
video.paint_content(canvas, &layout, &props, &ctx);
176+
}
177+
178+
let key = format!("video-frame:{}", src_str);
179+
assert!(
180+
!crate::warn_once_for(&key),
181+
"paint_content must have claimed this warning slot on the failed extraction \
182+
path — it is still unclaimed (first sighting), meaning nothing warned about \
183+
the failure"
184+
);
185+
}
186+
}

0 commit comments

Comments
 (0)