From 341b7a5922cb304edf9eb63bcf7fdd757a62b171 Mon Sep 17 00:00:00 2001 From: iris <2+iris@noreply.localhost> Date: Tue, 8 Sep 2026 14:27:09 -0400 Subject: [PATCH] iris: a device change re-uploads its textures, and a mark is one texture per shape The bench APK panicked on frame 1 on the emulator: iris panic at iris/core/src/render/texture.rs:461:22: texture slot 89 is not a live standalone image: None widget::mark called Textures::add per widget, so a folded card per tool call meant a standalone image, a bind group and a draw call each -- hundreds of copies of three pictures. Textures::reset, which the Android surface-rebuild path calls for a genuinely new renderer, then threw the slot numbering away with the pixels, leaving every one of those live handles naming a slot nothing recognised. Its doc had said the only standalone image in the workspace was tabs-ui's, "confirmed by grep" -- true when written, false the moment mark existed. Textures::reupload replaces reset: queue every slot for upload again in slot order, empty slots included, so the new device gets the same slot numbering and a handle a widget has been holding still names its own texture. The glyph atlas is no longer cleared on that path either, so an app switch stops re-rasterising every glyph on screen. Textures::shared(key, make) is one texture per description, keyed by a SharedTextureKey the caller packs exactly rather than hashes. mark keys on direction and colour: three mark textures for the screen, not one a card. And the devlog can finally show a panic. After a crash, Dev Updater's query starts the app process for the provider alone, so no activity ran, so set_crash_dir never replayed the panic hook's file -- the Runtime tab held one line, the provider announcing itself. DevLogProvider.nativeReady takes the files directory and does the replay from onCreate; the hook also saves the dying run's last 80 lines beside the panic, read through a new non-blocking LogRing::try_tail_text so a panic holding the ring's lock cannot deadlock the hook. Verified on this checkout's emulator: opens clean, survives 33 full-screen scrolls back through the fixture, image_bind_group_creates_prev=1; a real panic replays into the next launch, and a hand-written last-panic.txt replays in a process started by a provider query with no activity. Co-Authored-By: Claude Opus 5 --- client-core/src/log_ring.rs | 55 ++++++ docs/IRIS.md | 52 +++++ docs/RUST.md | 92 +++++++++ .../dev/iris/android/demo/DevLogProvider.java | 11 +- iris/android-app/src/app_log.rs | 74 ++++++- iris/android-app/src/devlog.rs | 33 +++- iris/core/src/primitive/texture.rs | 186 ++++++++++++++++-- iris/src/android/view.rs | 38 ++-- iris/src/widget/mark.rs | 66 ++++++- 9 files changed, 550 insertions(+), 57 deletions(-) diff --git a/client-core/src/log_ring.rs b/client-core/src/log_ring.rs index 51ae7fb..acdd662 100644 --- a/client-core/src/log_ring.rs +++ b/client-core/src/log_ring.rs @@ -278,6 +278,37 @@ impl LogRing { format!("{omitted} earlier lines omitted; full log in Dev Updater's Runtime tab\n{tail}") } + /// The newest `max_lines` lines, formatted, or `None` if the ring is + /// locked at this instant. + /// + /// For the one caller that must not block: **the panic hook**. A panic + /// raised while this ring's own lock was held -- an allocation failing + /// inside [`Self::push`], an assertion in a `log::Log` on the way here + /// -- would deadlock the hook against the thread that is panicking, + /// and the process would hang instead of aborting, with nothing + /// written anywhere. Losing the context lines is the right trade + /// against that, and `None` says which happened rather than looking + /// like an empty log. + pub fn try_tail_text(&self, max_lines: usize) -> Option { + let guard = match self.0.try_lock() { + Ok(guard) => guard, + // A poisoned lock is uncontended, so its contents are still + // readable -- the same judgement as `with`. + Err(std::sync::TryLockError::Poisoned(poisoned)) => poisoned.into_inner(), + Err(std::sync::TryLockError::WouldBlock) => return None, + }; + let lines = &guard.lines; + let from = lines.len().saturating_sub(max_lines); + Some( + lines + .iter() + .skip(from) + .map(LogLine::format) + .collect::>() + .join("\n"), + ) + } + /// One line for a diagnostics pane: how much is held, how much was /// dropped, and when the last line arrived. "no lines yet" is its own /// wording rather than a count of zero with a made-up time, because @@ -587,6 +618,30 @@ mod tests { assert!(rest.last().unwrap().ends_with("line 199")); } + #[test] + fn try_tail_text_gives_the_newest_lines_with_no_header() { + let ring = LogRing::new(1000, 1 << 20); + fill(&ring, 200); + let tail = ring.try_tail_text(80).expect("nothing holds the lock"); + let lines: Vec<&str> = tail.lines().collect(); + assert_eq!(lines.len(), 80, "the cap, and no header: this is a file"); + assert!(lines[0].ends_with("line 120"), "{}", lines[0]); + assert!(lines[79].ends_with("line 199"), "{}", lines[79]); + } + + /// The whole point of the `try_`: the panic hook calls this from a + /// thread that may already hold the ring's lock, and a blocking read + /// there would hang the process instead of aborting it. + #[test] + fn try_tail_text_answers_none_rather_than_blocking_on_a_held_lock() { + let ring = LogRing::new(10, 1 << 20); + fill(&ring, 3); + let held = ring.0.lock().expect("fresh ring"); + assert_eq!(ring.try_tail_text(80), None); + drop(held); + assert!(ring.try_tail_text(80).is_some()); + } + #[test] fn an_empty_ring_says_so_rather_than_reporting_a_time() { let ring = LogRing::with_defaults(); diff --git a/docs/IRIS.md b/docs/IRIS.md index 34625c3..0901958 100644 --- a/docs/IRIS.md +++ b/docs/IRIS.md @@ -1370,3 +1370,55 @@ worth knowing because it changes what a build of the Android app *is*. both the desktop and the phone -- only the directory differs. `desktop-app --ca` is now the override for a link that carried no CA rather than a required flag. + +## 2026-09-08: a new GPU device re-uploads its textures instead of forgetting them, and a mark is one texture per shape + +Two defects with one cause: **`widget::mark` built a texture per widget**, +so a transcript screen had one 48x48 standalone image, one bind group and +one draw call *per folded card* rather than one per picture -- and the +Android surface-rebuild path assumed no long-lived widget held a texture +handle at all. + +- **`Textures::reset` is gone; `Textures::reupload` replaces it.** A new + GPU device holds none of the old one's textures, but this side still + holds their pixels, so the answer is to queue every slot for upload + again in slot order (empty slots included, as `PushFree`, so the + indices after a hole still land where they were) rather than to throw + the slot numbering away. Resetting left every live `TextureHandle` + naming a slot nothing recognised: the first frame after the emulator's + Vulkan-to-GLES fallback panicked with *"texture slot 89 is not a live + standalone image: None"*, before anything had been touched. +- **The glyph atlas is no longer cleared on that path either**, which + falls out of the same change: its pages are slots here and their pixels + are on this side, so re-uploading restores exactly the atlas that was + there. An app switch no longer re-rasterises every glyph on screen. +- **`Textures::shared(key, make)`** (new): the one texture for a + description, built on the first ask and handed out again after, keyed + by a `SharedTextureKey { owner, id }` the caller packs *exactly* rather + than hashes. The map holds its own reference, so a shared slot is never + freed and never recycled under a widget still drawing it. `mark()` is + its first caller: three marks now exist for the whole transcript screen + (open, closed, collapse) instead of one per card, and the rasterising is + paid once. + +## 2026-09-08: an app's own log survives the process that wrote it + +`devlog`'s provider could only ever show the run that was still up. After +a crash, Dev Updater's query starts the app process **for the provider +alone** -- no activity runs, so `MainActivity.nativeSetFilesDir` never +fired and the panic hook's file was never replayed. The Runtime tab +therefore showed one line, `iris devlog: serving this app's log at ...`, +which is exactly the run nobody needs. + +- **`DevLogProvider.nativeReady` now takes the files directory too**, and + `app_log::set_crash_dir` is called from whichever of the provider and + the activity runs first (it deletes the file, so the second says + nothing). +- **The panic hook saves context, not just the panic**: the dying run's + last 80 log lines go into the file with it, and are replayed into the + new run's ring ahead of the panic line, so the Runtime tab reads + chronologically -- what the app was doing, then what killed it, then + this run. They are read with a new non-blocking + `LogRing::try_tail_text`, because a panic raised while the ring's own + lock was held would otherwise deadlock the hook and hang the process + instead of aborting it. diff --git a/docs/RUST.md b/docs/RUST.md index 3ee843f..0cb6202 100644 --- a/docs/RUST.md +++ b/docs/RUST.md @@ -8064,3 +8064,95 @@ states the refresh it ran at. **Not yet confirmed on the phone** -- that needs a build in her hands, and the emulator cannot answer it (it is a 60Hz GLES rig, so the defect is invisible there by construction). + +## Iris's phone report, 2026-09-08 evening: "crashes as soon as I scroll up" and a devlog that showed one line + +Her two sentences: the bench crashes shortly after she scrolls back from +where it opens, and Dev Updater's Runtime tab held only `iris devlog: +serving this app's log at content://dev.iris.android.demo.bench.devlog`. + +### The devlog could only ever show the run that was still up + +**Fixed.** After a crash, Dev Updater's query starts the app process **for +the provider alone**: no activity runs, so +`MainActivity.nativeSetFilesDir` never fired, so `app_log::set_crash_dir` +never replayed the line the panic hook had written to +`files/last-panic.txt`. The one line she saw is the provider announcing +itself in a process that had just been started to answer her. + +`DevLogProvider.nativeReady` takes the files directory now and calls +`set_crash_dir` from `onCreate`; whichever of the provider and the +activity runs first does the replay, and the file is deleted before it is +replayed, so a replay that itself panicked cannot make a loop. The hook +also saves the dying run's last 80 log lines beside the panic (a new +non-blocking `LogRing::try_tail_text`, so a panic raised while the ring's +lock was held cannot deadlock the hook), replayed into the new ring ahead +of the panic line under the target `previous_run`. + +Verified on this checkout's emulator both ways: a real panic replayed into +the next launch, and a hand-written `files/last-panic.txt` replayed by a +process started by `content query` alone with **no activity** (the query +is refused for the `shell` uid, which does not hold +`dev.updater.permission.READ_DEVLOG` -- the provider's `onCreate` still +runs, which is the half under test). + +### The crash: one texture per mark, and a device change that forgot them + +The bench APK **panicked on frame 1** on this checkout's emulator, which +is where this was found rather than by reasoning from her report: + + iris panic at iris/core/src/render/texture.rs:461:22: + texture slot 89 is not a live standalone image: None + +`widget::mark` (added the same day) called `Textures::add` **per widget**, +so a folded card per tool call meant a standalone image, a bind group and +a draw call each -- slot 89 was one of hundreds of copies of three +pictures. `Textures::reset`, which the Android surface-rebuild path calls +when it builds a genuinely new renderer, then threw the slot *numbering* +away along with the pixels, leaving every one of those live handles naming +a slot nothing recognised. Its own doc had said the only standalone image +in the workspace was `tabs-ui`'s, "confirmed by grep" -- true when it was +written, false the moment `mark` existed. **A rule stated on one member of +a set** (CODE_RULES): the grep was the invariant, and nothing re-ran it. + +Two fixes, both in `iris-core`: + +- **`Textures::reupload` replaces `reset`**: queue every slot for upload + again in slot order, empty slots included (they cross as `PushFree`, so + the indices after a hole still land where they were). This side holds + the images, so the new device gets the same slot numbering and every + handle a widget is holding still names its own texture. The glyph atlas + is no longer cleared on that path either -- its pages are slots here, so + they come back with everything else, and an app switch stops costing a + re-rasterisation of every glyph on screen. +- **`Textures::shared(key, make)`**: one texture per description. `mark` + keys on direction and colour, packed exactly into a + `SharedTextureKey { owner, id }` rather than hashed, so the screen has + three mark textures rather than one per card and the rasterising is paid + once. The map holds its own reference, so a shared slot is never freed + and never recycled under a widget still drawing it. + +After the fix the emulator opens clean and survives 33 full-screen scroll +gestures back through the fixture, with +`image_bind_group_creates_prev=1`. + +**Whether this is the crash on *her* phone is not yet confirmed** -- her +build is release, its startup does not take the GLES fallback that made +the emulator rebuild its renderer, and the panic she saw was never +recorded anywhere. It is the crash this code had; the devlog fix above is +what will say whether it was hers. Ask for the Runtime tab after the next +bench APK. + +### Open: why `mark` exists at all (Iris asked, 2026-09-08) + +Her question, mid-fix: "why does mark exist? The font should be working if +it's working for compose and nerd fonts are bundled." The Compose app +draws its chevron from **its own committed Nerd Fonts subset** +(`app/build-icon-font.sh`, `NerdIcons.kt`); iris bundles no font at all +since 2026-09-07, and `tool.rs` was using bare geometric codepoints +(U+25B8/25BE/25B4) out of whatever system face resolved -- which her phone +had none for. So the two apps were never doing the same thing, and the +2026-09-07 note that "iris had no equivalent icon font to keep" is what +left the gap. The alternative to `mark` is to bundle the same ~100-glyph +subset in iris and take icons from it, which would serve every future icon +rather than one triangle. Not done: it is hers to choose. diff --git a/iris/android-app/app/src/main/java/dev/iris/android/demo/DevLogProvider.java b/iris/android-app/app/src/main/java/dev/iris/android/demo/DevLogProvider.java index 9e34841..afa1557 100644 --- a/iris/android-app/app/src/main/java/dev/iris/android/demo/DevLogProvider.java +++ b/iris/android-app/app/src/main/java/dev/iris/android/demo/DevLogProvider.java @@ -74,8 +74,15 @@ public final class DevLogProvider extends ContentProvider { * the diagnostics pane can name somewhere a reader can actually query * -- and so "declared but never created" is a state it can say. Only * the provider knows it was instantiated; Android creates one lazily. + * + *

