Sixel fixes for VT DOOM: decoder crash, dropped frames, aspect ratio, shared registers, GC pressure

README.md

@@ -63,33 +63,41 @@ painted them is not kept at all.
 
 | 2.x | 3.0 |
 | --- | --- |
-| A Sixel appears in `line.Placements` and `line.Images` | It is painted into `line.Sixel`; `Placements` and `Images` hold Kitty pictures only |
+| A Sixel appears in `line.Placements` and `line.Images` | It is painted into `line.Sixel`, a `SixelStrip` read with `CopyPixelsTo`; `Placements` and `Images` hold Kitty pictures only |
 | `PlacementKind.Sixel` runs | Never created. The value remains so code that compares against it still compiles |
 | `TryGetPlacementAt` / `TryGetImageAt` find Sixel | They find Kitty pictures; ask `line.Sixel?.HasPixelsAt(column)` |
 | A later Sixel splits the one under it | It paints over it: opaque pixels replace, unset pixels (background select 1) leave what was there |
 | `ICH`/`DCH`, insert mode and margin scrolls left Sixel behind | Its pixels move with their cells |
 | Kitty deletes by position or z-index also removed Sixel runs | They no longer touch Sixel; `a=d,d=a` still clears everything |
 | `MaxImageBytes` counted pictures | It counts pictures and strips; scrollback strips go first, oldest line first, and what is on screen is never taken |
 | iTerm2 inline images were Sixel placements | They are painted into strips, as Sixel is |
+| Sixel colour registers were private to each image (xterm's default) | They are shared between images, as on a VT340 and in Windows Terminal; `CSI ? 1070 h` asks for private registers, and RIS resets the shared ones |
+| The raster attribute's aspect ratio (`"Pan;Pad`) was ignored | Each sixel row is Pan/Pad pixels tall (capped at 16); P1 is still ignored |
 
 `line.HasImages` is true for a line with a strip as well as one with Kitty runs.
 
 **What a renderer does now.** Per line: Kitty pictures behind the text (negative `ZIndex`), then the
 text, then the line's Sixel strip, then Kitty pictures in front — see [Images](#images) for the whole
-row. The strip is one blit:
+row. The strip is one blit, read out with `CopyPixelsTo`:
 
 ```csharp
 if (line.Sixel is { } strip)
 {
-    // BGRA8888, straight alpha, top row first: PaintedColumns cells of CellWidth x CellHeight pixels.
-    // Cache the upload against the strip and upload again when strip.Version changes.
+    // Up to the last painted cell, which is all the row shows. Cache the upload against the strip
+    // and upload again when strip.Version changes.
     var cols = Math.Min(strip.PaintedColumns, line.Length);
-    DrawImage(bitmapFor(strip),
-        source: (0, 0, cols * strip.CellWidth, strip.CellHeight),
+    var stride = cols * strip.CellWidth * 4;
+    var pixels = new byte[stride * strip.PixelHeight];   // or a pooled buffer
+    strip.CopyPixelsTo(pixels, stride, cols);            // BGRA8888, straight alpha, top row first
+    DrawImage(BitmapFrom(pixels, cols * strip.CellWidth, strip.PixelHeight, stride),
         dest: (0, row * cellHeight, cols * cellWidth, cellHeight));
 }
 ```
 
+A strip has no `Pixels` property to hand out its memory: it is stored so that copying a line -- which
+a host capturing frames does many times a second -- stays cheap, and `CopyPixelsTo` gives back an
+ordinary contiguous image.
+
 A strip is painted at the cell size in force when its Sixels arrived, so after a font change it is
 stretched onto the new grid, as a picture is. A strip can be painted by the thread writing to the
 terminal while you read it: read `Version` before copying and again after, and treat a copy whose
@@ -605,11 +613,13 @@ void DrawKittyRun(BufferLine line, LinePlacement p, int row)
   it lets the bitmap die when the image does, with no eviction list to maintain, and two placements
   of one picture share the single upload. For animation draw `image.CurrentPixels` and upload again
   when `image.FrameSerial` changes.
-- A Sixel strip: cache against the `SixelStrip` and upload again when `strip.Version` changes. A
-  strip is painted by the thread writing to the terminal, so read `Version` before copying the
-  pixels and again after: a copy whose version moved may be torn, so draw it but do not keep it.
-  A line's copies (`Clone`, `CopyFrom`) share its strip until one of them is written, so a frame
-  captured for a renderer costs a reference rather than the pixels.
+- A Sixel strip: cache against the `SixelStrip` and upload again when `strip.Version` changes,
+  copying the pixels out with `strip.CopyPixelsTo(destination, stride, columns)` -- into a pooled
+  buffer if you upload every frame, so the upload is not itself a large allocation per frame. A
+  strip is painted by the thread writing to the terminal, so read `Version` before copying and again
+  after: a copy whose version moved may be torn, so draw it but do not keep it. A line's copies
+  (`Clone`, `CopyFrom`) share its strip, and writing to a shared strip copies only the part the
+  write touches, so a frame captured for a renderer is cheap.
 
 Draws, not pixels, are what a busy screen costs. A program repainting small regions over and over
 leaves at most one strip per row however many Sixels it sends, and a Kitty client that tiles the

src/XTerm.NET.Tests/ApiLifecycleTests.cs

@@ -63,7 +63,7 @@ public void Reset_restores_the_sixel_modes()
 
         terminal.Write($"{Esc}c");
         Assert.False(terminal.SixelDisplayMode);
-        Assert.True(terminal.SixelPrivateColorRegisters);
+        Assert.False(terminal.SixelPrivateColorRegisters);
     }
 
     [Fact]

