fix(renderer): Don't force images to grid/cell sizes. (#7367)

Also fixed Metal not interpolating scaled images.

See commit message for more details.

|`main` (Metal)|`main` (OpenGL)|Kitty|fixed (Metal)|fixed (OpenGL)|
|-|-|-|-|-|

|![image](https://github.com/user-attachments/assets/bfe09451-0a41-4952-8a55-5d7a9e5b2813)|![image](https://github.com/user-attachments/assets/70ec6775-ba00-40b7-a32d-dcaaa70671b1)|![image](https://github.com/user-attachments/assets/6d03729b-9b8e-4b25-850d-913e76f6183b)|![image](https://github.com/user-attachments/assets/aedfccf0-220c-4575-b5e4-8b467de6d5df)|![image](https://github.com/user-attachments/assets/300a080a-73f0-4c69-9603-df342767a83f)|

<sup>(Zoom in for details.)</sup>

> [!NOTE]
> This comparison reveals a separate problem we have with image scaling,
which Kitty gets right but we don't -- the interpolation is too dark
because of gamma error, we need to be interpolating in linear space but
instead we're interpolating in gamma compressed space. I'll try to
figure out the best way to resolve this.
pull/7368/head
Mitchell Hashimoto 2025-05-15 11:42:05 -07:00 committed by GitHub
commit 1d0cb1a9b0
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
7 changed files with 187 additions and 191 deletions

View File

@ -1872,6 +1872,8 @@ fn prepKittyGraphics(
// points. This lets us determine offsets and containment of placements.
const top = t.screen.pages.getTopLeft(.viewport);
const bot = t.screen.pages.getBottomRight(.viewport).?;
const top_y = t.screen.pages.pointFromPin(.screen, top).?.screen.y;
const bot_y = t.screen.pages.pointFromPin(.screen, bot).?.screen.y;
// Go through the placements and ensure the image is loaded on the GPU.
var it = storage.placements.iterator();
@ -1903,7 +1905,7 @@ fn prepKittyGraphics(
continue;
};
try self.prepKittyPlacement(t, &top, &bot, &image, p);
try self.prepKittyPlacement(t, top_y, bot_y, &image, p);
}
// If we have virtual placements then we need to scan for placeholders.
@ -2009,8 +2011,8 @@ fn prepKittyVirtualPlacement(
fn prepKittyPlacement(
self: *Metal,
t: *terminal.Terminal,
top: *const terminal.Pin,
bot: *const terminal.Pin,
top_y: u32,
bot_y: u32,
image: *const terminal.kitty.graphics.Image,
p: *const terminal.kitty.graphics.ImageStorage.Placement,
) !void {
@ -2018,78 +2020,47 @@ fn prepKittyPlacement(
// a rect then its virtual or something so skip it.
const rect = p.rect(image.*, t) orelse return;
// This is expensive but necessary.
const img_top_y = t.screen.pages.pointFromPin(.screen, rect.top_left).?.screen.y;
const img_bot_y = t.screen.pages.pointFromPin(.screen, rect.bottom_right).?.screen.y;
// If the selection isn't within our viewport then skip it.
if (bot.before(rect.top_left)) return;
if (rect.bottom_right.before(top.*)) return;
// If the top left is outside the viewport we need to calc an offset
// so that we render (0, 0) with some offset for the texture.
const offset_y: u32 = if (rect.top_left.before(top.*)) offset_y: {
const vp_y = t.screen.pages.pointFromPin(.screen, top.*).?.screen.y;
const img_y = t.screen.pages.pointFromPin(.screen, rect.top_left).?.screen.y;
const offset_cells = vp_y - img_y;
const offset_pixels = offset_cells * self.grid_metrics.cell_height;
break :offset_y @intCast(offset_pixels);
} else 0;
// Get the grid size that respects aspect ratio
const grid_size = p.gridSize(image.*, t);
// If we specify `rows` then our offset above is in viewport space
// and not in the coordinate space of the source image. Without `rows`
// that's one and the same.
const source_offset_y: u32 = if (grid_size.rows > 0) source_offset_y: {
// Determine the scale factor to apply for this row height.
const image_height: f64 = @floatFromInt(image.height);
const viewport_height: f64 = @floatFromInt(grid_size.rows * self.grid_metrics.cell_height);
const scale: f64 = image_height / viewport_height;
// Apply the scale to the offset
const offset_y_f64: f64 = @floatFromInt(offset_y);
const source_offset_y_f64: f64 = offset_y_f64 * scale;
break :source_offset_y @intFromFloat(@round(source_offset_y_f64));
} else offset_y;
if (img_top_y > bot_y) return;
if (img_bot_y < top_y) return;
// We need to prep this image for upload if it isn't in the cache OR
// it is in the cache but the transmit time doesn't match meaning this
// image is different.
try self.prepKittyImage(image);
// Convert our screen point to a viewport point
const viewport: terminal.point.Point = t.screen.pages.pointFromPin(
.viewport,
rect.top_left,
) orelse .{ .viewport = .{} };
// Calculate the dimensions of our image, taking in to
// account the rows / columns specified by the placement.
const dest_size = p.calculatedSize(image.*, t);
// Calculate the source rectangle
const source_x = @min(image.width, p.source_x);
const source_y = @min(image.height, p.source_y + source_offset_y);
const source_y = @min(image.height, p.source_y);
const source_width = if (p.source_width > 0)
@min(image.width - source_x, p.source_width)
else
image.width;
const source_height = if (p.source_height > 0)
@min(image.height, p.source_height)
@min(image.height - source_y, p.source_height)
else
image.height -| source_y;
image.height;
// Calculate the width/height of our image.
const dest_width = grid_size.cols * self.grid_metrics.cell_width;
const dest_height = if (grid_size.rows > 0) rows: {
// Clip to the viewport to handle scrolling. offset_y is already in
// viewport scale so we can subtract it directly.
break :rows (grid_size.rows * self.grid_metrics.cell_height) - offset_y;
} else source_height;
// Get the viewport-relative Y position of the placement.
const y_pos: i32 = @as(i32, @intCast(img_top_y)) - @as(i32, @intCast(top_y));
// Accumulate the placement
if (image.width > 0 and image.height > 0) {
if (dest_size.width > 0 and dest_size.height > 0) {
try self.image_placements.append(self.alloc, .{
.image_id = image.id,
.x = @intCast(rect.top_left.x),
.y = @intCast(viewport.viewport.y),
.y = y_pos,
.z = p.z,
.width = dest_width,
.height = dest_height,
.width = dest_size.width,
.height = dest_size.height,
.cell_offset_x = p.x_offset,
.cell_offset_y = p.y_offset,
.source_x = source_x,

View File

@ -913,6 +913,8 @@ fn prepKittyGraphics(
// points. This lets us determine offsets and containment of placements.
const top = t.screen.pages.getTopLeft(.viewport);
const bot = t.screen.pages.getBottomRight(.viewport).?;
const top_y = t.screen.pages.pointFromPin(.screen, top).?.screen.y;
const bot_y = t.screen.pages.pointFromPin(.screen, bot).?.screen.y;
// Go through the placements and ensure the image is loaded on the GPU.
var it = storage.placements.iterator();
@ -944,7 +946,7 @@ fn prepKittyGraphics(
continue;
};
try self.prepKittyPlacement(t, &top, &bot, &image, p);
try self.prepKittyPlacement(t, top_y, bot_y, &image, p);
}
// If we have virtual placements then we need to scan for placeholders.
@ -1050,8 +1052,8 @@ fn prepKittyVirtualPlacement(
fn prepKittyPlacement(
self: *OpenGL,
t: *terminal.Terminal,
top: *const terminal.Pin,
bot: *const terminal.Pin,
top_y: u32,
bot_y: u32,
image: *const terminal.kitty.graphics.Image,
p: *const terminal.kitty.graphics.ImageStorage.Placement,
) !void {
@ -1059,78 +1061,47 @@ fn prepKittyPlacement(
// a rect then its virtual or something so skip it.
const rect = p.rect(image.*, t) orelse return;
// This is expensive but necessary.
const img_top_y = t.screen.pages.pointFromPin(.screen, rect.top_left).?.screen.y;
const img_bot_y = t.screen.pages.pointFromPin(.screen, rect.bottom_right).?.screen.y;
// If the selection isn't within our viewport then skip it.
if (bot.before(rect.top_left)) return;
if (rect.bottom_right.before(top.*)) return;
// If the top left is outside the viewport we need to calc an offset
// so that we render (0, 0) with some offset for the texture.
const offset_y: u32 = if (rect.top_left.before(top.*)) offset_y: {
const vp_y = t.screen.pages.pointFromPin(.screen, top.*).?.screen.y;
const img_y = t.screen.pages.pointFromPin(.screen, rect.top_left).?.screen.y;
const offset_cells = vp_y - img_y;
const offset_pixels = offset_cells * self.grid_metrics.cell_height;
break :offset_y @intCast(offset_pixels);
} else 0;
// Get the grid size that respects aspect ratio
const grid_size = p.gridSize(image.*, t);
// If we specify `rows` then our offset above is in viewport space
// and not in the coordinate space of the source image. Without `rows`
// that's one and the same.
const source_offset_y: u32 = if (grid_size.rows > 0) source_offset_y: {
// Determine the scale factor to apply for this row height.
const image_height: f64 = @floatFromInt(image.height);
const viewport_height: f64 = @floatFromInt(grid_size.rows * self.grid_metrics.cell_height);
const scale: f64 = image_height / viewport_height;
// Apply the scale to the offset
const offset_y_f64: f64 = @floatFromInt(offset_y);
const source_offset_y_f64: f64 = offset_y_f64 * scale;
break :source_offset_y @intFromFloat(@round(source_offset_y_f64));
} else offset_y;
if (img_top_y > bot_y) return;
if (img_bot_y < top_y) return;
// We need to prep this image for upload if it isn't in the cache OR
// it is in the cache but the transmit time doesn't match meaning this
// image is different.
try self.prepKittyImage(image);
// Convert our screen point to a viewport point
const viewport: terminal.point.Point = t.screen.pages.pointFromPin(
.viewport,
rect.top_left,
) orelse .{ .viewport = .{} };
// Calculate the dimensions of our image, taking in to
// account the rows / columns specified by the placement.
const dest_size = p.calculatedSize(image.*, t);
// Calculate the source rectangle
const source_x = @min(image.width, p.source_x);
const source_y = @min(image.height, p.source_y + source_offset_y);
const source_y = @min(image.height, p.source_y);
const source_width = if (p.source_width > 0)
@min(image.width - source_x, p.source_width)
else
image.width;
const source_height = if (p.source_height > 0)
@min(image.height, p.source_height)
@min(image.height - source_y, p.source_height)
else
image.height -| source_y;
image.height;
// Calculate the width/height of our image.
const dest_width = grid_size.cols * self.grid_metrics.cell_width;
const dest_height = if (grid_size.rows > 0) rows: {
// Clip to the viewport to handle scrolling. offset_y is already in
// viewport scale so we can subtract it directly.
break :rows (grid_size.rows * self.grid_metrics.cell_height) - offset_y;
} else source_height;
// Get the viewport-relative Y position of the placement.
const y_pos: i32 = @as(i32, @intCast(img_top_y)) - @as(i32, @intCast(top_y));
// Accumulate the placement
if (image.width > 0 and image.height > 0) {
if (dest_size.width > 0 and dest_size.height > 0) {
try self.image_placements.append(self.alloc, .{
.image_id = image.id,
.x = @intCast(rect.top_left.x),
.y = @intCast(viewport.viewport.y),
.y = y_pos,
.z = p.z,
.width = dest_width,
.height = dest_height,
.width = dest_size.width,
.height = dest_size.height,
.cell_offset_x = p.x_offset,
.cell_offset_y = p.y_offset,
.source_x = source_x,
@ -2511,8 +2482,8 @@ fn drawImages(
// Setup our data
try bind.vbo.setData(ImageProgram.Input{
.grid_col = @intCast(p.x),
.grid_row = @intCast(p.y),
.grid_col = p.x,
.grid_row = p.y,
.cell_offset_x = p.cell_offset_x,
.cell_offset_y = p.cell_offset_y,
.source_x = p.source_x,

View File

@ -13,16 +13,16 @@ pub const Placement = struct {
image_id: u32,
/// The grid x/y where this placement is located.
x: u32,
y: u32,
x: i32,
y: i32,
z: i32,
/// The width/height of the placed image.
width: u32,
height: u32,
/// The offset in pixels from the top left of the cell. This is
/// clamped to the size of a cell.
/// The offset in pixels from the top left of the cell.
/// This is clamped to the size of a cell.
cell_offset_x: u32,
cell_offset_y: u32,
@ -441,7 +441,7 @@ pub const Image = union(enum) {
};
// Set our properties
desc.setProperty("pixelFormat", @intFromEnum(mtl.MTLPixelFormat.rgba8uint));
desc.setProperty("pixelFormat", @intFromEnum(mtl.MTLPixelFormat.rgba8unorm));
desc.setProperty("width", @as(c_ulong, @intCast(p.width)));
desc.setProperty("height", @as(c_ulong, @intCast(p.height)));

View File

@ -11,8 +11,8 @@ vbo: gl.Buffer,
pub const Input = extern struct {
/// vec2 grid_coord
grid_col: u16,
grid_row: u16,
grid_col: i32,
grid_row: i32,
/// vec2 cell_offset
cell_offset_x: u32 = 0,
@ -66,8 +66,8 @@ pub fn init() !ImageProgram {
var vbobind = try vbo.bind(.array);
defer vbobind.unbind();
var offset: usize = 0;
try vbobind.attributeAdvanced(0, 2, gl.c.GL_UNSIGNED_SHORT, false, @sizeOf(Input), offset);
offset += 2 * @sizeOf(u16);
try vbobind.attributeAdvanced(0, 2, gl.c.GL_INT, false, @sizeOf(Input), offset);
offset += 2 * @sizeOf(i32);
try vbobind.attributeAdvanced(1, 2, gl.c.GL_UNSIGNED_INT, false, @sizeOf(Input), offset);
offset += 2 * @sizeOf(u32);
try vbobind.attributeAdvanced(2, 4, gl.c.GL_UNSIGNED_INT, false, @sizeOf(Input), offset);

View File

@ -11,8 +11,8 @@ pub const Placement = struct {
image_id: u32,
/// The grid x/y where this placement is located.
x: u32,
y: u32,
x: i32,
y: i32,
z: i32,
/// The width/height of the placed image.

View File

@ -621,9 +621,6 @@ vertex ImageVertexOut image_vertex(
texture2d<uint> image [[texture(0)]],
constant Uniforms& uniforms [[buffer(1)]]
) {
// The size of the image in pixels
float2 image_size = float2(image.get_width(), image.get_height());
// Turn the image position into a vertex point depending on the
// vertex ID. Since we use instanced drawing, we have 4 vertices
// for each corner of the cell. We can use vertex ID to determine
@ -638,11 +635,12 @@ vertex ImageVertexOut image_vertex(
corner.x = (vid == 0 || vid == 1) ? 1.0f : 0.0f;
corner.y = (vid == 0 || vid == 3) ? 0.0f : 1.0f;
// The texture coordinates start at our source x/y, then add the width/height
// as enabled by our instance id, then normalize to [0, 1]
// The texture coordinates start at our source x/y
// and add the width/height depending on the corner.
//
// We don't need to normalize because we use pixel addressing for our sampler.
float2 tex_coord = in.source_rect.xy;
tex_coord += in.source_rect.zw * corner;
tex_coord /= image_size;
ImageVertexOut out;
@ -659,18 +657,19 @@ vertex ImageVertexOut image_vertex(
fragment float4 image_fragment(
ImageVertexOut in [[stage_in]],
texture2d<uint> image [[texture(0)]],
texture2d<float> image [[texture(0)]],
constant Uniforms& uniforms [[buffer(1)]]
) {
constexpr sampler textureSampler(address::clamp_to_edge, filter::linear);
constexpr sampler textureSampler(
coord::pixel,
address::clamp_to_edge,
filter::linear
);
// Ehhhhh our texture is in RGBA8Uint but our color attachment is
// BGRA8Unorm. So we need to convert it. We should really be converting
// our texture to BGRA8Unorm.
uint4 rgba = image.sample(textureSampler, in.tex_coord);
float4 rgba = image.sample(textureSampler, in.tex_coord);
return load_color(
uchar4(rgba),
uchar4(rgba * 255.0),
// We assume all images are sRGB regardless of the configured colorspace
// TODO: Maybe support wide gamut images?
false,

View File

@ -658,6 +658,86 @@ pub const ImageStorage = struct {
}
}
/// Calculates the size of this placement's image in pixels,
/// taking in to account the specified rows and columns.
pub fn calculatedSize(
self: Placement,
image: Image,
t: *const terminal.Terminal,
) struct {
width: u32,
height: u32,
} {
// Height / width of the image in px.
const width = if (self.source_width > 0) self.source_width else image.width;
const height = if (self.source_height > 0) self.source_height else image.height;
// If we don't have any specified cols or rows then the placement
// should be the native size of the image, and doesn't need to be
// re-scaled.
if (self.columns == 0 and self.rows == 0) return .{
.width = width,
.height = height,
};
// We calculate the size of a cell so that we can multiply
// it by the specified cols/rows to get the correct px size.
//
// We assume that the width is divided evenly by the column
// count and the height by the row count, because it should be.
const cell_width: u32 = t.width_px / t.cols;
const cell_height: u32 = t.height_px / t.rows;
const width_f64: f64 = @floatFromInt(width);
const height_f64: f64 = @floatFromInt(height);
// If we have a specified cols AND rows then we calculate
// the width and height from them directly, we don't need
// to adjust for aspect ratio.
if (self.columns > 0 and self.rows > 0) {
const calc_width = cell_width * self.columns;
const calc_height = cell_height * self.rows;
return .{
.width = calc_width,
.height = calc_height,
};
}
// Either the columns or the rows were specified, but not both,
// so we need to calculate the other one based on the aspect ratio.
// If only the columns were specified, we determine
// the height of the image based on the aspect ratio.
if (self.columns > 0) {
const aspect = height_f64 / width_f64;
const calc_width: u32 = cell_width * self.columns;
const calc_height: u32 = @intFromFloat(@round(
@as(f64, @floatFromInt(calc_width)) * aspect,
));
return .{
.width = calc_width,
.height = calc_height,
};
}
// Otherwise, only the rows were specified, so we
// determine the width based on the aspect ratio.
{
const aspect = width_f64 / height_f64;
const calc_height: u32 = cell_height * self.rows;
const calc_width: u32 = @intFromFloat(@round(
@as(f64, @floatFromInt(calc_height)) * aspect,
));
return .{
.width = calc_width,
.height = calc_height,
};
}
}
/// Returns the size in grid cells that this placement takes up.
pub fn gridSize(
self: Placement,
@ -667,60 +747,29 @@ pub const ImageStorage = struct {
cols: u32,
rows: u32,
} {
// If we have a specified columns and rows then this is trivial.
if (self.columns > 0 and self.rows > 0) return .{
.cols = self.columns,
.rows = self.rows,
};
// Calculate our cell size.
const terminal_width_f64: f64 = @floatFromInt(t.width_px);
const terminal_height_f64: f64 = @floatFromInt(t.height_px);
const grid_columns_f64: f64 = @floatFromInt(t.cols);
const grid_rows_f64: f64 = @floatFromInt(t.rows);
const cell_width_f64 = terminal_width_f64 / grid_columns_f64;
const cell_height_f64 = terminal_height_f64 / grid_rows_f64;
// Our image width
const width_px = if (self.source_width > 0) self.source_width else image.width;
const height_px = if (self.source_height > 0) self.source_height else image.height;
// Calculate our image size in grid cells
const width_f64: f64 = @floatFromInt(width_px);
const height_f64: f64 = @floatFromInt(height_px);
// If only columns is specified, calculate rows based on aspect ratio
if (self.columns > 0 and self.rows == 0) {
const cols_f64: f64 = @floatFromInt(self.columns);
const cols_px = cols_f64 * cell_width_f64;
const aspect_ratio = height_f64 / width_f64;
const rows_px = cols_px * aspect_ratio;
const rows_cells = rows_px / cell_height_f64;
// Otherwise we calculate the pixel size, divide by
// cell size, and round up to the nearest integer.
const calc_size = self.calculatedSize(image, t);
return .{
.cols = self.columns,
.rows = @intFromFloat(@ceil(rows_cells)),
};
}
// If only rows is specified, calculate columns based on aspect ratio
if (self.rows > 0 and self.columns == 0) {
const rows_f64: f64 = @floatFromInt(self.rows);
const rows_px = rows_f64 * cell_height_f64;
const aspect_ratio = width_f64 / height_f64;
const cols_px = rows_px * aspect_ratio;
const cols_cells = cols_px / cell_width_f64;
return .{
.cols = @intFromFloat(@ceil(cols_cells)),
.rows = self.rows,
};
}
const width_cells: u32 = @intFromFloat(@ceil(width_f64 / cell_width_f64));
const height_cells: u32 = @intFromFloat(@ceil(height_f64 / cell_height_f64));
return .{
.cols = width_cells,
.rows = height_cells,
.cols = std.math.divCeil(
u32,
calc_size.width + self.x_offset,
t.width_px / t.cols,
) catch 0,
.rows = std.math.divCeil(
u32,
calc_size.height + self.y_offset,
t.height_px / t.rows,
) catch 0,
};
// NOTE: Above `divCeil`s can only fail if the cell size is 0,
// in such a case it seems safe to return 0 for this.
}
/// Returns a selection of the entire rectangle this placement
@ -1269,36 +1318,42 @@ test "storage: aspect ratio calculation when only columns or rows specified" {
var t = try terminal.Terminal.init(alloc, .{ .cols = 100, .rows = 100 });
defer t.deinit(alloc);
t.width_px = 100;
t.height_px = 100;
t.width_px = 1000; // 10 px per col
t.height_px = 2000; // 20 px per row
// Case 1: Only columns specified
{
const image = Image{ .id = 1, .width = 4, .height = 2 };
const image = Image{ .id = 1, .width = 16, .height = 9 };
var placement = ImageStorage.Placement{
.location = .{ .virtual = {} },
.columns = 6,
.columns = 10,
.rows = 0,
};
const grid_size = placement.gridSize(image, &t);
// 6 columns * (2/4) = 3 rows
try testing.expectEqual(@as(u32, 6), grid_size.cols);
try testing.expectEqual(@as(u32, 3), grid_size.rows);
// Image is 16x9, set to a width of 10 columns, at 10px per column
// that's 100px width. 100px * (9 / 16) = 56.25, which should round
// to a height of 56px.
const calc_size = placement.calculatedSize(image, &t);
try testing.expectEqual(@as(u32, 100), calc_size.width);
try testing.expectEqual(@as(u32, 56), calc_size.height);
}
// Case 2: Only rows specified
{
const image = Image{ .id = 2, .width = 2, .height = 4 };
const image = Image{ .id = 2, .width = 16, .height = 9 };
var placement = ImageStorage.Placement{
.location = .{ .virtual = {} },
.columns = 0,
.rows = 6,
.rows = 5,
};
const grid_size = placement.gridSize(image, &t);
// 6 rows * (2/4) = 3 columns
try testing.expectEqual(@as(u32, 3), grid_size.cols);
try testing.expectEqual(@as(u32, 6), grid_size.rows);
// Image is 16x9, set to a height of 5 rows, at 20px per row that's
// 100px height. 100px * (16 / 9) = 177.77..., which should round to
// a width of 178px.
const calc_size = placement.calculatedSize(image, &t);
try testing.expectEqual(@as(u32, 178), calc_size.width);
try testing.expectEqual(@as(u32, 100), calc_size.height);
}
}