The files directory goes with it because this is usually the + * only thing running: after the app has died, Dev Updater's query + * starts the process for the provider alone, with no activity, so + * {@code MainActivity.nativeSetFilesDir} is never called and the line + * the panic hook left on disk is never replayed into the ring. That is + * exactly the run whose log somebody wants. */ - private static native void nativeReady(String authority); + private static native void nativeReady(String authority, String filesDir); @Override public boolean onCreate() { @@ -87,7 +94,7 @@ public final class DevLogProvider extends ContentProvider { matcher = new UriMatcher(UriMatcher.NO_MATCH); matcher.addURI(authority, "lines", LINES); matcher.addURI(authority, "status", STATUS); - nativeReady(authority); + nativeReady(authority, getContext().getFilesDir().getAbsolutePath()); return true; } diff --git a/iris/android-app/src/app_log.rs b/iris/android-app/src/app_log.rs index 7a41589..e0ed7c6 100644 --- a/iris/android-app/src/app_log.rs +++ b/iris/android-app/src/app_log.rs @@ -71,11 +71,29 @@ pub fn diagnostics_line() -> String { format!("{}\n{where_to_read}", ring().summary()) } -/// Where the panic hook leaves its one line, under the app's private +/// Where the panic hook leaves its report, under the app's private /// directory. Read back and dropped by [`set_crash_dir`] on the next /// start. const CRASH_FILE: &str = "last-panic.txt"; +/// How many of the dying run's own log lines the panic hook saves with +/// the panic, and [`set_crash_dir`] replays. +/// +/// The panic's message and location say *what* broke; these say what the +/// app was doing on the way there, which is the half that is otherwise +/// unrecoverable -- the ring is memory only, so an abort takes every line +/// before the panic with it. Bounded rather than the whole ring because +/// this is written by a hook on a process that is about to die, and +/// because the replay pushes each line into the new run's ring, where an +/// unbounded paste would evict the run that is actually being watched. +const CRASH_CONTEXT_LINES: usize = 80; + +/// The target the replayed context lines carry, so a reader can tell a +/// line from the run that died from one this run wrote. They keep their +/// original timestamp and level inside the text, which is why the level +/// they are re-pushed at is not meaningful and the target has to be. +const PREVIOUS_RUN_TARGET: &str = "previous_run"; + static CRASH_PATH: std::sync::OnceLock = std::sync::OnceLock::new(); /// Installs a `log`-level panic hook, so a panic's message and location @@ -105,26 +123,64 @@ fn install_panic_hook() { let line = format!("iris panic at {where_at}: {message}"); log::error!("{line}"); if let Some(path) = CRASH_PATH.get() { + // The panic line first, then what the app was doing before + // it: one file, split again on that first newline by + // `set_crash_dir`. + let context = ring() + .try_tail_text(CRASH_CONTEXT_LINES) + // Said rather than left empty, so "the ring was locked as + // we died" cannot be read as "nothing had been logged". + .unwrap_or_else(|| { + "(the log ring was locked as this run died; no context)".to_string() + }); // Best effort by design: a panic is already the failure, and // failing to record it must not become a second one. - let _ = std::fs::write(path, &line); + let _ = std::fs::write(path, format!("{line}\n{context}")); } previous(info); })); } -/// Tells the panic hook where to leave its line, and replays the line a -/// previous run left there into the ring before deleting it. +/// Tells the panic hook where to leave its report, and replays the report +/// a previous run left there into the ring before deleting it. /// -/// Called from `nativeSetFilesDir`, which is the first moment the app's -/// private directory is known. The replay is at `error` level and says -/// it is from the previous run, so a crash loop shows the reason it is -/// looping in the Runtime tab of the run that is still up. +/// Called from **both** `MainActivity.nativeSetFilesDir` and +/// `DevLogProvider.nativeReady` -- whichever of the two runs first in +/// this process, since after a crash Dev Updater's query starts the +/// process for the provider alone and no activity ever runs. Safe to call +/// twice: the file is gone after the first, so the second finds nothing +/// and says nothing. The panic itself is replayed at `error` level and +/// says it is from the previous run, so a crash loop shows the reason it +/// is looping in the Runtime tab of the run that is still up. pub fn set_crash_dir(dir: &std::path::Path) { let path = dir.join(CRASH_FILE); if let Ok(previous) = std::fs::read_to_string(&path) { - log::error!("iris app log: the previous run died -- {}", previous.trim()); + // Delete before replaying rather than after: a replay that itself + // panicked would otherwise leave the file to be replayed again on + // every start, and a crash loop nothing can get out of is worse + // than one report lost. let _ = std::fs::remove_file(&path); + replay_crash(&previous); } let _ = CRASH_PATH.set(path); } + +/// Puts a previous run's report back in the ring: its context lines in +/// the order they happened, then the panic itself. +/// +/// Chronological, so the Runtime tab reads as one story -- the lines that +/// led to the crash, then the crash, then this run. The context goes in +/// through `LogRing::push` rather than through `log::info!` so it is not +/// stamped with this run's clock: each line already carries the time and +/// level it was written at, and [`PREVIOUS_RUN_TARGET`] is what says +/// whose run it was. +fn replay_crash(report: &str) { + let (panic_line, context) = report.split_once('\n').unwrap_or((report, "")); + for line in context.lines().filter(|line| !line.is_empty()) { + ring().push(log::Level::Info, PREVIOUS_RUN_TARGET, line.to_string()); + } + log::error!( + "iris app log: the previous run died -- {}", + panic_line.trim() + ); +} diff --git a/iris/android-app/src/devlog.rs b/iris/android-app/src/devlog.rs index 82a5bc6..6d6a608 100644 --- a/iris/android-app/src/devlog.rs +++ b/iris/android-app/src/devlog.rs @@ -50,7 +50,17 @@ pub fn authority() -> Option<&'static str> { } /// `DevLogProvider.nativeReady` -- the provider announcing the authority -/// it registered under, from its own `onCreate`. +/// it registered under and the app's private directory, from its own +/// `onCreate`. +/// +/// The directory is taken here as well as in +/// `MainActivity.nativeSetFilesDir` because **the provider is often the +/// only thing running**: once the app has died, Dev Updater's query +/// starts the process for the provider alone, so no activity ever runs +/// and the panic hook's file would never be replayed into the ring. That +/// is precisely the run whose log is being asked for. Whichever of the +/// two arrives first does the replay; `set_crash_dir` deletes the file, +/// so the second finds nothing and says nothing. /// /// # Safety /// Called by the JVM with the arguments its `native` declaration names. @@ -59,18 +69,31 @@ pub extern "system" fn Java_dev_iris_android_demo_DevLogProvider_nativeReady( mut env: JNIEnv, _class: JClass, authority: JString, + files_dir: JString, ) { - if authority.is_null() { - return; + // Before the authority line, so the previous run's death is above the + // line announcing this one rather than buried under it. + #[cfg(feature = "transcript-screen")] + if let Some(dir) = string_arg(&mut env, &files_dir) { + crate::app_log::set_crash_dir(std::path::Path::new(&dir)); } - let Ok(authority) = env.get_string(&authority) else { + #[cfg(not(feature = "transcript-screen"))] + let _ = &files_dir; + let Some(authority) = string_arg(&mut env, &authority) else { return; }; - let authority: String = authority.into(); log::info!("iris devlog: serving this app's log at content://{authority}"); let _ = AUTHORITY.set(authority); } +/// One `String` argument, or `None` for a null or unreadable one. +fn string_arg(env: &mut JNIEnv, value: &JString) -> Option { + if value.is_null() { + return None; + } + env.get_string(value).ok().map(Into::into) +} + /// `DevLogProvider.nativeStatus` -- `held`, `dropped`, `newest_seq`, as /// three strings. /// diff --git a/iris/core/src/primitive/texture.rs b/iris/core/src/primitive/texture.rs index ab3ea10..04cd363 100644 --- a/iris/core/src/primitive/texture.rs +++ b/iris/core/src/primitive/texture.rs @@ -1,6 +1,7 @@ use crate::util::{RefCounter, Vec2}; use image::{DynamicImage, GenericImageView}; use std::{ + collections::HashMap, ops::Index, sync::mpsc::{Receiver, Sender, channel}, }; @@ -21,6 +22,16 @@ pub enum TextureKind { }, } +/// What a [`Textures::shared`] texture is a picture of -- exactly, not by +/// hash: `owner` names the widget kind whose description it is, and `id` +/// packs that description's own fields, so two owners cannot collide and +/// a debugger shows which picture a slot holds. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +pub struct SharedTextureKey { + pub owner: &'static str, + pub id: u64, +} + #[derive(Debug, Clone)] pub struct TextureHandle { slot: u32, @@ -35,6 +46,14 @@ pub struct TextureHandle { pub struct Textures { free: Vec, images: Vec>, + /// What each slot is, kept beside the image so a slot can be pushed + /// again without the handle that knows -- see [`Textures::reupload`]. + kinds: Vec, + /// Textures built from a description rather than from a file, one per + /// distinct description: see [`Textures::shared`]. The map holds a + /// reference of its own, so a shared texture outlives every widget + /// drawing it and its slot is never recycled underneath one. + shared: HashMap, /// Next layer to hand out to an atlas page. Pages are never freed (no /// atlas eviction), so this only grows and `free` never holds one. next_page_layer: u32, @@ -77,6 +96,8 @@ impl Textures { Self { free: Vec::new(), images: Vec::new(), + kinds: Vec::new(), + shared: HashMap::new(), next_page_layer: 0, updates: Vec::new(), send, @@ -119,16 +140,46 @@ impl Textures { fn push(&mut self, kind: TextureKind, image: DynamicImage) -> u32 { if let Some(i) = self.free.pop() { self.images[i as usize] = Some(image); + self.kinds[i as usize] = kind; self.updates.push(Update::Set(kind, i)); i } else { let i = self.images.len() as u32; self.images.push(Some(image)); + self.kinds.push(kind); self.updates.push(Update::Push(kind, i)); i } } + /// The one texture for `key`, building it on the first ask and handing + /// out a further reference to it every time after. + /// + /// **Why this exists**: a texture rasterised from a *description* -- + /// `widget::mark`'s triangle, from a direction and a colour -- has as + /// many copies as there are widgets asking for it, and each copy is + /// its own GPU texture, its own bind group and its own draw call. A + /// transcript screen with a folded card per tool call built one per + /// card: hundreds of 48x48 textures of three distinct pictures, + /// created and freed again as rows recycled. `make` is not called when + /// the key is already known, so the rasterising is paid once too. + /// + /// The map keeps its own reference for the life of the `Textures`, so + /// a shared slot is never freed and never reused for something else -- + /// which is what makes a handle held by a long-lived widget safe. + pub fn shared( + &mut self, + key: SharedTextureKey, + make: impl FnOnce() -> DynamicImage, + ) -> TextureHandle { + if let Some(handle) = self.shared.get(&key) { + return handle.clone(); + } + let handle = self.add(make()); + self.shared.insert(key, handle.clone()); + handle + } + /// The stored image for a handle, to be written into before `patch`. pub fn image_mut(&mut self, handle: &TextureHandle) -> &mut DynamicImage { self.images[handle.slot as usize] @@ -141,25 +192,35 @@ impl Textures { self.updates.push(Update::Patch(handle.slot, rect)); } - /// Forget every image, page and pending update -- what a genuinely new - /// GPU device needs alongside [`crate::render::atlas::GlyphAtlas:: - /// clear`], which this module's own doc references: every slot number - /// and every queued [`Update`] here describes the *old* device's - /// textures (an `Update::Push`/`Update::Patch` already drained into a - /// renderer that no longer exists is gone for good, and a fresh - /// `UiRenderNode`'s own texture manager starts with none of them - /// applied), so nothing is lost by starting this bookkeeping over too. - /// Any `TextureHandle` a caller still holds across the reset (none in - /// the transcript screen this reset is wired up for today -- confirmed - /// by grep, the only standalone (non-atlas) image anywhere in this - /// workspace is `iris/widget/image.rs`'s `Image`, used by the separate - /// `tabs-ui` example) is left pointing at a slot this instance no - /// longer recognises and needs reinserting via `add`/`add_page` again - /// -- the same pre-existing gap a renderer restart already left for - /// such a handle before this method existed, just named rather than - /// silent now. - pub fn reset(&mut self) { - *self = Self::new(); + /// Queue every live slot for upload again, in slot order -- what a + /// genuinely new GPU device needs, in place of forgetting everything. + /// + /// A new device starts with no textures, and the renderer-side mirror + /// of these slots (`render::texture::GpuTextures`) starts empty with + /// it. What it must not do is start empty while the handles widgets + /// are still holding name slots by *index*: `Textures::reset` used to + /// throw this bookkeeping away, which left every live `TextureHandle` + /// -- one per `widget::mark`, hundreds on a transcript screen -- + /// pointing at a slot nothing recognised, and the first frame after an + /// Android surface rebuild panicked in `image_bind_group` ("texture + /// slot 89 is not a live standalone image: None"). Re-uploading + /// instead keeps every index meaning what it meant, because this side + /// still holds the images: the slot list is rebuilt identically, + /// including the empty slots, which go across as `PushFree` so the + /// ones after them still land where they were. + /// + /// The glyph atlas comes back with it and is deliberately *not* + /// cleared any more: its pages are slots here, this side holds their + /// pixels, and re-uploading them restores exactly the atlas that was + /// there -- so an app switch no longer costs a re-rasterisation of + /// every glyph on screen either. + /// + /// Pending updates are dropped rather than kept: each is either a push + /// or a patch of a slot this replays in full. + pub fn reupload(&mut self) { + self.updates.clear(); + self.updates + .extend((0..self.images.len() as u32).map(|i| Update::Push(self.kinds[i as usize], i))); } pub fn free(&mut self) { @@ -245,3 +306,90 @@ impl Default for Textures { Self::new() } } + +#[cfg(test)] +mod tests { + use super::*; + use image::RgbaImage; + + fn image(n: u32) -> DynamicImage { + RgbaImage::new(n, n).into() + } + + fn key(id: u64) -> SharedTextureKey { + SharedTextureKey { owner: "test", id } + } + + /// What `widget::mark` needs: one texture per description, however + /// many widgets ask for it, and a different description is a + /// different texture. + #[test] + fn a_shared_texture_is_built_once_and_handed_out_again() { + let mut textures = Textures::new(); + let built = std::cell::Cell::new(0); + let make = |textures: &mut Textures, id: u64| { + textures.shared(key(id), || { + built.set(built.get() + 1); + image(4) + }) + }; + let first = make(&mut textures, 1); + let again = make(&mut textures, 1); + let other = make(&mut textures, 2); + assert_eq!(built.get(), 2, "the second ask for key 1 rasterised again"); + assert_eq!(first.image_index(), again.image_index()); + assert_ne!(first.image_index(), other.image_index()); + } + + /// The map's own reference is what keeps a shared slot alive: every + /// widget holding one can go away and the slot must not be recycled, + /// because the next widget to ask gets that same index back. + #[test] + fn a_shared_slot_is_not_freed_when_the_last_widget_drops_it() { + let mut textures = Textures::new(); + let slot = textures.shared(key(1), || image(4)).image_index(); + textures.free(); + let plain = textures.add(image(4)); + assert_ne!( + plain.image_index(), + slot, + "an ordinary texture was handed the shared mark's slot" + ); + } + + /// A new GPU device gets the same slot numbering back, so a handle a + /// widget has been holding all along still names its own texture -- + /// the crash `reupload` replaced `reset` to fix. + #[test] + fn reupload_replays_every_slot_in_order_including_the_empty_ones() { + let mut textures = Textures::new(); + let keep_a = textures.add(image(4)); + let dropped = textures.add(image(4)); + let keep_b = textures.add(image(4)); + let (a, gone, b) = ( + keep_a.image_index(), + dropped.image_index(), + keep_b.image_index(), + ); + drop(dropped); + textures.free(); + // Drain the updates so far, the way a frame does. + assert!(textures.updates().count() > 0); + + textures.reupload(); + let kinds: Vec = textures + .updates() + .map(|u| match u { + TextureUpdate::Push(..) => "push".to_string(), + TextureUpdate::PushFree(..) => "push-free".to_string(), + _ => "other".to_string(), + }) + .collect(); + assert_eq!( + kinds, + ["push", "push-free", "push"], + "slots {a}, {gone} (freed) and {b} must replay in order, so the \ + indices after a hole still land where they were" + ); + } +} diff --git a/iris/src/android/view.rs b/iris/src/android/view.rs index 9ac6397..ebfac1b 100644 --- a/iris/src/android/view.rs +++ b/iris/src/android/view.rs @@ -853,29 +853,33 @@ impl ViewPeer for IrisViewPeer { let content_scale = self.state.android_state().content_scale; match AndroidRenderer::new(window, width as u32, height as u32, content_scale) { Ok(renderer) => { - // A genuinely new renderer means a genuinely new GPU device - // and a fresh, empty glyph atlas -- the CPU-side glyph - // cache (`TextData::atlas`) and the texture bookkeeping it - // is built on (`UiData::textures`) both outlive `renderer` - // itself (they live on `self.rsc`, not on `AndroidRenderer`), - // so without this they would keep pointing at the *old* - // device's now-gone textures -- the app-switch counterpart - // to the keyboard-resize glyph wipe this same function's - // `already_live` branch above already fixed by reusing the - // renderer instead of rebuilding it. One mechanism either - // way: this call only runs on the branch that actually - // builds a new renderer, exactly where invalidation is - // needed, never on the reuse branch, where it would throw - // away perfectly valid GPU state for nothing. + // A genuinely new renderer means a genuinely new GPU + // device, holding none of the textures the old one did -- + // while the CPU side of them (`UiData::textures`, and the + // glyph atlas built on it) lives on `self.rsc` and + // survives. So every slot has to be uploaded again, and + // `Textures::reupload` queues exactly that, in slot order. + // + // It replaces clearing them, which threw away the *slot + // numbering* as well as the pixels: every `TextureHandle` + // a live widget still held -- one per `widget::mark`, so + // one per folded card on the transcript screen -- then + // named a slot nothing recognised, and the next frame + // panicked in `image_bind_group` ("texture slot 89 is not + // a live standalone image: None"). Re-uploading also keeps + // the glyph atlas, so an app switch no longer re-rasterises + // every glyph on screen. This only runs on the branch that + // actually builds a new renderer, never on the reuse + // branch above, where the textures are still on the device + // that holds them. log::info!( - "iris surface: new renderer built ({:?}), clearing glyph atlas: \ + "iris surface: new renderer built ({:?}), re-uploading textures: \ glyphs={} pages={}", renderer.adapter_backend, self.rsc.ui.text.atlas.glyph_count(), self.rsc.ui.text.atlas.page_count(), ); - self.rsc.ui.text.atlas.clear(); - self.rsc.ui.textures.reset(); + self.rsc.ui.textures.reupload(); self.state.android_state_mut().renderer = Some(renderer); self.render(ctx); } diff --git a/iris/src/widget/mark.rs b/iris/src/widget/mark.rs index 17c4b96..b5d76d0 100644 --- a/iris/src/widget/mark.rs +++ b/iris/src/widget/mark.rs @@ -12,18 +12,24 @@ //! Rasterised into iris's ordinary texture path rather than needing a new //! primitive: iris has rects, text and textures, and a triangle is not //! expressible as any number of rects without a staircase edge. One -//! oversampled bitmap per mark is drawn scaled into the box the caller +//! oversampled bitmap per *shape* is drawn scaled into the box the caller //! asks for, so the same texture is correct at any density -- which is //! also why it is built at construction, where the density is not known //! yet, and scaled at draw, where it is. +//! +//! Per shape, not per widget: the bitmap depends only on the direction and +//! the colour, so it goes through [`Textures::shared`] and a screen full of +//! folded cards draws the three marks it actually has rather than one +//! texture, bind group and draw call per card. use crate::prelude::*; use image::{Rgba, RgbaImage}; +use iris_core::SharedTextureKey; /// The bitmap's own size. Generous enough that a 12dp mark at density 3 /// (36px) is still sampling *down*, which is what keeps the diagonal -/// clean; small enough that a handful of them cost nothing (48x48 RGBA is -/// 9 KB, and a card draws one). +/// clean; small enough that the handful the program has cost nothing +/// (48x48 RGBA is 9 KB). const TEXTURE_PX: u32 = 48; /// Subsamples per pixel per axis when measuring how much of a pixel the /// triangle covers. 4x4 is the point where the edge stops looking stepped @@ -53,14 +59,39 @@ impl Widget for Mark { /// A filled triangle `size_dp` across, pointing along `dir`, in `color` -- /// what a row uses to say "this opens" and "this is open". +/// +/// `size_dp` is not part of what is rasterised (the bitmap is scaled at +/// draw), so two marks differing only in size share one texture. pub fn mark(dir: Dir, size_dp: f32, color: UiColor) -> impl WidgetFn { - let image = rasterise(dir, color); move |state| Mark { - handle: state.ui_mut().textures.add(image), + handle: state + .ui_mut() + .textures + .shared(key(dir, color), || rasterise(dir, color).into()), size_dp, } } +/// The mark's whole description, packed exactly: the axis and sign of the +/// direction and the four colour channels, one byte each, so distinct +/// marks are distinct keys and equal ones are equal. +fn key(dir: Dir, color: UiColor) -> SharedTextureKey { + let axis = match dir.axis { + Axis::X => 0u64, + Axis::Y => 1, + }; + let sign = match dir.sign { + Sign::Neg => 0u64, + Sign::Pos => 1, + }; + SharedTextureKey { + owner: "mark", + id: u64::from_le_bytes([ + axis as u8, sign as u8, color.r, color.g, color.b, color.a, 0, 0, + ]), + } +} + /// The triangle, as coverage: for each pixel, how much of it the shape /// covers, measured by subsampling rather than by an analytic edge /// function -- one bitmap is built per mark in the whole program, so the @@ -175,6 +206,31 @@ mod tests { } } + /// Two marks of the same shape share a texture and two of different + /// shapes do not -- the whole of what [`Textures::shared`] is being + /// keyed on, checked here rather than through a widget so it does not + /// need a resource tree. + #[test] + fn a_marks_key_is_its_direction_and_its_colour() { + assert_eq!(key(Dir::UP, UiColor::WHITE), key(Dir::UP, UiColor::WHITE)); + for (name, other) in [ + ("down", Dir::DOWN), + ("left", Dir::LEFT), + ("right", Dir::RIGHT), + ] { + assert_ne!( + key(Dir::UP, UiColor::WHITE), + key(other, UiColor::WHITE), + "{name} shares a key with up" + ); + } + assert_ne!( + key(Dir::UP, UiColor::WHITE), + key(Dir::UP, UiColor::new(255, 255, 255, 128)), + "two colours differing only in alpha share a key" + ); + } + /// A corner of the bitmap is never covered, whichever way the mark /// points -- what says the shape is a triangle rather than a filled /// box, and that the inset in `corners` is keeping its antialiasing