src/XTerm.NET.Tests/Graphics/ImageAssertions.cs

@@ -118,11 +118,19 @@ public static (byte R, byte G, byte B, byte A)? SixelPixel(Terminal terminal, in
             return null;
 
         var strip = StripOn(terminal, screenRow)!;
+        var p = StripPixels(strip);
         var offset = y * strip.Stride + (col * strip.CellWidth + x) * TerminalImage.BytesPerPixel;
-        var p = strip.Pixels.Span;
         return (p[offset + 2], p[offset + 1], p[offset], p[offset + 3]);
     }
 
+    /// <summary>A strip's pixels laid out contiguously, BGRA, <see cref="SixelStrip.Stride"/> bytes a row.</summary>
+    public static byte[] StripPixels(SixelStrip strip)
+    {
+        var pixels = new byte[strip.Stride * strip.PixelHeight];
+        strip.CopyPixelsTo(pixels, strip.Stride, strip.Columns);
+        return pixels;
+    }
+
     /// <summary>How many cells of the buffer show Sixel pixels, clipped to each line's width.</summary>
     public static int VisibleSixelCells(Terminal terminal)
     {

src/XTerm.NET.Tests/Graphics/ImageCellLifetimeTests.cs

@@ -351,12 +351,15 @@ public void Clearing_a_dropped_image_leaves_a_blank_rather_than_a_hole()
     [Fact]
     public void Scrollback_pixels_are_dropped_first_when_the_budget_is_exceeded()
     {
-        // Each painted line is a 20-cell strip of 2x3 BGRA cells: 480 bytes. Two pictures of four
-        // lines are 3840; 2500 holds one of them.
-        var terminal = Fresh(o => o.MaxImageBytes = 2500);
+        var terminal = Fresh();
         WriteSixel(terminal);
         var oldest = terminal.Buffer.Lines[terminal.Buffer.YBase]!;
 
+        // Two pictures of four painted lines each are eight strips; a budget of five and a half
+        // holds one picture and part of the other.
+        var lineBytes = oldest.Sixel!.ByteCount;
+        terminal.Options.MaxImageBytes = (int)(lineBytes * 11 / 2);
+
         // Scroll the first picture off the screen into the scrollback, then draw a second.
         terminal.Write($"{Esc}[10;1H" + string.Concat(Enumerable.Repeat("\r\n", 10)));
         terminal.Write($"{Esc}[1;1H");
@@ -536,7 +539,7 @@ public void A_copy_of_a_line_keeps_its_pixels_when_the_original_is_painted()
         WriteSixel(terminal, BlueTwoByFour);
 
         Assert.NotSame(line.Sixel, clone.Sixel);
-        var pixels = clone.Sixel!.Pixels.Span;
+        var pixels = ImageAssertions.StripPixels(clone.Sixel!);
         Assert.Equal((byte)255, pixels[2]); // still red: B, G, R, A
         Assert.Equal((byte)0, pixels[0]);
     }

src/XTerm.NET.Tests/Graphics/SixelDecoderTests.cs

@@ -237,6 +237,144 @@ public void An_image_that_grows_past_the_budget_while_drawing_is_discarded()
         Assert.Null(image);
     }
 
