From 9298169dcb14048ed5f3839e095ffd8cfbc12fe8 Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Mon, 13 Nov 2023 14:12:50 -0500 Subject: [PATCH] Update the chunking API to address some shortcomings and support sharding (#41) # Description The chunking parameters have been updated in the `StorageProperties` struct, allowing users to specify the number of actual planes per chunk, rather than setting the max bytes per chunk. Also, sharding properties have been added. ## Related issue(s) - See also: - https://github.com/acquire-project/acquire-driver-zarr/pull/137 ## Type of change - [ ] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [X] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] Documentation - [ ] Other (please describe): # How has this been tested? Added a unit test to check sharding property setter convenience function works as expected. Updated the chunking property setter function. # Checklist - [X] I have performed a self-review of my own code. - [X] I have commented my code, particularly in hard-to-understand areas. - [X] I have updated the documentation, if necessary. - [X] I added tests for my changes, and/or the testing strategy is described above. - [X] I updated the [changelog](../CHANGELOG.md). --- CHANGELOG.md | 11 +++ .../device/props/storage.c | 73 +++++++++++++----- .../device/props/storage.h | 77 +++++++++++-------- tests/unit-tests.cpp | 2 + 4 files changed, 112 insertions(+), 51 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c6af67f..a23faf2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- Users can specify the full chunk size in width, height, and planes. + +### Removed + +- `max_bytes_per_chunk` from `StorageProperties` chunking properties. + ### Fixed - Removes 30-second timeout from `thread_join` on Windows. @@ -17,6 +25,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `acquire-device-properties`: A corresponding entry in `StoragePropertyMetadata`. - `acquire-device-properties`: A convenience function for enabling multiscale, similar to the function for setting chunking properties. +- `acquire-device-properties`: A `struct storage_properties_sharding_s` member `shard_dims_chunks` of + `StorageProperties`. Users can now configure sharding properties where supported. +- `acquire-device-properties`: A convenience function for setting sharding parameters. ## [0.1.3](https://github.com/acquire-project/acquire-core-libs/compare/v0.1.2...v0.1.3) - 2023-06-27 diff --git a/src/acquire-device-properties/device/props/storage.c b/src/acquire-device-properties/device/props/storage.c index 8de9026..e0d091d 100644 --- a/src/acquire-device-properties/device/props/storage.c +++ b/src/acquire-device-properties/device/props/storage.c @@ -76,18 +76,29 @@ storage_properties_set_external_metadata(struct StorageProperties* out, int storage_properties_set_chunking_props(struct StorageProperties* out, - uint32_t tile_width, - uint32_t tile_height, - uint32_t tile_planes, - uint64_t max_bytes_per_chunk) + uint32_t chunk_width, + uint32_t chunk_height, + uint32_t chunk_planes) { CHECK(out); - out->chunking.tile.width = tile_width; - out->chunking.tile.height = tile_height; - out->chunking.tile.planes = tile_planes; - // Use 16 MB default chunk size if 0 is passed in. - out->chunking.max_bytes_per_chunk = - max_bytes_per_chunk ? max_bytes_per_chunk : (1ULL << 24); + out->chunk_dims_px.width = chunk_width; + out->chunk_dims_px.height = chunk_height; + out->chunk_dims_px.planes = chunk_planes; + return 1; +Error: + return 0; +} + +int +storage_properties_set_sharding_props(struct StorageProperties* out, + uint32_t shard_width, + uint32_t shard_height, + uint32_t shard_planes) +{ + CHECK(out); + out->shard_dims_chunks.width = shard_width; + out->shard_dims_chunks.height = shard_height; + out->shard_dims_chunks.planes = shard_planes; return 1; Error: return 0; @@ -353,20 +364,40 @@ int unit_test__storage_properties_set_chunking_props() { struct StorageProperties props = { 0 }; - CHECK(0 == props.chunking.tile.width); - CHECK(0 == props.chunking.tile.height); - CHECK(0 == props.chunking.tile.planes); - CHECK(0 == props.chunking.max_bytes_per_chunk); + CHECK(0 == props.chunk_dims_px.width); + CHECK(0 == props.chunk_dims_px.height); + CHECK(0 == props.chunk_dims_px.planes); - const uint32_t tile_width = 1, tile_height = 2, tile_planes = 3; + const uint32_t chunk_width = 1, chunk_height = 2, chunk_planes = 3; CHECK(storage_properties_set_chunking_props( - &props, tile_width, tile_height, tile_planes, 0)); + &props, chunk_width, chunk_height, chunk_planes)); + + CHECK(chunk_width == props.chunk_dims_px.width); + CHECK(chunk_height == props.chunk_dims_px.height); + CHECK(chunk_planes == props.chunk_dims_px.planes); + + storage_properties_destroy(&props); + + return 1; +Error: + return 0; +} + +int +unit_test__storage_properties_set_sharding_props() +{ + struct StorageProperties props = { 0 }; + CHECK(0 == props.shard_dims_chunks.width); + CHECK(0 == props.shard_dims_chunks.height); + CHECK(0 == props.shard_dims_chunks.planes); + + const uint32_t shard_width = 1, shard_height = 2, shard_planes = 3; + CHECK(storage_properties_set_sharding_props( + &props, shard_width, shard_height, shard_planes)); - CHECK(tile_width == props.chunking.tile.width); - CHECK(tile_height == props.chunking.tile.height); - CHECK(tile_planes == props.chunking.tile.planes); - // This is the default value if you set bytes_per_chunk to 0 - CHECK((1ULL << 24) == props.chunking.max_bytes_per_chunk); + CHECK(shard_width == props.shard_dims_chunks.width); + CHECK(shard_height == props.shard_dims_chunks.height); + CHECK(shard_planes == props.shard_dims_chunks.planes); storage_properties_destroy(&props); diff --git a/src/acquire-device-properties/device/props/storage.h b/src/acquire-device-properties/device/props/storage.h index 55d0821..709d978 100644 --- a/src/acquire-device-properties/device/props/storage.h +++ b/src/acquire-device-properties/device/props/storage.h @@ -19,18 +19,17 @@ extern "C" uint32_t first_frame_id; struct PixelScale pixel_scale_um; - /// Parameters for chunking. - /// Tile dimensions, width, height, and planes, are in pixels and - /// determine how to break up frames. Together with - /// `max_bytes_per_chunk`, they determine the dimensions of a chunk. + /// Dimensions of chunks, in pixels. struct storage_properties_chunking_s { - uint64_t max_bytes_per_chunk; - struct storage_properties_chunking_tile_s - { - uint32_t width, height, planes; - } tile; - } chunking; + uint32_t width, height, planes; + } chunk_dims_px; + + /// Dimensions of shards, in chunks. + struct storage_properties_sharding_s + { + uint32_t width, height, planes; + } shard_dims_chunks; /// Enable multiscale storage if true. uint8_t enable_multiscale; @@ -39,21 +38,26 @@ extern "C" struct StoragePropertyMetadata { /// Metadata for chunking. - /// Indicates whether chunking is supported, and if so, bounds on the - /// dimensions of tiles and the maximum number of bytes per chunk. + /// Indicates whether chunking is supported, and if so, bounds on what + /// the dimensions (in px) of the chunks are. struct storage_property_metadata_chunking_s { - uint8_t supported; - struct Property max_bytes_per_chunk; - struct storage_property_metadata_chunk_dim_s - { - struct Property width, height, planes; - } tile; - } chunking; + uint8_t is_supported; + struct Property width, height, planes; + } chunk_dims_px; + + /// Metadata for sharding. + /// Indicates whether sharding is supported, and if so, bounds on what + /// the dimensions (in chunks) of the shards are. + struct storage_property_metadata_sharding_s + { + uint8_t is_supported; + struct Property width, height, planes; + } shard_dims_chunks; struct storage_property_metadata_multiscale_s { - uint8_t supported; + uint8_t is_supported; } multiscale; }; @@ -111,19 +115,32 @@ extern "C" size_t bytes_of_metadata); /// @brief Set chunking properties for `out`. - /// Convenience function to set tiling/chunking properties in a single call. + /// Convenience function to set chunking properties in a single call. /// @returns 1 on success, otherwise 0 /// @param[in, out] out The storage properties to change. - /// @param[in] tile_width The width, in px, of a tile. - /// @param[in] tile_height The height, in px, of a tile. - /// @param[in] tile_planes The number of @p tile_width x @p tile_height - /// planes in a single tile. - /// @param[in] max_bytes_per_chunk The maximum size, in bytes, of a chunk. + /// @param[in] chunk_width The width, in px, of a chunk. + /// @param[in] chunk_height The height, in px, of a chunk. + /// @param[in] chunk_planes The number of @p chunk_width x @p chunk_height + /// planes in a single chunk. int storage_properties_set_chunking_props(struct StorageProperties* out, - uint32_t tile_width, - uint32_t tile_height, - uint32_t tile_planes, - uint64_t max_bytes_per_chunk); + uint32_t chunk_width, + uint32_t chunk_height, + uint32_t chunk_planes); + + /// @brief Set sharding properties for `out`. + /// Convenience function to set sharding properties in a single call. + /// @returns 1 on success, otherwise 0 + /// @param[in, out] out The storage properties to change. + /// @param[in] shard_width The number of chunks in a shard along the x + /// dimension. + /// @param[in] shard_height The number of chunks in a shard along the y + /// dimension. + /// @param[in] shard_planes The number of chunks in a shard along the append + /// dimension. + int storage_properties_set_sharding_props(struct StorageProperties* out, + uint32_t shard_width, + uint32_t shard_height, + uint32_t shard_planes); /// @brief Set multiscale properties for `out`. /// Convenience function to enable multiscale. diff --git a/tests/unit-tests.cpp b/tests/unit-tests.cpp index 120dcdf..3db7a26 100644 --- a/tests/unit-tests.cpp +++ b/tests/unit-tests.cpp @@ -68,6 +68,7 @@ extern "C" int unit_test__storage__storage_property_string_check(); int unit_test__storage__copy_string(); int unit_test__storage_properties_set_chunking_props(); + int unit_test__storage_properties_set_sharding_props(); int unit_test__device_state_as_string__is_defined_for_all(); int unit_test__device_kind_as_string__is_defined_for_all(); int unit_test__sample_type_as_string__is_defined_for_all(); @@ -93,6 +94,7 @@ main() CASE(unit_test__storage__storage_property_string_check), CASE(unit_test__storage__copy_string), CASE(unit_test__storage_properties_set_chunking_props), + CASE(unit_test__storage_properties_set_sharding_props), CASE(unit_test__device_state_as_string__is_defined_for_all), CASE(unit_test__device_kind_as_string__is_defined_for_all), CASE(unit_test__sample_type_as_string__is_defined_for_all),