fix(spryte): rebind the sprite units before drawing into a target (#118)

.changeset/spryte-target-feedback.md

@@ -0,0 +1,5 @@
+---
+"spryte": patch
+---
+
+A render target refilled with `draw.into()` after it was drawn stays filled. Its texture could still be bound to a texture unit from the earlier draw (or from its own creation), so the next draw into it was a feedback loop: WebGL rejected it and logged `GL_INVALID_OPERATION ... Feedback loop formed between Framebuffer and active Texture`, and the target stayed blank. Switching to a target now rebinds the 8 sprite units first. The same loop hit frames of shapes only under `blendSpace: "linear"`, whose sRGB framebuffer texture stayed bound after `end()` presented it; it no longer does. Reported in #118.

docs/decisions.md

@@ -155,3 +155,7 @@ Papercut #26: a full-screen vignette drawn as a rect inside `withShader` seemed
 ## An audio stream's stop() is final
 
 From 0.31.1 `audio.stream()` tracks one state (playing, paused or stopped) instead of a `wanted` flag. `stop()` unloads the element, so `resume()` after it does nothing; call `stream()` again to play the track afresh. Before, `resume()` during a `stop(fade)` restarted playback that the pending unload then cut off, and after the unload it armed a retry that could never succeed. Considered: letting `resume()` cancel a pending fade-out (needs the gain restored and the unload timer cancelled, and gives stop two meanings). A blocked `play()` now waits for the context unlocking or the next pointer or key gesture, with one wait armed at most: media elements have their own autoplay rules, so the context can already be running while the element is still blocked.
+
+## spryte rebinds all 8 sprite units when drawing into a target
+
+#118: a target refilled with `draw.into()` every frame and also drawn every frame stayed blank, with a feedback-loop error per draw. The earlier draw left the target's texture bound to a texture unit, and every spryte program samples all 8 sprite units, so the next draw into the target read the texture it wrote. From 0.16 `setTarget` binds the white texture to all 8 units after binding a target's framebuffer, and the linear-light framebuffer's texture no longer stays on unit 0 after it is created or presented. Considered: the issue's proposal, rebinding only the units that `units` records as holding the target. `units` records what sprite batches bound, but uploads, `copy()`, `copies()`, static layers and a target's creation bind units without recording it, and those can run mid-batch, where updating the record would break the pending batch. A full mirror of every unit's binding would touch all of those paths. Rebinding all 8 costs 16 GL calls per `into()`, none on the per-sprite path.

packages/spryte/browser-test/checks.ts

@@ -76,6 +76,14 @@ async function render(scene: (draw: Draw) => void, backend: "webgl2" | "2d" = "w
   return out;
 }
 
+/** readPixels rows are bottom first: flip them to compare with `render`. */
+function flipRows(out: Uint8Array): Uint8Array {
+  const row = W * 4;
+  const flipped = new Uint8Array(out.length);
+  for (let y = 0; y < H; y++) flipped.set(out.subarray((H - 1 - y) * row, (H - y) * row), y * row);
+  return flipped;
+}
+
 function diff(a: Uint8Array, b: Uint8Array): number {
   let n = 0;
   for (let i = 0; i < a.length; i += 4) if (a[i] !== b[i] || a[i + 1] !== b[i + 1] || a[i + 2] !== b[i + 2] || a[i + 3] !== b[i + 3]) n++;
@@ -1794,4 +1802,86 @@ function hostCanvas(): WebGL2RenderingContext {
   }
 }
 
+// A target refilled with into() every frame and drawn every frame: the previous frame's draw left the target's
+// texture on a texture unit, and drawing into it while bound is a feedback loop that WebGL rejects (#118).
+{
+  type Target = ReturnType<Draw["createTarget"]>;
+  const pos1 = new Float32Array([30, 15]);
+  const fillRect = (draw: Draw) => draw.rect(0, 0, 16, 16, "#cffb2d");
+  const fillPolygon = (draw: Draw) => polygon(draw, [0, 0, 16, 0, 16, 16, 0, 16], "#cffb2d");
+  // The image first puts the target on unit 1, which a batch of one texture doesn't rebind.
+  const afterImage = (draw: Draw, t: Target) => {
+    draw.sprite(img, 4, 4);
+    draw.sprite(t, 30, 15, { origin: [0, 0] });
+  };
+  const alone = (draw: Draw) => draw.rect(30, 15, 16, 16, "#cffb2d");
+  const withImage = (draw: Draw) => {
+    draw.sprite(img, 4, 4);
+    alone(draw);
+  };
+  for (const [how, fill, show, direct] of [
+    ["sprite after an image, refilled by a rect", fillRect, afterImage, withImage],
+    ["sprite after an image, refilled by a polygon", fillPolygon, afterImage, withImage],
+    ["sprite, refilled by a polygon", fillPolygon, (draw: Draw, t: Target) => draw.sprite(t, 30, 15, { origin: [0, 0] }), alone],
+    ["sprites, refilled by a polygon", fillPolygon, (draw: Draw, t: Target) => draw.sprites(t, pos1, 1, { origin: [0, 0] }), alone],
+  ] as const) {
+    const frames = async (n: number): Promise<{ pixels: Uint8Array; error: number }> => {
+      const canvas = document.createElement("canvas");
+      canvas.width = W;
+      canvas.height = H;
+      const draw = createRenderer(canvas, { backend: "webgl2", smooth: false });
+      await draw.ready;
+      draw.resize(W, H);
+      const t = draw.createTarget(16, 16);
+      const raw = draw.raw;
+      if (raw.kind !== "webgl2") throw new Error("expected webgl2");
+      let error = 0;
+      for (let i = 0; i < n; i++) {
+        draw.begin("#102030", W / 2, H / 2, 2);
+        draw.into(t, () => fill(draw));
+        show(draw, t);
+        draw.end();
+        error ||= raw.gl.getError();
+      }
+      const pixels = new Uint8Array(W * H * 4);
+      raw.gl.readPixels(0, 0, W, H, raw.gl.RGBA, raw.gl.UNSIGNED_BYTE, pixels);
+      draw.dispose();
+      raw.gl.getExtension("WEBGL_lose_context")?.loseContext();
+      return { pixels, error };
+    };
+    const thrice = await frames(3);
+    const expected = await render(direct);
+    check(`into(t) every frame, drawing t with ${how}: no GL error`, thrice.error === 0, `0x${thrice.error.toString(16)}`);
+    record(`into(t) every frame, drawing t with ${how} == drawn directly`, flipRows(thrice.pixels), expected);
+  }
+}
+
+// The same loop under blendSpace "linear": each frame is drawn into a framebuffer whose texture end() samples to
+// present it. A frame of shapes only (no batch unit rebound) drew into it while it was still bound.
+{
+  const canvas = document.createElement("canvas");
+  canvas.width = W;
+  canvas.height = H;
+  const draw = createRenderer(canvas, { backend: "webgl2", smooth: false, blendSpace: "linear" });
+  await draw.ready;
+  draw.resize(W, H);
+  const raw = draw.raw;
+  let error = 0;
+  const pixel = new Uint8Array(4);
+  if (raw.kind === "webgl2") {
+    for (let i = 0; i < 3; i++) {
+      draw.begin("#102030", W / 2, H / 2, 2);
+      polygon(draw, [0, 0, 16, 0, 16, 16, 0, 16], "#cffb2d");
+      draw.end();
+      error ||= raw.gl.getError();
+    }
+    // Inside the polygon: 8, 8 in the view is 16, 16 on the canvas, read from the bottom row up.
+    raw.gl.readPixels(16, H - 17, 1, 1, raw.gl.RGBA, raw.gl.UNSIGNED_BYTE, pixel);
+  }
+  draw.dispose();
+  if (raw.kind === "webgl2") raw.gl.getExtension("WEBGL_lose_context")?.loseContext();
+  check("frames of shapes only under blendSpace linear: no GL error", error === 0, `0x${error.toString(16)}`);
+  check("frames of shapes only under blendSpace linear: the shape is drawn", pixel[1] > 240, `${pixel.join(",")}`);
+}
+
 (window as unknown as { __checks: unknown }).__checks = results;

packages/spryte/src/webgl2.ts

@@ -778,7 +778,9 @@ export class WebGL2Backend implements Backend {
     gl.bindFramebuffer(gl.FRAMEBUFFER, fbo);
     gl.framebufferTexture2D(gl.FRAMEBUFFER, gl.COLOR_ATTACHMENT0, gl.TEXTURE_2D, tex, 0);
     this.out = { fbo, tex, w, h };
-    // Unit 0 now holds this texture; drop the batch's record of what it held.
+    // Not left on unit 0: the frame draws into this texture, and sampling it meanwhile is a feedback loop (#118).
+    gl.bindTexture(gl.TEXTURE_2D, this.white.tex);
+    // Unit 0 no longer holds what the batch bound there; drop the batch's record of it.
     this.units[0] = null;
     this.lastSrc = null;
     return fbo;
@@ -807,6 +809,8 @@ export class WebGL2Backend implements Backend {
     gl.bindTexture(gl.TEXTURE_2D, out.tex);
     gl.bindSampler(0, this.nearest);
     gl.drawArrays(gl.TRIANGLE_STRIP, 0, 4);
+    // The next frame draws into out.tex again: it can't stay bound (#118).
+    gl.bindTexture(gl.TEXTURE_2D, this.white.tex);
     gl.enable(gl.BLEND);
     gl.useProgram(this.active.program);
     gl.bindVertexArray(this.vao);
@@ -1533,6 +1537,14 @@ export class WebGL2Backend implements Backend {
       gl.bindFramebuffer(gl.FRAMEBUFFER, this.targetStore(target).fbo);
       gl.viewport(0, 0, target.width, target.height);
       this.outH = target.height;
+      // The target's texture may still be bound to a unit: by the batch that last drew it, a copy, a layer or its
+      // own creation. Every program samples all the units, so the next draw would read the texture it writes, a
+      // feedback loop WebGL rejects (#118). Only some of those bindings are tracked: rebind them all.
+      const white = this.white.tex;
+      for (let i = UNITS - 1; i >= 0; i--) {
+        gl.activeTexture(gl.TEXTURE0 + i);
+        gl.bindTexture(gl.TEXTURE_2D, white);
+      }
     } else this.bindOutput();
     this.target = target;
     this.viewW = width;

packages/spryte/test/webgl2.test.ts

@@ -67,6 +67,30 @@ function fakeGL(active: { name: string; type: string; size?: number }[] = []) {
 const M: Matrix = [16, 0, 0, 16, 0, 0];
 const image = (w = 16, h = 16) => ({ width: w, height: h }) as unknown as TextureSource;
 
+/**
+ * Replay the recorded calls and count the draws made while one of the 8 sprite units (which every program samples)
+ * held the texture of the framebuffer being drawn into: a feedback loop, which WebGL rejects.
+ */
+function feedbackDraws(calls: { name: string; args: unknown[] }[]): number {
+  const units = new Map<unknown, unknown>();
+  const attached = new Map<unknown, unknown>();
+  let unit: unknown = "TEXTURE00";
+  let framebuffer: unknown = null;
+  let loops = 0;
+  for (const { name, args } of calls) {
+    // The fake's constants are strings: unit i is "TEXTURE0" + i, and plain TEXTURE0 is unit 0 too.
+    if (name === "activeTexture") unit = args[0] === "TEXTURE0" ? "TEXTURE00" : args[0];
+    else if (name === "bindTexture") units.set(unit, args[1]);
+    else if (name === "bindFramebuffer") framebuffer = args[1];
+    else if (name === "framebufferTexture2D") attached.set(framebuffer, args[3]);
+    else if (/^draw(Arrays|Elements)/.test(name)) {
+      const into = attached.get(framebuffer);
+      for (let i = 0; i < 8; i++) if (into !== undefined && units.get(`TEXTURE0${i}`) === into) loops++;
+    }
+  }
+  return loops;
+}
+
 describe("WebGL2Backend", () => {
   it("uses highp texture coordinates so large atlases sample the right texel on mobile GPUs", () => {
     const { gl, shaderSources } = fakeGL();
@@ -792,6 +816,30 @@ describe("WebGL2Backend", () => {
       expect([info[0], info[1], info[2]]).toEqual([0.1, -0.1, 1].map(Math.fround));
     });
 
+    it("never draws into a target while a sprite unit still holds its texture (#118)", () => {
+      const { gl, calls } = fakeGL();
+      const draw = new Draw(new WebGL2Backend(gl), false);
+      const t = draw.createTarget(16, 16);
+      const hero = image();
+      const at = new Float32Array([30, 15]);
+      for (let frame = 0; frame < 3; frame++) {
+        draw.begin("#000", 100, 50, 1);
+        // Shapes only, so the batch rebinds no unit, then a rect, which binds unit 0 only.
+        draw.into(t, () => roundRect(draw, 0, 0, 16, 16, 2, "#fff"));
+        draw.into(t, () => draw.rect(0, 0, 16, 16, "#fff"));
+        // The image first puts the target on unit 1; sprites() binds it to unit 0 on its own.
+        draw.sprite(hero, 0, 0);
+        draw.sprite(t, 30, 15);
+        draw.end();
+        draw.begin("#000", 100, 50, 1);
+        draw.into(t, () => roundRect(draw, 0, 0, 16, 16, 2, "#fff"));
+        draw.sprites(t, at, 1);
+        draw.end();
+      }
+      expect(calls.some((c) => c.name === "framebufferTexture2D")).toBe(true);
+      expect(feedbackDraws(calls)).toBe(0);
+    });
+
     it("refuses to draw a target into itself, or dispose it while drawing into it", () => {
       const { gl } = fakeGL();
       const draw = new Draw(new WebGL2Backend(gl), false);
@@ -888,6 +936,18 @@ describe("blendSpace linear", () => {
     return f;
   };
 
+  it("never leaves the frame's own texture on a unit while drawing into it (#118)", () => {
+    const { gl, calls } = linearGL();
+    const draw = new Draw(new WebGL2Backend(gl), false);
+    for (let frame = 0; frame < 3; frame++) {
+      draw.begin("#000", 100, 50, 1);
+      roundRect(draw, 0, 0, 16, 16, 2, "#fff");
+      draw.end();
+    }
+    expect(calls.some((c) => c.name === "framebufferTexture2D")).toBe(true);
+    expect(feedbackDraws(calls)).toBe(0);
+  });
+
   it("defines SPRYTE_LINEAR only on linear contexts, and gives every shader spryte_toLinear", () => {
     const plain = fakeGL();
     new WebGL2Backend(plain.gl);