+    /// <summary>
+    /// A wide picture that grows downwards near the budget keeps what it has drawn. Growing for a
+    /// taller band doubled the width as well, overshot the budget, and the retry at "exactly the
+    /// size asked for" took the narrow width the new band needed -- narrower than the canvas, so
+    /// copying the old rows across ran off the end of the buffer. The exception escaped
+    /// Terminal.Write and took the host's read loop with it: VT DOOM froze on its first frame.
+    /// </summary>
+    [Fact]
+    public void A_wide_image_growing_downwards_near_the_budget_keeps_its_rows()
+    {
+        // 300 wide, then a 10-wide second band: doubling both ways asks 600x12 = 7200 pixels.
+        TerminalImage? image = null;
+        var exception = Record.Exception(() =>
+            image = TryDecode("#0;2;100;0;0!300~-!10~", configure: o => o.MaxSixelPixels = 5000));
+
+        Assert.Null(exception);
+        Assert.NotNull(image);
+        Assert.Equal(300, image!.PixelWidth);
+        Assert.Equal(12, image.PixelHeight);
+        Assert.Equal(Red, Pixel(image, 299, 0));
+        Assert.Equal(Red, Pixel(image, 9, 6));
+    }
+
+    /// <summary>
+    /// A picture that fits the budget is kept even when the canvas's doubling would not. Drawn in
+    /// pieces, the canvas doubles ahead of the picture -- 300 wide on a 512-wide canvas here -- and
+    /// the retry kept that whole capacity, failed the budget, and threw the picture away. VT DOOM at
+    /// 307x75 cells lost every frame that grew that way: data arriving, nothing on screen.
+    /// </summary>
+    [Fact]
+    public void A_picture_inside_the_budget_survives_the_canvas_doubling_past_it()
+    {
+        // Three runs make it 300 wide by doubling to 512; ten bands make it 60 tall.
+        var body = "#0;2;100;0;0!100~!100~!100~" + string.Concat(Enumerable.Repeat("-!300~", 9));
+        var image = TryDecode(body, configure: o => o.MaxSixelPixels = 300 * 60 * 13 / 10);
+
+        Assert.NotNull(image);
+        Assert.Equal(300, image!.PixelWidth);
+        Assert.Equal(60, image.PixelHeight);
+        Assert.Equal(Red, Pixel(image, 299, 59));
+    }
+
+    /// <summary>
+    /// The raster attribute's Pan;Pad is the pixel aspect ratio: each sixel row is Pan/Pad pixel
+    /// rows tall. VT DOOM scales its picture vertically this way (and horizontally with repeat
+    /// counts); ignoring it drew the game squashed to a fifth of its height.
+    /// </summary>
+    [Fact]
+    public void The_raster_aspect_ratio_makes_each_sixel_row_that_many_pixels_tall()
+    {
+        // 3:1, so one band of six sixel rows is eighteen pixels; '@' sets only the top sixel row.
+        var image = Decode("\"3;1#0;2;100;0;0!2@-!2~");
+
+        Assert.Equal(2, image.PixelWidth);
+        Assert.Equal(36, image.PixelHeight);
+        Assert.Equal(Red, Pixel(image, 0, 0));
+        Assert.Equal(Red, Pixel(image, 0, 2));            // still the first sixel row
+        Assert.Equal((byte)0, Pixel(image, 0, 3).A);     // the second sixel row was not set
+        Assert.Equal(Red, Pixel(image, 1, 18));           // the second band starts 18 rows down
+        Assert.Equal(Red, Pixel(image, 1, 35));
+    }
+
+    [Fact]
+    public void A_square_aspect_ratio_and_none_at_all_draw_the_same()
+    {
+        var square = Decode("\"1;1#0;2;100;0;0!3~-!3~");
+        var none = Decode("#0;2;100;0;0!3~-!3~");
+
+        Assert.Equal(none.PixelHeight, square.PixelHeight);
+        Assert.Equal(12, square.PixelHeight);
+        Assert.True(square.Pixels.Span.SequenceEqual(none.Pixels.Span));
+    }
+
+    [Fact]
+    public void An_absurd_aspect_ratio_is_capped()
+    {
+        var image = Decode("\"1000;1#0;2;100;0;0~");
+
+        Assert.Equal(16 * 6, image.PixelHeight);
+    }
+
+    /// <summary>
+    /// Colour registers are shared between images by default, as on a VT340 and in Windows
+    /// Terminal: a register one image defines is still defined for the next. VT DOOM sends its
+    /// palette only when it changes, and drew in a scatter of default colours on black without this.
+    /// </summary>
+    [Fact]
+    public void A_colour_defined_by_one_image_is_still_defined_for_the_next()
+    {
+        var terminal = Fresh();
+        WriteSixel(terminal, "#200;2;0;100;0#200!2~");
+        WriteSixel(terminal, "#200!2~");
+
+        Assert.Equal(Green, Pixel(Placed(terminal)!, 0, 0));
+    }
+
+    [Fact]
+    public void Private_registers_are_still_one_mode_away()
+    {
+        var terminal = Fresh();
+        terminal.Write($"{Esc}[?1070h");
+        WriteSixel(terminal, "#200;2;0;100;0#200!2~");
+        WriteSixel(terminal, "#200!2~");
+
+        // A fresh palette for the second image: register 200 is back to its default.
+        Assert.NotEqual(Green, Pixel(Placed(terminal)!, 0, 0));
+    }
+
+    [Fact]
+    public void A_full_reset_returns_the_shared_registers_to_their_defaults()
+    {
+        var terminal = Fresh();
+        WriteSixel(terminal, "#200;2;0;100;0#200!2~");
+        terminal.Write($"{Esc}c");
+        WriteSixel(terminal, "#200!2~");
+
+        Assert.NotEqual(Green, Pixel(Placed(terminal)!, 0, 0));
+    }
+
+    /// <summary>
+    /// The decoder reuses its canvas from one picture to the next, so a smaller, transparent picture
+    /// after a large opaque one must start from clear pixels rather than the last picture's.
+    /// </summary>
+    [Fact]
+    public void A_reused_canvas_does_not_show_the_previous_picture()
+    {
+        var terminal = Fresh();
+        WriteSixel(terminal, "#0;2;100;0;0!40~-!40~-!40~", OpaqueBackground);
+        WriteSixel(terminal, "#1;2;0;100;0!2@");
+
+        var second = Placed(terminal)!;
+        Assert.Equal(2, second.PixelWidth);
+        Assert.Equal(6, second.PixelHeight);
+        Assert.Equal(Green, Pixel(second, 0, 0));
+        Assert.Equal((byte)0, Pixel(second, 0, 1).A);   // unset, so transparent -- not the red before it
+        Assert.Equal((byte)0, Pixel(second, 1, 5).A);
+    }
+
     [Fact]
     public void An_abandoned_payload_produces_no_image()
     {

src/XTerm.NET.Tests/RequestModeTests.cs

@@ -109,6 +109,7 @@ public void Reports_each_mode_as_set_after_setting_it_and_reset_after_resetting_
     [InlineData(1006)]  // SGR mouse encoding
     [InlineData(1016)]  // SGR-Pixels mouse encoding
     [InlineData(1049)]  // alternate buffer
+    [InlineData(1070)]  // private Sixel colour registers: shared by default, as on a VT340
     [InlineData(2004)]  // bracketed paste
     [InlineData(2026)]  // synchronized output
     public void Reports_an_untouched_mode_as_reset(int mode)
@@ -138,13 +139,11 @@ public void Reports_grapheme_clustering_as_permanently_set()
 
     /// <summary>
     /// Some modes start on, and reporting them as reset would cost an application the feature: it
-    /// would turn wraparound on that was already on, show a cursor it meant to leave alone, and
-    /// switch Sixel colour registers to shared when they were already private.
+    /// would turn wraparound on that was already on, or show a cursor it meant to leave alone.
     /// </summary>
     [Theory]
     [InlineData(7)]     // wraparound
     [InlineData(25)]    // cursor visible
-    [InlineData(1070)]  // private Sixel colour registers
     public void Reports_a_mode_that_starts_on_as_set(int mode)
     {
         var terminal = Fresh();