Merge pull request #1639 from qwerasd205/kitty-image-fixes

Kitty image fixes

src/terminal/Screen.zig

@@ -540,6 +540,9 @@ pub fn cursorDownScroll(self: *Screen) !void {
     assert(self.cursor.y == self.pages.rows - 1);
     defer self.assertIntegrity();
 
+    // Scrolling dirties the images because it updates their placements pins.
+    self.kitty_images.dirty = true;
+
     // If we have no scrollback, then we shift all our rows instead.
     if (self.no_scrollback) {
         // If we have a single-row screen, we have no rows to shift

src/terminal/Terminal.zig

@@ -1075,6 +1075,9 @@ pub fn index(self: *Terminal) !void {
         self.screen.cursor.x >= self.scrolling_region.left and
         self.screen.cursor.x <= self.scrolling_region.right)
     {
+        // Scrolling dirties the images because it updates their placements pins.
+        self.screen.kitty_images.dirty = true;
+
         // If our scrolling region is the full screen, we create scrollback.
         // Otherwise, we simply scroll the region.
         if (self.scrolling_region.top == 0 and
@@ -1391,6 +1394,9 @@ pub fn insertLines(self: *Terminal, count: usize) void {
         self.screen.cursor.x < self.scrolling_region.left or
         self.screen.cursor.x > self.scrolling_region.right) return;
 
+    // Scrolling dirties the images because it updates their placements pins.
+    self.screen.kitty_images.dirty = true;
+
     // Remaining rows from our cursor to the bottom of the scroll region.
     const rem = self.scrolling_region.bottom - self.screen.cursor.y + 1;
 
@@ -1534,6 +1540,9 @@ pub fn deleteLines(self: *Terminal, count_req: usize) void {
         self.screen.cursor.x < self.scrolling_region.left or
         self.screen.cursor.x > self.scrolling_region.right) return;
 
+    // Scrolling dirties the images because it updates their placements pins.
+    self.screen.kitty_images.dirty = true;
+
     // top is just the cursor position. insertLines starts at the cursor
     // so this is our top. We want to shift lines down, down to the bottom
     // of the scroll region.

src/terminal/kitty/graphics_command.zig

@@ -3,6 +3,8 @@ const assert = std.debug.assert;
 const Allocator = std.mem.Allocator;
 const ArenaAllocator = std.heap.ArenaAllocator;
 
+const log = std.log.scoped(.kitty_gfx);
+
 /// The key-value pairs for the control information for a command. The
 /// keys are always single characters and the values are either single
 /// characters or 32-bit unsigned integers.
@@ -27,8 +29,11 @@ pub const CommandParser = struct {
     kv_temp_len: u4 = 0,
     kv_current: u8 = 0, // Current kv key
 
-    /// This is the list of bytes that contains both KV data and final
+    /// This is the list we use to collect the bytes from the data payload.
-    /// data. You shouldn't access this directly.
+    /// The Kitty Graphics protocol specification seems to imply that the
+    /// payload content of a single command should never exceed 4096 bytes,
+    /// but Kitty itself supports larger payloads, so we use an ArrayList
+    /// here instead of a fixed buffer so that we can too.
     data: std.ArrayList(u8),
 
     /// Internal state for parsing.
@@ -42,7 +47,7 @@ pub const CommandParser = struct {
         control_value,
         control_value_ignore,
 
-        /// We're parsing the data blob.
+        /// Collecting the data payload blob.
         data,
     };
 
@@ -106,9 +111,6 @@ pub const CommandParser = struct {
 
             .data => try self.data.append(c),
         }
-
-        // We always add to our data list because this is our stable
-        // array of bytes that we'll reference everywhere else.
     }
 
     /// Complete the parsing. This must be called after all the
@@ -165,12 +167,45 @@ pub const CommandParser = struct {
         return .{
             .control = control,
             .quiet = quiet,
-            .data = if (self.data.items.len == 0) "" else data: {
+            .data = try self.decodeData(),
-                break :data try self.data.toOwnedSlice();
-            },
         };
     }
 
+    /// Decodes the payload data from base64 and returns it as a slice.
+    /// This function will destroy the contents of self.data, it should
+    /// only be used once we are done collecting payload bytes.
+    fn decodeData(self: *CommandParser) ![]const u8 {
+        if (self.data.items.len == 0) {
+            return "";
+        }
+
+        const Base64Decoder = std.base64.standard_no_pad.Decoder;
+
+        // We remove any padding, since it's optional, and decode without it.
+        while (self.data.items[self.data.items.len - 1] == '=') {
+            self.data.items.len -= 1;
+        }
+
+        const size = Base64Decoder.calcSizeForSlice(self.data.items) catch |err| {
+            log.warn("failed to calculate base64 size for payload: {}", .{err});
+            return error.InvalidData;
+        };
+
+        // This is kinda cursed, but we can decode the base64 on top of
+        // itself, since it's guaranteed that the encoded size is larger,
+        // and any bytes in areas that are written to will have already
+        // been used (assuming scalar decoding).
+        Base64Decoder.decode(self.data.items[0..size], self.data.items) catch |err| {
+            log.warn("failed to decode base64 payload data: {}", .{err});
+            return error.InvalidData;
+        };
+
+        // Remove the extra bytes.
+        self.data.items.len = size;
+
+        return try self.data.toOwnedSlice();
+    }
+
     fn accumulateValue(self: *CommandParser, c: u8, overflow_state: State) !void {
         const idx = self.kv_temp_len;
         self.kv_temp_len += 1;
@@ -855,7 +890,7 @@ test "query command" {
     var p = CommandParser.init(alloc);
     defer p.deinit();
 
-    const input = "i=31,s=1,v=1,a=q,t=d,f=24;AAAA";
+    const input = "i=31,s=1,v=1,a=q,t=d,f=24;QUFBQQ";
     for (input) |c| try p.feed(c);
     const command = try p.complete();
     defer command.deinit(alloc);

src/terminal/kitty/graphics_image.zig

@@ -5,6 +5,7 @@ const Allocator = std.mem.Allocator;
 const ArenaAllocator = std.heap.ArenaAllocator;
 const posix = std.posix;
 
+const fastmem = @import("../../fastmem.zig");
 const command = @import("graphics_command.zig");
 const point = @import("../point.zig");
 const PageList = @import("../PageList.zig");
@@ -56,30 +57,16 @@ pub const LoadingImage = struct {
             .display = cmd.display(),
         };
 
-        // Special case for the direct medium, we just add it directly
+        // Special case for the direct medium, we just add the chunk directly.
-        // which will handle copying the data, base64 decoding, etc.
         if (t.medium == .direct) {
             try result.addData(alloc, cmd.data);
             return result;
         }
 
-        // For every other medium, we'll need to at least base64 decode
+        // Otherwise, the payload data is guaranteed to be a path.
-        // the data to make it useful so let's do that. Also, all the data
-        // has to be path data so we can put it in a stack-allocated buffer.
-        var buf: [std.fs.MAX_PATH_BYTES]u8 = undefined;
-        const Base64Decoder = std.base64.standard.Decoder;
-        const size = Base64Decoder.calcSizeForSlice(cmd.data) catch |err| {
-            log.warn("failed to calculate base64 size for file path: {}", .{err});
-            return error.InvalidData;
-        };
-        if (size > buf.len) return error.FilePathTooLong;
-        Base64Decoder.decode(&buf, cmd.data) catch |err| {
-            log.warn("failed to decode base64 data: {}", .{err});
-            return error.InvalidData;
-        };
 
         if (comptime builtin.os.tag != .windows) {
-            if (std.mem.indexOfScalar(u8, buf[0..size], 0) != null) {
+            if (std.mem.indexOfScalar(u8, cmd.data, 0) != null) {
                 // posix.realpath *asserts* that the path does not have
                 // internal nulls instead of erroring.
                 log.warn("failed to get absolute path: BadPathName", .{});
@@ -88,7 +75,7 @@ pub const LoadingImage = struct {
         }
 
         var abs_buf: [std.fs.MAX_PATH_BYTES]u8 = undefined;
-        const path = posix.realpath(buf[0..size], &abs_buf) catch |err| {
+        const path = posix.realpath(cmd.data, &abs_buf) catch |err| {
             log.warn("failed to get absolute path: {}", .{err});
             return error.InvalidData;
         };
@@ -229,42 +216,25 @@ pub const LoadingImage = struct {
         alloc.destroy(self);
     }
 
-    /// Adds a chunk of base64-encoded data to the image. Use this if the
+    /// Adds a chunk of data to the image. Use this if the image
-    /// image is coming in chunks (the "m" parameter in the protocol).
+    /// is coming in chunks (the "m" parameter in the protocol).
     pub fn addData(self: *LoadingImage, alloc: Allocator, data: []const u8) !void {
         // If no data, skip
         if (data.len == 0) return;
 
-        // Grow our array list by size capacity if it needs it
-        const Base64Decoder = std.base64.standard.Decoder;
-        const size = Base64Decoder.calcSizeForSlice(data) catch |err| {
-            log.warn("failed to calculate size for base64 data: {}", .{err});
-            return error.InvalidData;
-        };
-
         // If our data would get too big, return an error
-        if (self.data.items.len + size > max_size) {
+        if (self.data.items.len + data.len > max_size) {
             log.warn("image data too large max_size={}", .{max_size});
             return error.InvalidData;
         }
 
-        try self.data.ensureUnusedCapacity(alloc, size);
+        // Ensure we have enough room to add the data
+        // to the end of the ArrayList before doing so.
+        try self.data.ensureUnusedCapacity(alloc, data.len);
 
-        // We decode directly into the arraylist
         const start_i = self.data.items.len;
-        self.data.items.len = start_i + size;
+        self.data.items.len = start_i + data.len;
-        const buf = self.data.items[start_i..];
+        fastmem.copy(u8, self.data.items[start_i..], data);
-        Base64Decoder.decode(buf, data) catch |err| switch (err) {
-            // We have to ignore invalid padding because lots of encoders
-            // add the wrong padding. Since we validate image data later
-            // (PNG decode or simple dimensions check), we can ignore this.
-            error.InvalidPadding => {},
-
-            else => {
-                log.warn("failed to decode base64 data: {}", .{err});
-                return error.InvalidData;
-            },
-        };
     }
 
     /// Complete the chunked image, returning a completed image.
@@ -457,23 +427,6 @@ pub const Rect = struct {
     bottom_right: PageList.Pin,
 };
 
-/// Easy base64 encoding function.
-fn testB64(alloc: Allocator, data: []const u8) ![]const u8 {
-    const B64Encoder = std.base64.standard.Encoder;
-    const b64 = try alloc.alloc(u8, B64Encoder.calcSize(data.len));
-    errdefer alloc.free(b64);
-    return B64Encoder.encode(b64, data);
-}
-
-/// Easy base64 decoding function.
-fn testB64Decode(alloc: Allocator, data: []const u8) ![]const u8 {
-    const B64Decoder = std.base64.standard.Decoder;
-    const result = try alloc.alloc(u8, try B64Decoder.calcSizeForSlice(data));
-    errdefer alloc.free(result);
-    try B64Decoder.decode(result, data);
-    return result;
-}
-
 // This specifically tests we ALLOW invalid RGB data because Kitty
 // documents that this should work.
 test "image load with invalid RGB data" {
@@ -548,7 +501,7 @@ test "image load: rgb, zlib compressed, direct" {
         } },
         .data = try alloc.dupe(
             u8,
-            @embedFile("testdata/image-rgb-zlib_deflate-128x96-2147483647.data"),
+            @embedFile("testdata/image-rgb-zlib_deflate-128x96-2147483647-raw.data"),
         ),
     };
     defer cmd.deinit(alloc);
@@ -576,7 +529,7 @@ test "image load: rgb, not compressed, direct" {
         } },
         .data = try alloc.dupe(
             u8,
-            @embedFile("testdata/image-rgb-none-20x15-2147483647.data"),
+            @embedFile("testdata/image-rgb-none-20x15-2147483647-raw.data"),
         ),
     };
     defer cmd.deinit(alloc);
@@ -593,7 +546,7 @@ test "image load: rgb, zlib compressed, direct, chunked" {
     const testing = std.testing;
     const alloc = testing.allocator;
 
-    const data = @embedFile("testdata/image-rgb-zlib_deflate-128x96-2147483647.data");
+    const data = @embedFile("testdata/image-rgb-zlib_deflate-128x96-2147483647-raw.data");
 
     // Setup our initial chunk
     var cmd: command.Command = .{
@@ -630,7 +583,7 @@ test "image load: rgb, zlib compressed, direct, chunked with zero initial chunk"
     const testing = std.testing;
     const alloc = testing.allocator;
 
-    const data = @embedFile("testdata/image-rgb-zlib_deflate-128x96-2147483647.data");
+    const data = @embedFile("testdata/image-rgb-zlib_deflate-128x96-2147483647-raw.data");
 
     // Setup our initial chunk
     var cmd: command.Command = .{
@@ -668,11 +621,7 @@ test "image load: rgb, not compressed, temporary file" {
 
     var tmp_dir = try internal_os.TempDir.init();
     defer tmp_dir.deinit();
-    const data = try testB64Decode(
+    const data = @embedFile("testdata/image-rgb-none-20x15-2147483647-raw.data");
-        alloc,
-        @embedFile("testdata/image-rgb-none-20x15-2147483647.data"),
-    );
-    defer alloc.free(data);
     try tmp_dir.dir.writeFile("image.data", data);
 
     var buf: [std.fs.MAX_PATH_BYTES]u8 = undefined;
@@ -687,7 +636,7 @@ test "image load: rgb, not compressed, temporary file" {
             .height = 15,
             .image_id = 31,
         } },
-        .data = try testB64(alloc, path),
+        .data = try alloc.dupe(u8, path),
     };
     defer cmd.deinit(alloc);
     var loading = try LoadingImage.init(alloc, &cmd);
@@ -706,11 +655,7 @@ test "image load: rgb, not compressed, regular file" {
 
     var tmp_dir = try internal_os.TempDir.init();
     defer tmp_dir.deinit();
-    const data = try testB64Decode(
+    const data = @embedFile("testdata/image-rgb-none-20x15-2147483647-raw.data");
-        alloc,
-        @embedFile("testdata/image-rgb-none-20x15-2147483647.data"),
-    );
-    defer alloc.free(data);
     try tmp_dir.dir.writeFile("image.data", data);
 
     var buf: [std.fs.MAX_PATH_BYTES]u8 = undefined;
@@ -725,7 +670,7 @@ test "image load: rgb, not compressed, regular file" {
             .height = 15,
             .image_id = 31,
         } },
-        .data = try testB64(alloc, path),
+        .data = try alloc.dupe(u8, path),
     };
     defer cmd.deinit(alloc);
     var loading = try LoadingImage.init(alloc, &cmd);
@@ -757,7 +702,7 @@ test "image load: png, not compressed, regular file" {
             .height = 0,
             .image_id = 31,
         } },
-        .data = try testB64(alloc, path),
+        .data = try alloc.dupe(u8, path),
     };
     defer cmd.deinit(alloc);
     var loading = try LoadingImage.init(alloc, &cmd);

src/terminal/kitty/testdata/image-rgb-none-20x15-2147483647.data

@@ -1 +0,0 @@
-DRoeCxgcCxcjEh4qDBgkCxcjChYiCxcjCRclBRMhBxIXHysvTVNRbHJwcXB2Li0zCBYXEyEiCxkaDBobChcbCBUZDxsnBBAcEBwoChYiCxcjDBgkDhwqBxUjDBccm6aqy9HP1NrYzs3UsK+2IjAxCBYXCBYXBxUWFBoaDxUVICYqIyktERcZDxUXDxUVEhgYDhUTCxIQGh8XusC4zM7FvL61q6elmZWTTVtcDBobDRscCxkaKS8vaW9vxMnOur/EiY+RaW5wICYmW2FhfYOBQEZEnqSc4ebeqauilZaOsa2rm5eVcH5/GigpChgZCBYX0NHP3d7c3tzbx8XExsTEvry8wL241dLN0tDF0tDF29nM4d/StbKpzMrAUk5DZmJXeYSGKTU3ER0fDRkb1tfVysvJ0tDPsa+tr6ytop+gmZaRqaahuritw8G2urirqKaZiYZ9paKZZmJXamZbOkZIDhocBxMVBBASxMDBtrKzqqanoZ2ejYeLeHF2eXFvhn58npePta6ml5CKgXp0W1hPaWZdZWdSYmRPFiYADR0AFCQAEyMAt7O0lJCRf3t8eHR1Zl9kY1xhYVpYbGRieXJqeHFpdW1oc2tmcG1kX1xTbW9ajY96jp55kaF8kKB7kaF8sK6rcnFtX11cXFpZW1pWWFdTXVpTXltUaGJgY11bY11da2Vla25dam1ccHtTnqmBorVtp7pypLdvobRsh4aCaGdjWFZVXFpZYWBcZ2ZiaGVeZGFaY11bYlxaV1FRZ2FhdHdmbG9egItjo66GpLdvq752rL93rsF5kpKIZ2ddWFxTW19WbnZdipJ6cnhaaW9RaGhgV1ZPY2Jga2poanFQd35dk6Vpn7B0oLFvorNxm6xqmKlnv760enpwVlpRW19Wc3til5+Hl55/k5p7iIiAcnJqd3Z0bm1rcHdWh45tipxgladrkaJglKVjkaJgkqNh09DJiYZ/YmZdY2deeYZYjJlrj51ijpxhztHClJaIdHNvdHNvanNHi5RpmaxnjKBbmqhrmadqkJ5hi5lcxsO8jImCaGtiYmZdg5Bikp9xjJpfjpxh1djJqq2eamllZ2Zid4BVmKF2kqZhh5tWlaNmlaNmjpxfjJpdw729rqiodnZ0cHBuiplij55nj6FVjJ5SzdC9t7qncW1sXlpZh45iqbCEmKllmapmmqlqnq1unaxtoK9w