From e16b3bb4c33985c961ec80f75e93b9d1a3ebdda5 Mon Sep 17 00:00:00 2001 From: Nathan Clack Date: Tue, 25 Aug 2026 16:17:16 +0000 Subject: [PATCH 1/4] Give each plate field its own pool slots --- src/hcs/hcs.c | 39 +++++++-------- src/ngff/ngff_multiscale.c | 48 ++++++++++++++----- src/ngff/ngff_multiscale.h | 14 ++++-- tests/test_hcs.c | 97 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 159 insertions(+), 39 deletions(-) diff --git a/src/hcs/hcs.c b/src/hcs/hcs.c index 7f1e8941..5ccea726 100644 --- a/src/hcs/hcs.c +++ b/src/hcs/hcs.c @@ -1,7 +1,6 @@ #include "hcs.h" #include "defs.limits.h" #include "hcs/hcs_metadata.h" -#include "lod/lod_plan.h" #include "ngff/ngff_multiscale.h" #include "util/prelude.h" #include "util/strbuf.h" @@ -167,25 +166,15 @@ hcs_plate_create(struct store* store, const struct hcs_plate_config* cfg) CHECK(Fail, rn_len < 64); // bounded by hcs_plate.row_names buffer } - // Compute pool size: max shard_inner_count across all LOD levels - struct lod_plan plan = { 0 }; - int max_lev = cfg->fov.nlod > 0 ? cfg->fov.nlod : LOD_MAX_LEVELS; - CHECK(Fail, - lod_plan_init_from_dims( - &plan, cfg->fov.dimensions, cfg->fov.rank, max_lev, 0) == 0); - - uint8_t na = dims_n_append(cfg->fov.dimensions, cfg->fov.rank); - uint64_t total_slots = 0; - for (int lv = 0; lv < plan.levels.nlod; ++lv) { - uint64_t sic = 1; - for (int d = na; d < cfg->fov.rank; ++d) - sic *= plan.levels.level[lv].dim[d].shard_count; - total_slots += sic; - } - lod_plan_free(&plan); - CHECK(Fail, total_slots > 0); + // The whole plate shares one pool so that it shares one io queue, so the + // pool has to be wide enough to give every field of view its own slots. + uint64_t slots_per_fov = ngff_multiscale_slot_count(&cfg->fov); + CHECK(Fail, slots_per_fov > 0); - struct shard_pool* pool = store->create_pool(store, total_slots); + uint64_t fov_count = + (uint64_t)cfg->rows * (uint64_t)cfg->cols * (uint64_t)cfg->field_count; + struct shard_pool* pool = + store->create_pool(store, fov_count * slots_per_fov); CHECK(Fail, pool); struct hcs_plate* p = (struct hcs_plate*)calloc(1, sizeof(*p)); @@ -263,10 +252,14 @@ hcs_plate_create(struct store* store, const struct hcs_plate_config* cfg) int fp_rc = strbuf_appendf(&fov_prefix, "%s/%c/%d/%d", cfg->name, rc, c + 1, f); int idx = fov_index(p, r, c, f); - p->fovs[idx] = fp_rc == 0 - ? ngff_multiscale_create_with_pool( - store, pool, strbuf_cstr(&fov_prefix), &cfg->fov) - : NULL; + p->fovs[idx] = + fp_rc == 0 + ? ngff_multiscale_create_with_pool(store, + pool, + (uint64_t)idx * slots_per_fov, + strbuf_cstr(&fov_prefix), + &cfg->fov) + : NULL; strbuf_free(&fov_prefix); CHECK(Fail_fovs, p->fovs[idx]); } diff --git a/src/ngff/ngff_multiscale.c b/src/ngff/ngff_multiscale.c index 62e62dc3..f31dee4c 100644 --- a/src/ngff/ngff_multiscale.c +++ b/src/ngff/ngff_multiscale.c @@ -200,6 +200,7 @@ ngff_multiscale_flush_fn(struct shard_sink* self) static struct ngff_multiscale* ngff_multiscale_init(struct store* store, struct shard_pool* pool, + uint64_t slot_base, const char* prefix, const struct ngff_multiscale_config* cfg, struct lod_plan* plan) @@ -237,7 +238,6 @@ ngff_multiscale_init(struct store* store, (struct zarr_array**)calloc((size_t)plan->levels.nlod, sizeof(void*)); CHECK(Fail_ms, ms->levels); - uint64_t slot_base = 0; for (int lv = 0; lv < plan->levels.nlod; ++lv) { struct dimension lv_dims[MAX_ZARR_RANK]; @@ -303,9 +303,38 @@ ngff_multiscale_init(struct store* store, // --- Private API --- +uint64_t +ngff_multiscale_slot_count(const struct ngff_multiscale_config* cfg) +{ + CHECK(Fail, cfg); + CHECK(Fail, cfg->rank > 0 && cfg->rank <= MAX_ZARR_RANK); + CHECK(Fail, cfg->dimensions); + + struct lod_plan plan = { 0 }; + int max_lev = cfg->nlod > 0 ? cfg->nlod : LOD_MAX_LEVELS; + CHECK(Fail, + lod_plan_init_from_dims( + &plan, cfg->dimensions, cfg->rank, max_lev, 0) == 0); + + uint8_t na = dims_n_append(cfg->dimensions, cfg->rank); + uint64_t total = 0; + for (int lv = 0; lv < plan.levels.nlod; ++lv) { + uint64_t sic = 1; + for (int d = na; d < cfg->rank; ++d) + sic *= plan.levels.level[lv].dim[d].shard_count; + total += sic; + } + lod_plan_free(&plan); + return total; + +Fail: + return 0; +} + struct ngff_multiscale* ngff_multiscale_create_with_pool(struct store* store, struct shard_pool* pool, + uint64_t slot_base, const char* prefix, const struct ngff_multiscale_config* cfg) { @@ -321,7 +350,7 @@ ngff_multiscale_create_with_pool(struct store* store, lod_plan_init_from_dims( &plan, cfg->dimensions, cfg->rank, max_lev, 0) == 0); - return ngff_multiscale_init(store, pool, prefix, cfg, &plan); + return ngff_multiscale_init(store, pool, slot_base, prefix, cfg, &plan); Fail: return NULL; @@ -339,28 +368,21 @@ ngff_multiscale_create(struct store* store, CHECK(Fail, cfg->rank > 0 && cfg->rank <= MAX_ZARR_RANK); CHECK(Fail, cfg->dimensions); + uint64_t total_slots = ngff_multiscale_slot_count(cfg); + CHECK(Fail, total_slots > 0); + struct lod_plan plan = { 0 }; int max_lev = cfg->nlod > 0 ? cfg->nlod : LOD_MAX_LEVELS; CHECK(Fail, lod_plan_init_from_dims( &plan, cfg->dimensions, cfg->rank, max_lev, 0) == 0); - uint8_t na = dims_n_append(cfg->dimensions, cfg->rank); - uint64_t total_slots = 0; - for (int lv = 0; lv < plan.levels.nlod; ++lv) { - uint64_t sic = 1; - for (int d = na; d < cfg->rank; ++d) - sic *= plan.levels.level[lv].dim[d].shard_count; - total_slots += sic; - } - CHECK(Fail_plan, total_slots > 0); - struct shard_pool* pool = store->create_pool(store, total_slots); CHECK(Fail_plan, pool); // plan ownership transfers to ngff_multiscale_init struct ngff_multiscale* ms = - ngff_multiscale_init(store, pool, prefix, cfg, &plan); + ngff_multiscale_init(store, pool, 0, prefix, cfg, &plan); if (!ms) { shard_pool_destroy(pool); return NULL; diff --git a/src/ngff/ngff_multiscale.h b/src/ngff/ngff_multiscale.h index 14cd855e..395d11b2 100644 --- a/src/ngff/ngff_multiscale.h +++ b/src/ngff/ngff_multiscale.h @@ -6,12 +6,20 @@ #include "zarr/shard_pool.h" #include "zarr/store.h" -// Private: create a multiscale sink that borrows an existing pool. -// The caller owns the pool lifetime — ngff_multiscale_destroy will NOT -// destroy it. +// Pool slots one multiscale needs, summed over its levels. A caller sharing +// one pool between several multiscales spaces their slot ranges by this much, +// because two multiscales on the same slot overwrite each other's shard files. +// Returns 0 if the config is unusable. +uint64_t +ngff_multiscale_slot_count(const struct ngff_multiscale_config* cfg); + +// Private: create a multiscale sink that borrows an existing pool, using the +// slots at slot_base and above. The caller owns the pool lifetime — +// ngff_multiscale_destroy will NOT destroy it. struct ngff_multiscale* ngff_multiscale_create_with_pool(struct store* store, struct shard_pool* pool, + uint64_t slot_base, const char* prefix, const struct ngff_multiscale_config* cfg); diff --git a/tests/test_hcs.c b/tests/test_hcs.c index f05acaab..b73edc6f 100644 --- a/tests/test_hcs.c +++ b/tests/test_hcs.c @@ -306,6 +306,102 @@ test_plate_metadata_with_mask(void) return 1; } +// --- Test: two fields open at the same time keep their own shard files --- + +static int +test_hcs_two_fields_open_together(void) +{ + log_info("=== test_hcs_two_fields_open_together ==="); + + struct store* store = store_fs_create(tmpdir, 0); + CHECK(Fail, store); + store->mkdirs(store, "."); + + struct dimension dims[] = { + { .size = 16, + .chunk_size = 16, + .chunks_per_shard = 1, + .name = "y", + .downsample = 1, + .storage_position = 0 }, + { .size = 16, + .chunk_size = 16, + .chunks_per_shard = 1, + .name = "x", + .downsample = 1, + .storage_position = 1 }, + }; + + struct hcs_plate_config cfg = { + .name = "together", + .rows = 1, + .cols = 1, + .field_count = 2, + .fov = { + .data_type = dtype_u8, + .rank = 2, + .dimensions = dims, + .nlod = 1, + }, + }; + + struct hcs_plate* plate = hcs_plate_create(store, &cfg); + CHECK(Fail2, plate); + + struct shard_sink* first = hcs_plate_fov_sink(plate, 0, 0, 0); + struct shard_sink* second = hcs_plate_fov_sink(plate, 0, 0, 1); + CHECK(Fail3, first && second); + + struct shard_writer* first_shard = first->open(first, 0, 0); + CHECK(Fail3, first_shard); + struct shard_writer* second_shard = second->open(second, 0, 0); + CHECK(Fail3, second_shard); + CHECK(Fail3, first_shard != second_shard); + + uint8_t first_data[256], second_data[256]; + memset(first_data, 0x11, sizeof(first_data)); + memset(second_data, 0x22, sizeof(second_data)); + + CHECK(Fail3, + first_shard->write( + first_shard, 0, first_data, first_data + sizeof(first_data)) == 0); + CHECK(Fail3, + second_shard->write( + second_shard, 0, second_data, second_data + sizeof(second_data)) == + 0); + CHECK(Fail3, first_shard->finalize(first_shard) == 0); + CHECK(Fail3, second_shard->finalize(second_shard) == 0); + + // Destroying the plate drains the writes, so the files are complete below. + hcs_plate_destroy(plate); + plate = NULL; + + char path[4096], buf[8192]; + size_t len; + + snprintf(path, sizeof(path), "%s/together/A/1/0/0/c/0/0", tmpdir); + CHECK(Fail3, read_file(path, buf, sizeof(buf), &len) == 0); + CHECK(Fail3, len == sizeof(first_data)); + CHECK(Fail3, memcmp(buf, first_data, sizeof(first_data)) == 0); + + snprintf(path, sizeof(path), "%s/together/A/1/1/0/c/0/0", tmpdir); + CHECK(Fail3, read_file(path, buf, sizeof(buf), &len) == 0); + CHECK(Fail3, len == sizeof(second_data)); + CHECK(Fail3, memcmp(buf, second_data, sizeof(second_data)) == 0); + + store_destroy(store); + log_info(" PASS"); + return 0; + +Fail3: + hcs_plate_destroy(plate); +Fail2: + store_destroy(store); +Fail: + log_error(" FAIL"); + return 1; +} + int main(void) { @@ -319,6 +415,7 @@ main(void) err |= test_plate_metadata_with_mask(); err |= test_hcs_plate_create(); err |= test_hcs_well_mask(); + err |= test_hcs_two_fields_open_together(); test_tmpdir_remove(tmpdir); From ab0885c4e01236ef2544dacfa09ba21c2118c285 Mon Sep 17 00:00:00 2001 From: Nathan Clack Date: Tue, 25 Aug 2026 16:47:15 +0000 Subject: [PATCH 2/4] Count slots the way levels are created --- src/hcs/hcs.c | 4 +- src/ngff/ngff_multiscale.c | 59 +++++++++++++++++---------- src/ngff/ngff_multiscale.h | 8 ++-- tests/test_hcs.c | 82 ++++++++++++++++++++++---------------- 4 files changed, 90 insertions(+), 63 deletions(-) diff --git a/src/hcs/hcs.c b/src/hcs/hcs.c index 5ccea726..4181fe3a 100644 --- a/src/hcs/hcs.c +++ b/src/hcs/hcs.c @@ -166,8 +166,8 @@ hcs_plate_create(struct store* store, const struct hcs_plate_config* cfg) CHECK(Fail, rn_len < 64); // bounded by hcs_plate.row_names buffer } - // The whole plate shares one pool so that it shares one io queue, so the - // pool has to be wide enough to give every field of view its own slots. + // One pool keeps the whole plate on one io queue, so the pool has to be wide + // enough for every field of view to have its own slots. uint64_t slots_per_fov = ngff_multiscale_slot_count(&cfg->fov); CHECK(Fail, slots_per_fov > 0); diff --git a/src/ngff/ngff_multiscale.c b/src/ngff/ngff_multiscale.c index f31dee4c..b0c164fc 100644 --- a/src/ngff/ngff_multiscale.c +++ b/src/ngff/ngff_multiscale.c @@ -195,6 +195,38 @@ ngff_multiscale_flush_fn(struct shard_sink* self) // --- Shared create logic --- +static void +level_dimensions(struct dimension* out, + const struct ngff_multiscale_config* cfg, + const struct lod_plan* plan, + int level) +{ + const struct level_dims* ld = &plan->levels.level[level]; + for (int d = 0; d < cfg->rank; ++d) { + out[d] = cfg->dimensions[d]; + out[d].size = + (d == 0 && cfg->dimensions[0].size == 0) ? 0 : ld->dim[d].size; + out[d].chunk_size = ld->dim[d].chunk_size; + out[d].chunks_per_shard = ld->dim[d].chunks_per_shard; + } +} + +// Slot ranges are spaced by this count, so it has to walk the levels the same +// way the create loop below does. +static uint64_t +plan_slot_count(const struct ngff_multiscale_config* cfg, + const struct lod_plan* plan) +{ + uint64_t total = 0; + for (int lv = 0; lv < plan->levels.nlod; ++lv) { + struct dimension lv_dims[MAX_ZARR_RANK]; + uint64_t sc[MAX_ZARR_RANK], cps[MAX_ZARR_RANK]; + level_dimensions(lv_dims, cfg, plan, lv); + total += dims_compute_shard_geometry(lv_dims, cfg->rank, sc, cps); + } + return total; +} + // Shared init: caller provides a pre-computed LOD plan. // The plan is consumed (freed on success, freed on failure). static struct ngff_multiscale* @@ -240,17 +272,7 @@ ngff_multiscale_init(struct store* store, for (int lv = 0; lv < plan->levels.nlod; ++lv) { struct dimension lv_dims[MAX_ZARR_RANK]; - - for (int d = 0; d < cfg->rank; ++d) { - lv_dims[d] = cfg->dimensions[d]; - if (d == 0 && cfg->dimensions[0].size == 0) - lv_dims[d].size = 0; - else - lv_dims[d].size = plan->levels.level[lv].dim[d].size; - lv_dims[d].chunk_size = plan->levels.level[lv].dim[d].chunk_size; - lv_dims[d].chunks_per_shard = - plan->levels.level[lv].dim[d].chunks_per_shard; - } + level_dimensions(lv_dims, cfg, plan, lv); struct strbuf level_prefix = { 0 }; int lp_rc = (prefix && prefix[0]) @@ -316,14 +338,7 @@ ngff_multiscale_slot_count(const struct ngff_multiscale_config* cfg) lod_plan_init_from_dims( &plan, cfg->dimensions, cfg->rank, max_lev, 0) == 0); - uint8_t na = dims_n_append(cfg->dimensions, cfg->rank); - uint64_t total = 0; - for (int lv = 0; lv < plan.levels.nlod; ++lv) { - uint64_t sic = 1; - for (int d = na; d < cfg->rank; ++d) - sic *= plan.levels.level[lv].dim[d].shard_count; - total += sic; - } + uint64_t total = plan_slot_count(cfg, &plan); lod_plan_free(&plan); return total; @@ -368,15 +383,15 @@ ngff_multiscale_create(struct store* store, CHECK(Fail, cfg->rank > 0 && cfg->rank <= MAX_ZARR_RANK); CHECK(Fail, cfg->dimensions); - uint64_t total_slots = ngff_multiscale_slot_count(cfg); - CHECK(Fail, total_slots > 0); - struct lod_plan plan = { 0 }; int max_lev = cfg->nlod > 0 ? cfg->nlod : LOD_MAX_LEVELS; CHECK(Fail, lod_plan_init_from_dims( &plan, cfg->dimensions, cfg->rank, max_lev, 0) == 0); + uint64_t total_slots = plan_slot_count(cfg, &plan); + CHECK(Fail_plan, total_slots > 0); + struct shard_pool* pool = store->create_pool(store, total_slots); CHECK(Fail_plan, pool); diff --git a/src/ngff/ngff_multiscale.h b/src/ngff/ngff_multiscale.h index 395d11b2..0f85958f 100644 --- a/src/ngff/ngff_multiscale.h +++ b/src/ngff/ngff_multiscale.h @@ -6,10 +6,10 @@ #include "zarr/shard_pool.h" #include "zarr/store.h" -// Pool slots one multiscale needs, summed over its levels. A caller sharing -// one pool between several multiscales spaces their slot ranges by this much, -// because two multiscales on the same slot overwrite each other's shard files. -// Returns 0 if the config is unusable. +// Count the pool slots one multiscale needs across its levels. Several +// multiscales sharing a pool space their slot ranges by this much, because two +// multiscales on one slot overwrite each other's shard files. Returns 0 when +// the config cannot be used. uint64_t ngff_multiscale_slot_count(const struct ngff_multiscale_config* cfg); diff --git a/tests/test_hcs.c b/tests/test_hcs.c index b73edc6f..055b0fec 100644 --- a/tests/test_hcs.c +++ b/tests/test_hcs.c @@ -317,14 +317,16 @@ test_hcs_two_fields_open_together(void) CHECK(Fail, store); store->mkdirs(store, "."); + // A field spans several slots here, two shards at level 0 and one at level 1, + // so a wrong stride between fields lands on another field's slot. struct dimension dims[] = { - { .size = 16, + { .size = 32, .chunk_size = 16, .chunks_per_shard = 1, .name = "y", .downsample = 1, .storage_position = 0 }, - { .size = 16, + { .size = 32, .chunk_size = 16, .chunks_per_shard = 1, .name = "x", @@ -341,36 +343,43 @@ test_hcs_two_fields_open_together(void) .data_type = dtype_u8, .rank = 2, .dimensions = dims, - .nlod = 1, }, }; struct hcs_plate* plate = hcs_plate_create(store, &cfg); CHECK(Fail2, plate); - struct shard_sink* first = hcs_plate_fov_sink(plate, 0, 0, 0); - struct shard_sink* second = hcs_plate_fov_sink(plate, 0, 0, 1); - CHECK(Fail3, first && second); - - struct shard_writer* first_shard = first->open(first, 0, 0); - CHECK(Fail3, first_shard); - struct shard_writer* second_shard = second->open(second, 0, 0); - CHECK(Fail3, second_shard); - CHECK(Fail3, first_shard != second_shard); - - uint8_t first_data[256], second_data[256]; - memset(first_data, 0x11, sizeof(first_data)); - memset(second_data, 0x22, sizeof(second_data)); - - CHECK(Fail3, - first_shard->write( - first_shard, 0, first_data, first_data + sizeof(first_data)) == 0); - CHECK(Fail3, - second_shard->write( - second_shard, 0, second_data, second_data + sizeof(second_data)) == - 0); - CHECK(Fail3, first_shard->finalize(first_shard) == 0); - CHECK(Fail3, second_shard->finalize(second_shard) == 0); + const struct + { + int field; + int level; + int shard; + } opens[] = { + { 0, 0, 0 }, { 0, 0, 1 }, { 0, 1, 0 }, + { 1, 0, 0 }, { 1, 0, 1 }, { 1, 1, 0 }, + }; + const int nopens = (int)(sizeof(opens) / sizeof(opens[0])); + + struct shard_writer* writers[sizeof(opens) / sizeof(opens[0])] = { 0 }; + uint8_t data[sizeof(opens) / sizeof(opens[0])][256]; + + for (int i = 0; i < nopens; ++i) { + struct shard_sink* sink = hcs_plate_fov_sink(plate, 0, 0, opens[i].field); + CHECK(Fail3, sink); + writers[i] = + sink->open(sink, (uint8_t)opens[i].level, (uint64_t)opens[i].shard); + CHECK(Fail3, writers[i]); + for (int j = 0; j < i; ++j) + CHECK(Fail3, writers[i] != writers[j]); + memset(data[i], (uint8_t)(0x10 + i), sizeof(data[i])); + } + + for (int i = 0; i < nopens; ++i) + CHECK(Fail3, + writers[i]->write( + writers[i], 0, data[i], data[i] + sizeof(data[i])) == 0); + for (int i = 0; i < nopens; ++i) + CHECK(Fail3, writers[i]->finalize(writers[i]) == 0); // Destroying the plate drains the writes, so the files are complete below. hcs_plate_destroy(plate); @@ -379,15 +388,18 @@ test_hcs_two_fields_open_together(void) char path[4096], buf[8192]; size_t len; - snprintf(path, sizeof(path), "%s/together/A/1/0/0/c/0/0", tmpdir); - CHECK(Fail3, read_file(path, buf, sizeof(buf), &len) == 0); - CHECK(Fail3, len == sizeof(first_data)); - CHECK(Fail3, memcmp(buf, first_data, sizeof(first_data)) == 0); - - snprintf(path, sizeof(path), "%s/together/A/1/1/0/c/0/0", tmpdir); - CHECK(Fail3, read_file(path, buf, sizeof(buf), &len) == 0); - CHECK(Fail3, len == sizeof(second_data)); - CHECK(Fail3, memcmp(buf, second_data, sizeof(second_data)) == 0); + for (int i = 0; i < nopens; ++i) { + snprintf(path, + sizeof(path), + "%s/together/A/1/%d/%d/c/0/%d", + tmpdir, + opens[i].field, + opens[i].level, + opens[i].shard); + CHECK(Fail3, read_file(path, buf, sizeof(buf), &len) == 0); + CHECK(Fail3, len == sizeof(data[i])); + CHECK(Fail3, memcmp(buf, data[i], sizeof(data[i])) == 0); + } store_destroy(store); log_info(" PASS"); From b0c2263a8e69f129d1c6abffe811e9ac1e97fc25 Mon Sep 17 00:00:00 2001 From: Nathan Clack Date: Tue, 25 Aug 2026 16:56:59 +0000 Subject: [PATCH 3/4] Fold the slot count into one helper --- src/hcs/hcs.c | 40 ++++++++++------------- src/ngff/ngff_multiscale.c | 67 +++++++++++++++----------------------- src/zarr/shard_pool.h | 4 +++ tests/test_hcs.c | 28 ++++++++-------- 4 files changed, 64 insertions(+), 75 deletions(-) diff --git a/src/hcs/hcs.c b/src/hcs/hcs.c index 4181fe3a..6c2d9151 100644 --- a/src/hcs/hcs.c +++ b/src/hcs/hcs.c @@ -26,7 +26,7 @@ struct hcs_plate // fovs[row * cols * field_count + col * field_count + fov] struct ngff_multiscale** fovs; - int nfovs; // total allocated FOV slots + uint64_t nfovs; // total allocated FOV slots // Plate-level custom attrs. struct attr_set plate_attrs; @@ -46,10 +46,10 @@ row_char(const struct hcs_plate_config* cfg, int r) return cfg->row_names ? cfg->row_names[r] : (char)('A' + r); } -static int +static uint64_t fov_index(const struct hcs_plate* p, int row, int col, int fov) { - return (row * p->cols + col) * p->field_count + fov; + return ((uint64_t)row * p->cols + col) * p->field_count + fov; } static int @@ -166,13 +166,11 @@ hcs_plate_create(struct store* store, const struct hcs_plate_config* cfg) CHECK(Fail, rn_len < 64); // bounded by hcs_plate.row_names buffer } - // One pool keeps the whole plate on one io queue, so the pool has to be wide - // enough for every field of view to have its own slots. + // One pool per plate keeps the whole plate on one io queue. uint64_t slots_per_fov = ngff_multiscale_slot_count(&cfg->fov); CHECK(Fail, slots_per_fov > 0); - uint64_t fov_count = - (uint64_t)cfg->rows * (uint64_t)cfg->cols * (uint64_t)cfg->field_count; + uint64_t fov_count = (uint64_t)cfg->rows * cfg->cols * cfg->field_count; struct shard_pool* pool = store->create_pool(store, fov_count * slots_per_fov); CHECK(Fail, pool); @@ -205,7 +203,7 @@ hcs_plate_create(struct store* store, const struct hcs_plate_config* cfg) for (int i = 0; i < cfg->rows * cfg->cols; ++i) attr_set_init(&p->well_attrs[i]); - p->nfovs = cfg->rows * cfg->cols * cfg->field_count; + p->nfovs = fov_count; p->fovs = (struct ngff_multiscale**)calloc((size_t)p->nfovs, sizeof(void*)); CHECK(Fail_well_attrs, p->fovs); @@ -249,17 +247,15 @@ hcs_plate_create(struct store* store, const struct hcs_plate_config* cfg) // FOV multiscale sinks for (int f = 0; f < cfg->field_count; ++f) { struct strbuf fov_prefix = { 0 }; - int fp_rc = - strbuf_appendf(&fov_prefix, "%s/%c/%d/%d", cfg->name, rc, c + 1, f); - int idx = fov_index(p, r, c, f); - p->fovs[idx] = - fp_rc == 0 - ? ngff_multiscale_create_with_pool(store, - pool, - (uint64_t)idx * slots_per_fov, - strbuf_cstr(&fov_prefix), - &cfg->fov) - : NULL; + uint64_t idx = fov_index(p, r, c, f); + if (strbuf_appendf( + &fov_prefix, "%s/%c/%d/%d", cfg->name, rc, c + 1, f) == 0) + p->fovs[idx] = + ngff_multiscale_create_with_pool(store, + pool, + idx * slots_per_fov, + strbuf_cstr(&fov_prefix), + &cfg->fov); strbuf_free(&fov_prefix); CHECK(Fail_fovs, p->fovs[idx]); } @@ -269,7 +265,7 @@ hcs_plate_create(struct store* store, const struct hcs_plate_config* cfg) return p; Fail_fovs: - for (int i = 0; i < p->nfovs; ++i) { + for (uint64_t i = 0; i < p->nfovs; ++i) { if (p->fovs[i]) ngff_multiscale_destroy(p->fovs[i]); } @@ -304,7 +300,7 @@ hcs_plate_destroy(struct hcs_plate* p) write_well_group(p, r, c); // FOV multiscales flush their own metadata in their destroy. - for (int i = 0; i < p->nfovs; ++i) { + for (uint64_t i = 0; i < p->nfovs; ++i) { if (p->fovs[i]) ngff_multiscale_destroy(p->fovs[i]); } @@ -329,7 +325,7 @@ hcs_plate_fov_sink(struct hcs_plate* p, int row, int col, int fov) CHECK_SILENT(Bad, fov >= 0 && fov < p->field_count); CHECK_SILENT(Bad, well_active(p, row, col)); - int idx = fov_index(p, row, col, fov); + uint64_t idx = fov_index(p, row, col, fov); return ngff_multiscale_as_shard_sink(p->fovs[idx]); Bad: return NULL; diff --git a/src/ngff/ngff_multiscale.c b/src/ngff/ngff_multiscale.c index b0c164fc..0d1f58a7 100644 --- a/src/ngff/ngff_multiscale.c +++ b/src/ngff/ngff_multiscale.c @@ -195,6 +195,14 @@ ngff_multiscale_flush_fn(struct shard_sink* self) // --- Shared create logic --- +static int +plan_from_config(struct lod_plan* plan, + const struct ngff_multiscale_config* cfg) +{ + int max_lev = cfg->nlod > 0 ? cfg->nlod : LOD_MAX_LEVELS; + return lod_plan_init_from_dims(plan, cfg->dimensions, cfg->rank, max_lev, 0); +} + static void level_dimensions(struct dimension* out, const struct ngff_multiscale_config* cfg, @@ -211,20 +219,14 @@ level_dimensions(struct dimension* out, } } -// Slot ranges are spaced by this count, so it has to walk the levels the same -// way the create loop below does. +// Both the count that spaces slot ranges and the create loop below reach a +// level's slots through here, so the two cannot disagree. static uint64_t -plan_slot_count(const struct ngff_multiscale_config* cfg, - const struct lod_plan* plan) +dims_slot_count(const struct dimension* dims, uint8_t rank) { - uint64_t total = 0; - for (int lv = 0; lv < plan->levels.nlod; ++lv) { - struct dimension lv_dims[MAX_ZARR_RANK]; - uint64_t sc[MAX_ZARR_RANK], cps[MAX_ZARR_RANK]; - level_dimensions(lv_dims, cfg, plan, lv); - total += dims_compute_shard_geometry(lv_dims, cfg->rank, sc, cps); - } - return total; + uint64_t shard_counts[MAX_ZARR_RANK], chunks_per_shard[MAX_ZARR_RANK]; + return dims_compute_shard_geometry( + dims, rank, shard_counts, chunks_per_shard); } // Shared init: caller provides a pre-computed LOD plan. @@ -299,8 +301,7 @@ ngff_multiscale_init(struct store* store, strbuf_free(&level_prefix); CHECK(Fail_levels, ms->levels[lv]); - uint64_t sc[MAX_ZARR_RANK], cps[MAX_ZARR_RANK]; - slot_base += dims_compute_shard_geometry(lv_dims, cfg->rank, sc, cps); + slot_base += dims_slot_count(lv_dims, cfg->rank); } CHECK(Fail_levels, write_ngff_group_metadata(ms) == 0); @@ -333,12 +334,14 @@ ngff_multiscale_slot_count(const struct ngff_multiscale_config* cfg) CHECK(Fail, cfg->dimensions); struct lod_plan plan = { 0 }; - int max_lev = cfg->nlod > 0 ? cfg->nlod : LOD_MAX_LEVELS; - CHECK(Fail, - lod_plan_init_from_dims( - &plan, cfg->dimensions, cfg->rank, max_lev, 0) == 0); + CHECK(Fail, plan_from_config(&plan, cfg) == 0); - uint64_t total = plan_slot_count(cfg, &plan); + uint64_t total = 0; + for (int lv = 0; lv < plan.levels.nlod; ++lv) { + struct dimension lv_dims[MAX_ZARR_RANK]; + level_dimensions(lv_dims, cfg, &plan, lv); + total += dims_slot_count(lv_dims, cfg->rank); + } lod_plan_free(&plan); return total; @@ -360,10 +363,7 @@ ngff_multiscale_create_with_pool(struct store* store, CHECK(Fail, cfg->dimensions); struct lod_plan plan = { 0 }; - int max_lev = cfg->nlod > 0 ? cfg->nlod : LOD_MAX_LEVELS; - CHECK(Fail, - lod_plan_init_from_dims( - &plan, cfg->dimensions, cfg->rank, max_lev, 0) == 0); + CHECK(Fail, plan_from_config(&plan, cfg) == 0); return ngff_multiscale_init(store, pool, slot_base, prefix, cfg, &plan); @@ -379,25 +379,15 @@ ngff_multiscale_create(struct store* store, const struct ngff_multiscale_config* cfg) { CHECK(Fail, store); - CHECK(Fail, cfg); - CHECK(Fail, cfg->rank > 0 && cfg->rank <= MAX_ZARR_RANK); - CHECK(Fail, cfg->dimensions); - struct lod_plan plan = { 0 }; - int max_lev = cfg->nlod > 0 ? cfg->nlod : LOD_MAX_LEVELS; - CHECK(Fail, - lod_plan_init_from_dims( - &plan, cfg->dimensions, cfg->rank, max_lev, 0) == 0); - - uint64_t total_slots = plan_slot_count(cfg, &plan); - CHECK(Fail_plan, total_slots > 0); + uint64_t total_slots = ngff_multiscale_slot_count(cfg); + CHECK(Fail, total_slots > 0); struct shard_pool* pool = store->create_pool(store, total_slots); - CHECK(Fail_plan, pool); + CHECK(Fail, pool); - // plan ownership transfers to ngff_multiscale_init struct ngff_multiscale* ms = - ngff_multiscale_init(store, pool, 0, prefix, cfg, &plan); + ngff_multiscale_create_with_pool(store, pool, 0, prefix, cfg); if (!ms) { shard_pool_destroy(pool); return NULL; @@ -405,9 +395,6 @@ ngff_multiscale_create(struct store* store, ms->owns_pool = 1; return ms; -Fail_plan: - lod_plan_free(&plan); - Fail: return NULL; } diff --git a/src/zarr/shard_pool.h b/src/zarr/shard_pool.h index 7b269920..bee57653 100644 --- a/src/zarr/shard_pool.h +++ b/src/zarr/shard_pool.h @@ -13,6 +13,10 @@ struct shard_pool { // Open writer slot for shard data at the given key. // If the slot has a pending finalize, waits for it first. + // Reusing a slot is how a caller moves to its next shard, so the pool cannot + // tell that from two callers picking the same number. Callers sharing a pool + // have to keep to slot ranges that do not overlap, or they take over each + // other's open files. struct shard_writer* (*open)(struct shard_pool* self, uint64_t slot, const char* key); diff --git a/tests/test_hcs.c b/tests/test_hcs.c index 055b0fec..cb6d229a 100644 --- a/tests/test_hcs.c +++ b/tests/test_hcs.c @@ -358,27 +358,28 @@ test_hcs_two_fields_open_together(void) { 0, 0, 0 }, { 0, 0, 1 }, { 0, 1, 0 }, { 1, 0, 0 }, { 1, 0, 1 }, { 1, 1, 0 }, }; - const int nopens = (int)(sizeof(opens) / sizeof(opens[0])); + struct shard_writer* writers[countof(opens)] = { 0 }; + uint8_t payload[256]; - struct shard_writer* writers[sizeof(opens) / sizeof(opens[0])] = { 0 }; - uint8_t data[sizeof(opens) / sizeof(opens[0])][256]; - - for (int i = 0; i < nopens; ++i) { + for (uint64_t i = 0; i < countof(opens); ++i) { struct shard_sink* sink = hcs_plate_fov_sink(plate, 0, 0, opens[i].field); CHECK(Fail3, sink); writers[i] = sink->open(sink, (uint8_t)opens[i].level, (uint64_t)opens[i].shard); CHECK(Fail3, writers[i]); - for (int j = 0; j < i; ++j) + for (uint64_t j = 0; j < i; ++j) CHECK(Fail3, writers[i] != writers[j]); - memset(data[i], (uint8_t)(0x10 + i), sizeof(data[i])); } - for (int i = 0; i < nopens; ++i) + // Every writer is open before any write, so a shared slot shows up as one + // field's bytes landing in another's file. + for (uint64_t i = 0; i < countof(opens); ++i) { + memset(payload, (uint8_t)(0x10 + i), sizeof(payload)); CHECK(Fail3, writers[i]->write( - writers[i], 0, data[i], data[i] + sizeof(data[i])) == 0); - for (int i = 0; i < nopens; ++i) + writers[i], 0, payload, payload + sizeof(payload)) == 0); + } + for (uint64_t i = 0; i < countof(opens); ++i) CHECK(Fail3, writers[i]->finalize(writers[i]) == 0); // Destroying the plate drains the writes, so the files are complete below. @@ -388,7 +389,7 @@ test_hcs_two_fields_open_together(void) char path[4096], buf[8192]; size_t len; - for (int i = 0; i < nopens; ++i) { + for (uint64_t i = 0; i < countof(opens); ++i) { snprintf(path, sizeof(path), "%s/together/A/1/%d/%d/c/0/%d", @@ -396,9 +397,10 @@ test_hcs_two_fields_open_together(void) opens[i].field, opens[i].level, opens[i].shard); + memset(payload, (uint8_t)(0x10 + i), sizeof(payload)); CHECK(Fail3, read_file(path, buf, sizeof(buf), &len) == 0); - CHECK(Fail3, len == sizeof(data[i])); - CHECK(Fail3, memcmp(buf, data[i], sizeof(data[i])) == 0); + CHECK(Fail3, len == sizeof(payload)); + CHECK(Fail3, memcmp(buf, payload, sizeof(payload)) == 0); } store_destroy(store); From ce9602e22b6524719caade8a2f743f165a69fa6c Mon Sep 17 00:00:00 2001 From: Nathan Clack Date: Tue, 25 Aug 2026 16:59:17 +0000 Subject: [PATCH 4/4] Trim the new comments --- src/hcs/hcs.c | 2 +- src/ngff/ngff_multiscale.c | 3 +-- src/ngff/ngff_multiscale.h | 4 +--- src/zarr/shard_pool.h | 7 +++---- tests/test_hcs.c | 9 ++++----- 5 files changed, 10 insertions(+), 15 deletions(-) diff --git a/src/hcs/hcs.c b/src/hcs/hcs.c index 6c2d9151..045e4620 100644 --- a/src/hcs/hcs.c +++ b/src/hcs/hcs.c @@ -166,7 +166,7 @@ hcs_plate_create(struct store* store, const struct hcs_plate_config* cfg) CHECK(Fail, rn_len < 64); // bounded by hcs_plate.row_names buffer } - // One pool per plate keeps the whole plate on one io queue. + // One pool per plate keeps its fields of view on one io queue. uint64_t slots_per_fov = ngff_multiscale_slot_count(&cfg->fov); CHECK(Fail, slots_per_fov > 0); diff --git a/src/ngff/ngff_multiscale.c b/src/ngff/ngff_multiscale.c index 0d1f58a7..81354bb4 100644 --- a/src/ngff/ngff_multiscale.c +++ b/src/ngff/ngff_multiscale.c @@ -219,8 +219,7 @@ level_dimensions(struct dimension* out, } } -// Both the count that spaces slot ranges and the create loop below reach a -// level's slots through here, so the two cannot disagree. +// Spacing a slot range and advancing it must use one count. static uint64_t dims_slot_count(const struct dimension* dims, uint8_t rank) { diff --git a/src/ngff/ngff_multiscale.h b/src/ngff/ngff_multiscale.h index 0f85958f..aaeed5a9 100644 --- a/src/ngff/ngff_multiscale.h +++ b/src/ngff/ngff_multiscale.h @@ -6,9 +6,7 @@ #include "zarr/shard_pool.h" #include "zarr/store.h" -// Count the pool slots one multiscale needs across its levels. Several -// multiscales sharing a pool space their slot ranges by this much, because two -// multiscales on one slot overwrite each other's shard files. Returns 0 when +// Count the pool slots one multiscale needs across its levels. Returns 0 when // the config cannot be used. uint64_t ngff_multiscale_slot_count(const struct ngff_multiscale_config* cfg); diff --git a/src/zarr/shard_pool.h b/src/zarr/shard_pool.h index bee57653..5201961b 100644 --- a/src/zarr/shard_pool.h +++ b/src/zarr/shard_pool.h @@ -13,10 +13,9 @@ struct shard_pool { // Open writer slot for shard data at the given key. // If the slot has a pending finalize, waits for it first. - // Reusing a slot is how a caller moves to its next shard, so the pool cannot - // tell that from two callers picking the same number. Callers sharing a pool - // have to keep to slot ranges that do not overlap, or they take over each - // other's open files. + // Callers sharing a pool must use slot ranges that do not overlap. Slot + // reuse is how a caller reaches its next shard, so the pool cannot tell it + // from an overlap. struct shard_writer* (*open)(struct shard_pool* self, uint64_t slot, const char* key); diff --git a/tests/test_hcs.c b/tests/test_hcs.c index cb6d229a..3655cf30 100644 --- a/tests/test_hcs.c +++ b/tests/test_hcs.c @@ -317,8 +317,8 @@ test_hcs_two_fields_open_together(void) CHECK(Fail, store); store->mkdirs(store, "."); - // A field spans several slots here, two shards at level 0 and one at level 1, - // so a wrong stride between fields lands on another field's slot. + // A field spans several slots here, so a wrong stride between fields + // collides. struct dimension dims[] = { { .size = 32, .chunk_size = 16, @@ -371,8 +371,7 @@ test_hcs_two_fields_open_together(void) CHECK(Fail3, writers[i] != writers[j]); } - // Every writer is open before any write, so a shared slot shows up as one - // field's bytes landing in another's file. + // Every writer is open before any write, so a shared slot misroutes bytes. for (uint64_t i = 0; i < countof(opens); ++i) { memset(payload, (uint8_t)(0x10 + i), sizeof(payload)); CHECK(Fail3, @@ -382,7 +381,7 @@ test_hcs_two_fields_open_together(void) for (uint64_t i = 0; i < countof(opens); ++i) CHECK(Fail3, writers[i]->finalize(writers[i]) == 0); - // Destroying the plate drains the writes, so the files are complete below. + // Destroying the plate drains the writes, so the files are complete. hcs_plate_destroy(plate); plate = NULL;