TEXTURES.md: review -- wgpu-hal gate located, a Patch-cancels-Push bug, and a sort-free shape

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

TEXTURES.md

@@ -213,3 +213,123 @@ Open questions a reviewer should weigh in on:
   texture set should already be bounded by what is on screen rather than
   by the whole conversation — worth confirming that invariant holds before
   relying on it to keep atlas/bind-group churn small.
+
+## Review, 2026-09-04
+
+A second pass over the file above against the code, done before anything
+is implemented. Iris's worry going in: a bind group per texture means a
+draw call per image, and she wants this as efficient as it can be.
+
+### What checked out
+
+Every code reference above is accurate as of this commit: the 100,000 /
+1,000 limits, the one-time pipeline, the `rsc_group` rebuild on every
+`Push`/`Set`/`Free`, and the `Patch` exclusion. The device request that
+asks for the three features is `iris/src/default/render.rs:96`, which the
+text above does not name. egui-wgpu and Vello are described correctly.
+
+### The emulator refusal is a wgpu-hal gap, now located
+
+The file guessed "a likely instance-version negotiation gap." It is
+narrower than that and it is in wgpu-hal, not the emulator. wgpu-hal
+28.0.0 (`src/vulkan/adapter.rs:1618`) only queries
+`PhysicalDeviceDescriptorIndexingFeaturesEXT` **when the device advertises
+the `VK_EXT_descriptor_indexing` extension string**. A Vulkan 1.2+ driver
+that has descriptor indexing as core need not list the extension, and
+lavapipe at 1.3 evidently does not, so wgpu never asks and reports the
+features absent, which is why `ash` sees seven `true`s and wgpu sees none.
+The properties query beside it (line 1486) correctly accepts
+`device_api_version >= 1.2 || extension`; the features query does not.
+wgpu-hal 30.0.1 in the local registry has the same asymmetry (lines
+1872 and 2036). Worth an upstream issue, but not a reason to keep the
+design: on real phones the gate that matters is stricter still.
+
+**wgpu's `TEXTURE_BINDING_ARRAY` needs six sub-features, not one**
+(`adapter.rs:160-177`): non-uniform indexing *and* update-after-bind for
+sampled images, storage images and storage buffers, all together, because
+wgpu marks every array-bearing descriptor set update-after-bind. So Arm's
+"the extension is supported on Valhall" is necessary but not sufficient;
+a driver with sampled-image indexing and without storage-buffer
+update-after-bind is refused too. That widens the excluded set beyond
+what the Arm quote suggests and strengthens the conclusion.
+
+### A live bug in the current code, found on the way
+
+`GpuTextures::update` (`core/src/render/texture.rs:33`) implements
+"a patch must not report changed" as `changed = false`, unconditionally,
+which also **cancels a `Push` earlier in the same batch**. That ordering is
+exactly what opening a new atlas page produces: `GlyphAtlas::allocate`
+pushes the page and `insert` patches it in the same frame, so the bind
+group is not rebuilt and the new page's view is not bound until some
+unrelated texture change happens to rebuild it. It is hidden today only
+because the masks path also sets `changed`. The fix is one line
+(`changed |= !matches!(update, Patch)` in spirit); it should go in with
+the redesign since that code is being replaced, and it is recorded here
+so it is not rediscovered.
+
+### In-layer draw order is already undefined
+
+Relevant to any batching redesign: `Primitives::apply_free`
+(`core/src/render/primitive.rs:147`) uses `swap_remove`, so the instance
+order within a layer is permuted whenever anything is freed. Overlap order
+inside one layer is therefore not something the renderer promises today;
+ordering is done with layers. That means grouping a layer's draws by
+texture, or drawing a layer's images after its rects and glyphs, loses
+nothing that currently exists. It should be written down as an invariant
+when the redesign lands, because the new code will depend on it.
+
+### On "a draw call per image"
+
+Two corrections to the worry. First, it is a draw per *distinct texture per
+layer*, not per image primitive: every glyph quad in a layer shares the
+atlas and stays one instanced draw, and a thumbnail atlas would do the same
+for previews. Second, the count is bounded by what is on screen, which I3's
+virtualised transcript already bounds, and a mobile GPU is not draw-call
+bound at tens of draws per frame; egui ships exactly this on Android. What
+does cost is per-frame *bind group creation* and per-frame *sorting*, and
+the current code already creates a `primitive_group` bind group every time
+a layer updates (`render/mod.rs:103`), so one more per new image is not a
+regression in kind.
+
+### Recommended shape (proposal, for Iris to accept or change)
+
+Aimed at the fewest moving parts that need no feature beyond Vulkan 1.0:
+
+1. **Atlas pages become layers of one `texture_2d_array`**, not separate
+   textures. Every page is already `PAGE`x`PAGE` RGBA8, which is the one
+   constraint an array texture imposes. A layer index is an ordinary
+   sampling operand in WGSL and needs no indexing feature, so `GLYPH`
+   (and any future atlased-image primitive) carries a layer instead of a
+   `view_idx` and all of a layer's text stays **one draw**. This answers
+   the open question above about keeping a small binding array: no. Cost
+   of opening a page: recreate the array with one more layer and
+   `copy_texture_to_texture` the old ones, GPU-side, no readback; grow
+   with headroom (double) so it is rare. wgpu's default
+   `max_texture_array_layers` is 256, at 4 MB each, so the cap is memory
+   rather than the API.
+2. **Every standalone image is its own texture with its own bind group**,
+   and its instances live in a **separate per-layer instance list**, not
+   the main one. Then the main instance buffer never contains an image,
+   there is nothing to sort, no handle remapping beyond what
+   `apply_free` already does, and each image is `draw(0..4, k..k+1)` with
+   its bind group set first. Group 2's layout becomes `{atlas array,
+   one image texture, sampler, masks}`; the main draw binds a 1x1 null
+   image in the image slot, each image draw binds its own. One pipeline,
+   one shader, one layout.
+3. **No thumbnail atlas in the first version.** With images on their own
+   textures, the threshold and eviction questions above disappear: an
+   image is freed when the row that owns its `TextureHandle` scrolls out.
+   Add an image atlas only if a measured screen shows enough small images
+   to matter, which a transcript rarely does.
+4. **Drop the three features and the two `max_binding_array_*` limits from
+   `src/default/render.rs`**, and the `UiLimits` counts with them.
+5. **Sampling is `NonFiltering` today** (`render/mod.rs:290,299`), so a
+   downscaled attachment will alias. Either request a filtering sampler
+   for the image slot or downscale on the CPU before upload; decide when
+   the image widget is touched, not as part of this.
+
+What this costs against the file's original recommendation: `Textures`
+needs to know an image from a page (two kinds of handle, or a kind on
+`TextureHandle`), and `Primitives` gets a second instance list per layer.
+What it saves: the sort, the size threshold, the eviction policy, and any
+per-page bind group switch.