From bfb8062919297664e7e4f29a89d040952da1eaae Mon Sep 17 00:00:00 2001 From: James Panayis Date: Mon, 20 Jul 2026 12:48:32 +0100 Subject: [PATCH 1/2] Add failing test for save-overlap option Run with `zig test src/test_gold_all.zig --test-filter "disk save with overlap" --test-cmd timeout --test-cmd 30s --test-cmd-bin` because the issue causes the code to hang --- src/tests/test_gold_multicamera.zig | 221 ++++++++++++++++++---------- 1 file changed, 140 insertions(+), 81 deletions(-) diff --git a/src/tests/test_gold_multicamera.zig b/src/tests/test_gold_multicamera.zig index d4b7a91e..64c463f8 100644 --- a/src/tests/test_gold_multicamera.zig +++ b/src/tests/test_gold_multicamera.zig @@ -570,14 +570,24 @@ test "Multicamera grouped render groups match reference across scheduler modes" ); } -test "Multicamera memory matches both" { +fn runMulticameraSaveCase( + data_dir: []const u8, + pixel_num: [2]u32, + save_strategy: riley.SaveStrategy, + overlap: bool, +) !void { var gpa: std.heap.DebugAllocator(.{}) = .init; const allocator = gpa.allocator(); defer _ = gpa.deinit(); const io = std.testing.io; - const pixel_num = [_]u32{ 320, 200 }; - const data_dir = "data/bench/tri3_sphere200"; + const out_dir: ?[]const u8 = if (save_strategy == .disk) + if (overlap) "temp-tests/save-overlap-on" else "temp-tests/save-overlap-off" + else + null; + const cwd = std.Io.Dir.cwd(); + if (out_dir) |path| cwd.deleteTree(io, path) catch {}; + defer if (out_dir) |path| cwd.deleteTree(io, path) catch {}; var arena = std.heap.ArenaAllocator.init(allocator); defer arena.deinit(); @@ -595,7 +605,6 @@ test "Multicamera memory matches both" { aa, &[_][]const u8{ data_dir, "field.csv" }, ); - const sim_data = try meshio.loadSimData( aa, io, @@ -611,11 +620,42 @@ test "Multicamera memory matches both" { 1, true, ); - const camera_a = try orch.initCameraForCoords(aa, &sim_data.coords, pixel_num, 1.0); + const camera_a = try orch.initCameraForCoords( + aa, + &sim_data.coords, + pixel_num, + 1.0, + ); defer camera_a.deinit(aa); - const camera_b = try orch.initCameraForCoords(aa, &sim_data.coords, pixel_num, 1.0); + const camera_b = try orch.initCameraForCoords( + aa, + &sim_data.coords, + pixel_num, + 1.0, + ); defer camera_b.deinit(aa); - + const camera_inputs = [_]CameraInput{ + .{ + .pixels_num = camera_a.pixels_num, + .pixels_size = camera_a.pixels_size, + .pos_world = camera_a.pos_world, + .rot_world = camera_a.rot_world, + .roi_cent_world = camera_a.roi_cent_world, + .focal_length = camera_a.focal_length, + .sub_sample = camera_a.sub_sample, + .distortion = camera_a.distortion, + }, + .{ + .pixels_num = camera_b.pixels_num, + .pixels_size = camera_b.pixels_size, + .pos_world = camera_b.pos_world, + .rot_world = camera_b.rot_world, + .roi_cent_world = camera_b.roi_cent_world, + .focal_length = camera_b.focal_length, + .sub_sample = camera_b.sub_sample, + .distortion = camera_b.distortion, + }, + }; const mesh_input = mo.MeshInput{ .mesh_type = .tri3, .coords = sim_data.coords, @@ -632,91 +672,110 @@ test "Multicamera memory matches both" { }, }; - var memory_config = tcfg.getRasterConfig(.testing); - memory_config.save_strategy = .memory; - memory_config.report = .off; - - var both_config = memory_config; - both_config.save_strategy = .both; - - const memory_render_groups = [_]riley.RenderGroupSpec{ - .{ .io = io, .workers = @max(@as(u16, 1), memory_config.total_threads) }, + var reference_config = tcfg.getRasterConfig(.testing); + reference_config.save_strategy = .memory; + reference_config.report = .off; + const render_groups = [_]riley.RenderGroupSpec{ + .{ .io = io, .workers = @max(@as(u16, 1), reference_config.total_threads) }, }; - const memory = (try riley.raster( + const reference = (try riley.raster( aa, - &memory_render_groups, - &[_]CameraInput{ - CameraInput{ - .pixels_num = camera_a.pixels_num, - .pixels_size = camera_a.pixels_size, - .pos_world = camera_a.pos_world, - .rot_world = camera_a.rot_world, - .roi_cent_world = camera_a.roi_cent_world, - .focal_length = camera_a.focal_length, - .sub_sample = camera_a.sub_sample, - .distortion = camera_a.distortion, - }, - CameraInput{ - .pixels_num = camera_b.pixels_num, - .pixels_size = camera_b.pixels_size, - .pos_world = camera_b.pos_world, - .rot_world = camera_b.rot_world, - .roi_cent_world = camera_b.roi_cent_world, - .focal_length = camera_b.focal_length, - .sub_sample = camera_b.sub_sample, - .distortion = camera_b.distortion, - }, - }, + &render_groups, + &camera_inputs, &[_]mo.MeshInput{mesh_input}, - memory_config, + reference_config, null, )) orelse return error.NoResult; - defer aa.free(memory.slice); + defer aa.free(reference.slice); - const both_render_groups = [_]riley.RenderGroupSpec{ - .{ .io = io, .workers = @max(@as(u16, 1), both_config.total_threads) }, - }; - const both = (try riley.raster( - aa, - &both_render_groups, - &[_]CameraInput{ - CameraInput{ - .pixels_num = camera_a.pixels_num, - .pixels_size = camera_a.pixels_size, - .pos_world = camera_a.pos_world, - .rot_world = camera_a.rot_world, - .roi_cent_world = camera_a.roi_cent_world, - .focal_length = camera_a.focal_length, - .sub_sample = camera_a.sub_sample, - .distortion = camera_a.distortion, - }, - CameraInput{ - .pixels_num = camera_b.pixels_num, - .pixels_size = camera_b.pixels_size, - .pos_world = camera_b.pos_world, - .rot_world = camera_b.rot_world, - .roi_cent_world = camera_b.roi_cent_world, - .focal_length = camera_b.focal_length, - .sub_sample = camera_b.sub_sample, - .distortion = camera_b.distortion, + var target_config = reference_config; + target_config.save_strategy = save_strategy; + target_config.disk_save_overlap = overlap; + if (save_strategy == .disk) { + target_config.image_save_opts = &[_]iio.ImageSaveOpts{ + .{ + .format = .csv, + .bits = null, + .scaling = .none, + .channels = 1, }, - }, + }; + } + const result = try riley.raster( + aa, + &render_groups, + &camera_inputs, &[_]mo.MeshInput{mesh_input}, - both_config, - null, - )) orelse return error.NoResult; - defer aa.free(both.slice); - - try std.testing.expect(ndarray.matchArrayDims(F, &memory, &both)); - for (0..memory.slice.len) |ii| { - try std.testing.expectApproxEqAbs( - memory.slice[ii], - both.slice[ii], - duplicate_abs_tol, - ); + target_config, + out_dir, + ); + defer if (result) |image| aa.free(image.slice); + + switch (save_strategy) { + .both => { + const both = result orelse return error.NoResult; + try std.testing.expect(ndarray.matchArrayDims(F, &reference, &both)); + for (0..reference.slice.len) |ii| { + try std.testing.expectApproxEqAbs( + reference.slice[ii], + both.slice[ii], + duplicate_abs_tol, + ); + } + }, + .disk => { + try std.testing.expect(result == null); + for (0..camera_inputs.len) |camera_idx| { + const saved_path = try std.fmt.allocPrint( + aa, + "{s}/cam{d}_frame0_field0.csv", + .{ out_dir.?, camera_idx }, + ); + try testcommon.compareNDArrayToGold( + aa, + io, + &reference, + camera_idx, + 0, + 0, + 1, + saved_path, + duplicate_rel_tol, + duplicate_abs_tol, + ); + } + }, + else => unreachable, } } +test "Multicamera memory matches both" { + try runMulticameraSaveCase( + "data/bench/tri3_sphere200", + .{ 320, 200 }, + .both, + false, + ); +} + +test "disk save without overlap" { + try runMulticameraSaveCase( + "data/min/tri3_sphere200", + .{ 160, 100 }, + .disk, + false, + ); +} + +test "disk save with overlap" { + try runMulticameraSaveCase( + "data/min/tri3_sphere200", + .{ 160, 100 }, + .disk, + true, + ); +} + test "Sphere200 multicamera gold tests" { if (!simd_on) { std.debug.print( From b49c96eef2db11272f9c98428519506a689490f1 Mon Sep 17 00:00:00 2001 From: James Panayis Date: Mon, 20 Jul 2026 12:55:53 +0100 Subject: [PATCH 2/2] Fix disk-save overlap coordinator lifetime issue Also Heap-allocate shared state and harden cleanup, synchronization, and slot-release paths --- src/riley/zig/saveoverlap.zig | 66 +++++++++++++++++++++++------------ 1 file changed, 43 insertions(+), 23 deletions(-) diff --git a/src/riley/zig/saveoverlap.zig b/src/riley/zig/saveoverlap.zig index 17aa9559..a23c9192 100644 --- a/src/riley/zig/saveoverlap.zig +++ b/src/riley/zig/saveoverlap.zig @@ -102,7 +102,7 @@ pub const SaveOverlap = struct { enabled_flag: bool, arena: std.heap.ArenaAllocator, slot_buff: ?SaveSlotBuff = null, - coordinator: ?SaveCoordinator = null, + coordinator: ?*SaveCoordinator = null, thread: ?std.Thread = null, pub fn initMaybe( @@ -122,6 +122,7 @@ pub const SaveOverlap = struct { }; if (!is_enabled) return session; + errdefer session.arena.deinit(); session.slot_buff = try initSaveSlots( session.arena.allocator(), @@ -129,14 +130,18 @@ pub const SaveOverlap = struct { num_fields, @max(@as(usize, 1), config.save_frame_buff_count), ); - session.coordinator = .{ .slots = session.slot_buff.?.slots }; + + const coordinator = try outer_alloc.create(SaveCoordinator); + errdefer outer_alloc.destroy(coordinator); + coordinator.* = .{ .slots = session.slot_buff.?.slots }; + session.coordinator = coordinator; session.thread = try std.Thread.spawn( .{}, saveWorkerLoop, .{ outer_alloc, save_io, - &session.coordinator.?, + coordinator, config, }, ); @@ -144,22 +149,22 @@ pub const SaveOverlap = struct { } pub fn deinit(self: *SaveOverlap) void { - if (self.enabled_flag) { - if (self.coordinator) |*coordinator| { - coordinator.mutex.lockUncancelable(self.save_io); - coordinator.done_submitting = true; - coordinator.ready_cond.broadcast(self.save_io); - coordinator.free_cond.broadcast(self.save_io); - coordinator.mutex.unlock(self.save_io); - } + if (self.coordinator) |coordinator| { + coordinator.mutex.lockUncancelable(self.save_io); + coordinator.done_submitting = true; + coordinator.ready_cond.broadcast(self.save_io); + coordinator.free_cond.broadcast(self.save_io); + coordinator.mutex.unlock(self.save_io); + if (self.thread) |thread| { thread.join(); + self.thread = null; } - if (self.coordinator) |*coordinator| { - for (coordinator.slots) |*slot| { - slot.resetReportStorage(self.outer_alloc, self.config); - } + for (coordinator.slots) |*slot| { + slot.resetReportStorage(self.outer_alloc, self.config); } + self.outer_alloc.destroy(coordinator); + self.coordinator = null; } self.arena.deinit(); } @@ -172,9 +177,9 @@ pub const SaveOverlap = struct { self: *SaveOverlap, io: std.Io, ) !*SaveSlot { - std.debug.assert(self.coordinator != null); - const slot_idx = try acquireSaveSlot(io, &self.coordinator.?); - return &self.coordinator.?.slots[slot_idx]; + const coordinator = self.coordinator orelse unreachable; + const slot_idx = try acquireSaveSlot(io, coordinator); + return &coordinator.slots[slot_idx]; } pub fn publishSlot( @@ -185,10 +190,10 @@ pub const SaveOverlap = struct { report_storage: *FrameReportStorage, prep_meshes: []const mo.MeshPrepared, ) !void { - std.debug.assert(self.coordinator != null); + const coordinator = self.coordinator orelse unreachable; try publishRenderedSlot( io, - &self.coordinator.?, + coordinator, slot, meta, report_storage, @@ -196,11 +201,25 @@ pub const SaveOverlap = struct { ); } + fn releaseSlot( + self: *SaveOverlap, + io: std.Io, + slot: *SaveSlot, + ) void { + const coordinator = self.coordinator orelse unreachable; + coordinator.mutex.lockUncancelable(io); + defer coordinator.mutex.unlock(io); + slot.resetReportStorage(self.outer_alloc, self.config); + slot.state = .free; + coordinator.free_cond.signal(io); + } + pub fn checkError(self: *SaveOverlap) !void { if (!self.enabled_flag) return; - if (self.coordinator) |*coordinator| { - if (coordinator.first_err) |err| return err; - } + const coordinator = self.coordinator orelse unreachable; + coordinator.mutex.lockUncancelable(self.save_io); + defer coordinator.mutex.unlock(self.save_io); + if (coordinator.first_err) |err| return err; } pub fn runRasterStageAndQueue( @@ -212,6 +231,7 @@ pub const SaveOverlap = struct { comptime raster_stage_fn: anytype, ) !void { const slot = try self.acquireSlot(io); + errdefer self.releaseSlot(io, slot); job.desc.save_slot = slot; try raster_stage_fn( outer_alloc,