Files
galaxy/specs/kevinchevalier/APP-3857/TECH.md
T

100 lines
9.8 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
# APP-3857: Memory-safe animated image rendering in markdown
See `specs/kevinchevalier/APP-3857/PRODUCT.md` for the product spec.
## Problem
Markdown image blocks currently pay for full animated-image decoding even though the markdown renderer does not actually play animations. `RenderableImage` builds a plain `warpui::elements::Image` without `enable_animation_with_start_time`, so the markdown surface effectively shows only the first frame today (`crates/editor/src/render/element/image.rs:13-49`, `crates/warpui_core/src/elements/image.rs:107-172`). However, the shared image asset path still decodes animated GIF/WebP assets into `AnimatedImage { frames: Vec<...> }`, and the resize path then creates resized copies of every decoded frame (`crates/warpui_core/src/image_cache.rs (259-359)`, `crates/warpui_core/src/image_cache.rs (457-511)`). On large assets this produces the multi-GB memory spike seen in `APP-3857`.
## Relevant code
- `crates/editor/src/content/edit.rs:66-97` — resolves markdown image source strings into `AssetSource`.
- `crates/editor/src/content/edit.rs:677-759` — converts `BufferBlockItem::Image` into a laid-out `BlockItem::Image` with default markdown image sizing.
- `crates/editor/src/render/element/image.rs:13-49` — markdown render block that constructs `warpui::elements::Image` from the resolved asset source.
- `crates/warpui_core/src/elements/image.rs:107-172` — animation only occurs when callers explicitly provide a start time; otherwise the first frame is painted.
- `crates/warpui_core/src/image_cache.rs (259-359)``ImageType::try_from_bytes` currently uses `collect_frames()` for animated GIF and animated WebP inputs.
- `crates/warpui_core/src/image_cache.rs (457-511)` — animated resize path duplicates every frame at the target render size.
- `crates/warpui_core/src/image_cache.rs:666-772` — shared image-cache entry point; caches rendered images by asset source hash plus render properties and is where preview-vs-animation behavior can diverge without changing the underlying asset type.
- `crates/warpui_core/src/assets/asset_cache.rs:284-320` — asset cache keys include both `AssetSource` and `TypeId`; this implementation continues to use the existing `ImageType` asset path and keeps full animated assets there.
- `app/src/resource_center/section_views/changelog_section.rs:152` — an existing real app surface that explicitly enables animation and therefore must keep the current full-animation path.
- `crates/warpui_core/src/image_cache_tests.rs (1-261)` and `crates/editor/src/content/markdown_tests.rs (270-337)` — the closest existing test coverage for image decoding/cache behavior and markdown image handling.
## Current state
- Markdown image syntax is parsed as a block item and stored as `BufferBlockItem::Image { alt_text, source }`, then laid out as `BlockItem::Image { asset_source, config, .. }` using the shared editor/render pipeline (`crates/editor/src/content/text.rs:302-373`, `crates/editor/src/content/edit.rs:677-759`).
- The markdown render element constructs a generic `warpui::elements::Image` with `.contain()` and no animation start time (`crates/editor/src/render/element/image.rs:42-49`).
- The `Image` element will animate only if a caller explicitly opts in via `enable_animation_with_start_time`; otherwise it repeatedly paints frame 0 (`crates/warpui_core/src/elements/image.rs:107-172`).
- Despite that static markdown UX, the shared asset loader eagerly decodes animated GIF/WebP files into an in-memory `AnimatedImage` containing all RGBA frames (`crates/warpui_core/src/image_cache.rs (259-359)`).
- `ImageCache::image` then resizes animated assets by resizing every frame, which creates a second full set of frame buffers at markdown display size (`crates/warpui_core/src/image_cache.rs (457-511)`, `crates/warpui_core/src/image_cache.rs:666-772`).
- This is why the current surface is especially wasteful: markdown gets only a first-frame preview, but pays the memory cost of full animation decode plus resized-frame duplication.
## Proposed changes
- Add an explicit animated-image behavior to `warpui::elements::Image`, with two modes:
- `FullAnimation` — current behavior for surfaces that intentionally animate.
- `FirstFramePreview` — render animated sources as a static first-frame image.
- Keep `FullAnimation` as the default so existing animated callers continue to work unchanged. Markdown will opt into `FirstFramePreview` at the call site.
- Keep a single `ImageType` asset path in `crates/warpui_core/src/image_cache.rs`. Animated GIF/WebP assets will continue to decode into `AnimatedImage` in the asset cache, but `ImageCache::image` will materialize `FirstFramePreview` requests as a static first-frame render result instead of resizing every decoded animation frame.
- Thread the new behavior into `ImageCache::image` so the rendered-image cache chooses between a static preview and a full animation from the same underlying `ImageType` asset.
- Replace the current rendered-image cache key shape with a struct that includes, at minimum:
- asset source hash
- target bounds
- animated-image behavior
This avoids collisions where the same source and bounds are requested once as a full animation and once as a static preview.
- Update `crates/editor/src/render/element/image.rs` so markdown image blocks always construct `Image::new(asset_source).contain().first_frame_preview()` (or equivalent builder naming). No markdown storage, export, or copy behavior changes are needed.
- Leave explicit animated callers unchanged. `app/src/resource_center/section_views/changelog_section.rs:152` should continue to opt into animation, and the `warpui` animated-image example should still exercise the full-animation path.
- Do not implement a bounded frame-buffer animation system in this ticket. That is the browser / media-viewer style solution, but it is unnecessary for this markdown surface because markdown does not animate today. The simpler preview-only split matches the current product behavior and removes the blow-up at the source.
- This mirrors common approaches elsewhere:
- browser/media stacks like Chromium separate decode from caching and leave room for smarter bounded frame caches
- native image libraries often provide either a rolling frame buffer for true animation (for example Gifu) or a first-frame-only preview mode for list/thumbnail surfaces (for example Kingfisher)
## End-to-end flow
```mermaid
graph TD
A[Markdown ![](...)] --> B[BufferBlockItem::Image]
B --> C[LayoutTask::Image resolves AssetSource]
C --> D[BlockItem::Image]
D --> E[RenderableImage]
E --> F[warpui::elements::Image with FirstFramePreview]
F --> G[ImageCache preview path]
G --> H[AssetCache.load_asset<ImageType>]
H --> I[Decode AnimatedImage once]
I --> J[ImageCache extracts and resizes only frame 0]
J --> K[Paint as static markdown image]
L[Changelog / explicit animated surface] --> M[Image.enable_animation_with_start_time]
M --> N[ImageCache full-animation path]
N --> O[AssetCache.load_asset<ImageType>]
O --> P[AnimatedImage]
```
## Risks and mitigations
- **Risk: preview/full cache collisions.**
- Mitigation: include animated-image behavior in the rendered-image cache key rather than relying on `AssetSource + bounds` only.
- **Risk: the asset cache still holds full animated frame data.**
- Mitigation: acceptable for this version of the fix. The user-facing memory win comes from avoiding resized copies of every frame in `ImageCache`, and we add tests that preview requests still render static output while the asset cache keeps the full animated asset. If this is still too expensive in practice, a follow-up can teach `AssetCache` to cache raw bytes or accept decode parameters.
- **Risk: accidental regression for existing animated surfaces.**
- Mitigation: keep `FullAnimation` as the default, make preview behavior opt-in at the markdown call site, and add tests that the explicit animated path still produces `Image::Animated`.
- **Risk: very large single-frame assets remain expensive.**
- Mitigation: acceptable for this ticket because the current failure is multi-frame explosion. Follow-up work can add explicit decode limits or dimension guards if needed.
- **Risk: first-frame choice differs slightly from a browsers composited “current frame” semantics for malformed files.**
- Mitigation: the product spec only requires a stable first visible frame preview, not browser-perfect animation fidelity.
## Testing and validation
- Add `warpui_core` unit tests with small animated GIF and animated WebP fixtures:
- preview policy returns `Image::Static`
- full-animation policy still returns `Image::Animated`
- preview policy does not regress static formats
- Add an image-cache test that preview and full-animation requests for the same source/bounds do not collide in the rendered-image cache.
- Add/extend markdown image tests only where needed to confirm markdown export and serialization still preserve the authored `![](...)` reference.
- Manually validate against the Petra repro markdown from `APP-3857`:
- open the document
- scroll through it
- confirm the old multi-GB spike is gone
- confirm animated image sources render as stable first-frame previews
- Run the usual repo validation before implementation is considered done:
- `cargo fmt`
- targeted Rust tests for the touched editor / warpui_core modules
- wasm build check for this repo, since image/rendering code is shared across desktop and wasm-adjacent targets
## Follow-ups
- If product later wants real markdown animation, add a bounded frame-buffer animation path instead of reusing the eager all-frames asset.
- Add explicit decode limits / pixel-budget guards for pathological single-frame assets if the Petra fix reveals a second-order issue there.
- Consider reusing the preview-only animated-image behavior in any other list, thumbnail, or markdown-adjacent surfaces that currently do not need full animation.