From c77a987859e4dde46ee0a7317d2eadd94bbc5ee7 Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Mon, 8 Jul 2024 12:00:17 -0400 Subject: [PATCH 1/6] 238, part 2: Misc fixes (#262) - ~~Use the x64-osx triplet for build and release jobs on Mac (see error [here](https://github.com/acquire-project/acquire-driver-zarr/actions/runs/9765062597/job/26958778874))~~ lol never mind, here's an [issue](https://github.com/acquire-project/acquire-driver-zarr/issues/263) to address in a separate PR - Link nlohmann_json::nlohmann_json in examples - Fix array size bug in downsampling --- examples/CMakeLists.txt | 1 + examples/no-striping.cpp | 2 +- src/zarr.cpp | 18 +++++++++++------- 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/examples/CMakeLists.txt b/examples/CMakeLists.txt index 6846852a..8f33e332 100644 --- a/examples/CMakeLists.txt +++ b/examples/CMakeLists.txt @@ -23,6 +23,7 @@ if (${WITH_EXAMPLES}) acquire-core-logger acquire-core-platform acquire-video-runtime + nlohmann_json::nlohmann_json ) endforeach () diff --git a/examples/no-striping.cpp b/examples/no-striping.cpp index f59e1da2..09008c3f 100644 --- a/examples/no-striping.cpp +++ b/examples/no-striping.cpp @@ -12,7 +12,7 @@ #include #include -#include "tests/json.hpp" +#include namespace fs = std::filesystem; using json = nlohmann::json; diff --git a/src/zarr.cpp b/src/zarr.cpp index 6cc0264e..b9483eaa 100644 --- a/src/zarr.cpp +++ b/src/zarr.cpp @@ -141,8 +141,9 @@ scale_image(const VideoFrame* src) const auto height = src->shape.dims.height; const auto h_pad = height + (height % downscale); - auto* dst = (VideoFrame*)malloc(sizeof(VideoFrame) + - w_pad * h_pad * factor * sizeof(T)); + const size_t bytes_of_frame = common::align_up( + sizeof(VideoFrame) + w_pad * h_pad * factor * bytes_of_type, 8); + auto* dst = (VideoFrame*)malloc(bytes_of_frame); memcpy(dst, src, sizeof(VideoFrame)); dst->shape.dims.width = w_pad / downscale; @@ -152,12 +153,11 @@ scale_image(const VideoFrame* src) dst->shape.strides.planes = dst->shape.strides.height * dst->shape.dims.height; - dst->bytes_of_frame = - common::align_up(bytes_of_image(&dst->shape) + sizeof(*dst), 8); + dst->bytes_of_frame = bytes_of_frame; const auto* src_img = (T*)src->data; auto* dst_img = (T*)dst->data; - memset(dst_img, 0, dst->bytes_of_frame - sizeof(*dst)); + memset(dst_img, 0, bytes_of_image(&dst->shape)); size_t dst_idx = 0; for (auto row = 0; row < height; row += downscale) { @@ -690,8 +690,12 @@ template void test_average_frame_inner(const SampleType& stype) { - auto* src = (VideoFrame*)malloc(sizeof(VideoFrame) + 9 * sizeof(T)); - src->bytes_of_frame = common::align_up(sizeof(*src) + 9 * sizeof(T), 8); + const size_t bytes_of_frame = + common::align_up(sizeof(VideoFrame) + 9 * sizeof(T), 8); + auto* src = (VideoFrame*)malloc(bytes_of_frame); + CHECK(src); + + src->bytes_of_frame = bytes_of_frame; src->shape = { .dims = { .channels = 1, From 037d6eca8ff43eef328d66bffe3e677b40c79061 Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Tue, 9 Jul 2024 14:58:36 -0400 Subject: [PATCH 2/6] Add a separate macros-only header. --- src/common/macros.hh | 20 ++++++++++++++++++++ src/common/utilities.hh | 18 +----------------- 2 files changed, 21 insertions(+), 17 deletions(-) create mode 100644 src/common/macros.hh diff --git a/src/common/macros.hh b/src/common/macros.hh new file mode 100644 index 00000000..b4a5c263 --- /dev/null +++ b/src/common/macros.hh @@ -0,0 +1,20 @@ +#pragma once + +#include "logger.h" +#include + +#define LOG(...) aq_logger(0, __FILE__, __LINE__, __FUNCTION__, __VA_ARGS__) +#define LOGE(...) aq_logger(1, __FILE__, __LINE__, __FUNCTION__, __VA_ARGS__) +#define EXPECT(e, ...) \ + do { \ + if (!(e)) { \ + LOGE(__VA_ARGS__); \ + throw std::runtime_error("Expression was false: " #e); \ + } \ + } while (0) +#define CHECK(e) EXPECT(e, "Expression evaluated as false:\n\t%s", #e) + +#define TRACE(...) + +#define containerof(ptr, T, V) ((T*)(((char*)(ptr)) - offsetof(T, V))) +#define countof(e) (sizeof(e) / sizeof(*(e))) \ No newline at end of file diff --git a/src/common/utilities.hh b/src/common/utilities.hh index 5ce35e37..e2241c89 100644 --- a/src/common/utilities.hh +++ b/src/common/utilities.hh @@ -4,6 +4,7 @@ #include "logger.h" #include "device/props/components.h" #include "device/props/storage.h" +#include "macros.hh" #include "dimension.hh" #include @@ -15,23 +16,6 @@ #include #include -#define LOG(...) aq_logger(0, __FILE__, __LINE__, __FUNCTION__, __VA_ARGS__) -#define LOGE(...) aq_logger(1, __FILE__, __LINE__, __FUNCTION__, __VA_ARGS__) -#define EXPECT(e, ...) \ - do { \ - if (!(e)) { \ - LOGE(__VA_ARGS__); \ - throw std::runtime_error("Expression was false: " #e); \ - } \ - } while (0) -#define CHECK(e) EXPECT(e, "Expression evaluated as false:\n\t%s", #e) - -// #define TRACE(...) LOG(__VA_ARGS__) -#define TRACE(...) - -#define containerof(ptr, T, V) ((T*)(((char*)(ptr)) - offsetof(T, V))) -#define countof(e) (sizeof(e) / sizeof(*(e))) - namespace fs = std::filesystem; namespace acquire::sink::zarr { From 463fe6ed58d6aa7bfd87bd305506de76d15b6dca Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Tue, 9 Jul 2024 15:01:10 -0400 Subject: [PATCH 3/6] Update reported datatypes to conform with specs (#268) - [Zarr V2](https://zarr-specs.readthedocs.io/en/latest/v2/v2.0.html#data-type-encoding) - [Zarr V3](https://zarr-specs.readthedocs.io/en/latest/v3/core/v3.0.html#core-data-types) zarr-python had been reading our datatypes without complaining. Testing by @chris-delg has revealed that TensorStore will not. ## Changes - Link against nlohman::json in examples builds (orthogonal to the issue, but a light lift). - Remove `sample_type_to_dtype()` from common.{h,cpp} - Implement version-specific `sample_type_to_dtype()` - Update tests to check data type is correct. --- src/common.cpp | 16 ---------- src/common.hh | 7 ----- src/zarr.v2.cpp | 31 ++++++++++++++++++- src/zarr.v3.cpp | 26 +++++++++++++++- tests/multiscales-metadata.cpp | 4 +++ tests/repeat-start.cpp | 4 ++- tests/write-zarr-v2-compressed-multiscale.cpp | 4 +++ ...-compressed-with-chunking-and-rollover.cpp | 4 +++ ...write-zarr-v2-compressed-with-chunking.cpp | 4 +++ ...-raw-chunk-size-larger-than-frame-size.cpp | 4 +++ ...-raw-multiscale-with-trivial-tile-size.cpp | 4 +++ tests/write-zarr-v2-raw-multiscale.cpp | 4 +++ ...v2-raw-with-even-chunking-and-rollover.cpp | 4 +++ .../write-zarr-v2-raw-with-even-chunking.cpp | 4 +++ ...write-zarr-v2-raw-with-ragged-chunking.cpp | 4 +++ tests/write-zarr-v2-raw.cpp | 5 +++ tests/write-zarr-v2-with-lz4-compression.cpp | 4 +++ tests/write-zarr-v2-with-zstd-compression.cpp | 4 +++ tests/write-zarr-v3-compressed.cpp | 2 +- .../write-zarr-v3-raw-chunk-exceeds-array.cpp | 2 +- ...write-zarr-v3-raw-with-ragged-sharding.cpp | 2 +- tests/write-zarr-v3-raw.cpp | 2 +- 22 files changed, 115 insertions(+), 30 deletions(-) diff --git a/src/common.cpp b/src/common.cpp index d79ebd30..597f8764 100644 --- a/src/common.cpp +++ b/src/common.cpp @@ -1,7 +1,4 @@ #include "common.hh" -#include "zarr.hh" - -#include "platform.h" #include #include @@ -199,19 +196,6 @@ common::bytes_per_chunk(const std::vector& dimensions, return n_bytes; } -const char* -common::sample_type_to_dtype(SampleType t) - -{ - static const char* table[] = { "u1", "u2", "i1", "i2", - "f4", "u2", "u2", "u2" }; - if (t < countof(table)) { - return table[t]; - } else { - throw std::runtime_error("Invalid sample type."); - } -} - const char* common::sample_type_to_string(SampleType t) noexcept { diff --git a/src/common.hh b/src/common.hh index 2e10efe2..b1afe285 100644 --- a/src/common.hh +++ b/src/common.hh @@ -131,13 +131,6 @@ size_t bytes_per_chunk(const std::vector& dimensions, const SampleType& dtype); -/// @brief Get the Zarr dtype for a given SampleType. -/// @param t An enumerated sample type. -/// @throw std::runtime_error if @par t is not a valid SampleType. -/// @return A representation of the SampleType @par t expected by a Zarr reader. -const char* -sample_type_to_dtype(SampleType t); - /// @brief Get a string representation of the SampleType enum. /// @param t An enumerated sample type. /// @return A human-readable representation of the SampleType @par t. diff --git a/src/zarr.v2.cpp b/src/zarr.v2.cpp index f877c628..175ab9be 100644 --- a/src/zarr.v2.cpp +++ b/src/zarr.v2.cpp @@ -4,6 +4,8 @@ #include "nlohmann/json.hpp" +#include + namespace zarr = acquire::sink::zarr; namespace { @@ -22,6 +24,33 @@ compressed_zarr_v2_init() } return nullptr; } + +std::string +sample_type_to_dtype(SampleType t) + +{ + const std::string dtype_prefix = + std::endian::native == std::endian::big ? ">" : "<"; + + switch (t) { + case SampleType_u8: + return dtype_prefix + "u1"; + case SampleType_u10: + case SampleType_u12: + case SampleType_u14: + case SampleType_u16: + return dtype_prefix + "u2"; + case SampleType_i8: + return dtype_prefix + "i1"; + case SampleType_i16: + return dtype_prefix + "i2"; + case SampleType_f32: + return dtype_prefix + "f4"; + default: + throw std::runtime_error("Invalid SampleType: " + + std::to_string(static_cast(t))); + } +} } // end ::{anonymous} namespace /// ZarrV2 @@ -249,7 +278,7 @@ zarr::ZarrV2::write_array_metadata_(size_t level) const metadata["zarr_format"] = 2; metadata["shape"] = array_shape; metadata["chunks"] = chunk_shape; - metadata["dtype"] = common::sample_type_to_dtype(image_shape.type); + metadata["dtype"] = sample_type_to_dtype(image_shape.type); metadata["fill_value"] = 0; metadata["order"] = "C"; metadata["filters"] = nullptr; diff --git a/src/zarr.v3.cpp b/src/zarr.v3.cpp index 311753e5..840a2c35 100644 --- a/src/zarr.v3.cpp +++ b/src/zarr.v3.cpp @@ -24,6 +24,30 @@ compressed_zarr_v3_init() } return nullptr; } + +std::string +sample_type_to_dtype(SampleType t) + +{ + switch (t) { + case SampleType_u8: + return "uint8"; + case SampleType_u10: + case SampleType_u12: + case SampleType_u14: + case SampleType_u16: + return "uint16"; + case SampleType_i8: + return "int8"; + case SampleType_i16: + return "int16"; + case SampleType_f32: + return "float32"; + default: + throw std::runtime_error("Invalid SampleType: " + + std::to_string(static_cast(t))); + } +} } // end ::{anonymous} namespace zarr::ZarrV3::ZarrV3(BloscCompressionParams&& compression_params) @@ -175,7 +199,7 @@ zarr::ZarrV3::write_array_metadata_(size_t level) const }); metadata["chunk_memory_layout"] = "C"; - metadata["data_type"] = common::sample_type_to_dtype(image_shape.type); + metadata["data_type"] = sample_type_to_dtype(image_shape.type); metadata["extensions"] = json::array(); metadata["fill_value"] = 0; metadata["shape"] = array_shape; diff --git a/tests/multiscales-metadata.cpp b/tests/multiscales-metadata.cpp index b1fb4cc4..44c17b4a 100644 --- a/tests/multiscales-metadata.cpp +++ b/tests/multiscales-metadata.cpp @@ -182,6 +182,10 @@ verify_layer(const LayerTestCase& test_case) std::ifstream f(zarray_path); json zarray = json::parse(f); + const std::string dtype = + std::endian::native == std::endian::little ? "u1"; + CHECK(dtype == zarray["dtype"].get()); + const auto shape = zarray["shape"]; ASSERT_EQ(int, "%d", frames_per_layer, shape[0]); ASSERT_EQ(int, "%d", layer_frame_height, shape[1]); diff --git a/tests/repeat-start.cpp b/tests/repeat-start.cpp index cb9c2a49..e65eea9e 100644 --- a/tests/repeat-start.cpp +++ b/tests/repeat-start.cpp @@ -187,7 +187,9 @@ validate(AcquireRuntime* runtime) ASSERT_EQ(int, "%d", 32, chunk_shape[3]); CHECK("C" == metadata["chunk_memory_layout"]); - CHECK("u1" == metadata["data_type"]); + + CHECK("uint8" == metadata["data_type"].get()); + CHECK(metadata["extensions"].empty()); const auto array_shape = metadata["shape"]; diff --git a/tests/write-zarr-v2-compressed-multiscale.cpp b/tests/write-zarr-v2-compressed-multiscale.cpp index 870ebd03..1e4ed823 100644 --- a/tests/write-zarr-v2-compressed-multiscale.cpp +++ b/tests/write-zarr-v2-compressed-multiscale.cpp @@ -202,6 +202,10 @@ verify_layer(const LayerTestCase& test_case) std::ifstream f(zarray_path); json zarray = json::parse(f); + const std::string dtype = + std::endian::native == std::endian::little ? "u1"; + CHECK(dtype == zarray["dtype"].get()); + const auto shape = zarray["shape"]; ASSERT_EQ(int, "%d", frames_per_layer, shape[0]); ASSERT_EQ(int, "%d", 1, shape[1]); diff --git a/tests/write-zarr-v2-compressed-with-chunking-and-rollover.cpp b/tests/write-zarr-v2-compressed-with-chunking-and-rollover.cpp index ecbe9f71..411b1eb0 100644 --- a/tests/write-zarr-v2-compressed-with-chunking-and-rollover.cpp +++ b/tests/write-zarr-v2-compressed-with-chunking-and-rollover.cpp @@ -174,6 +174,10 @@ validate() std::ifstream f(zarray_path); json zarray = json::parse(f); + const std::string dtype = + std::endian::native == std::endian::little ? "u1"; + CHECK(dtype == zarray["dtype"].get()); + auto shape = zarray["shape"]; ASSERT_EQ(int, "%d", chunk_planes + 1, shape[0]); ASSERT_EQ(int, "%d", 1, shape[1]); diff --git a/tests/write-zarr-v2-compressed-with-chunking.cpp b/tests/write-zarr-v2-compressed-with-chunking.cpp index c393422f..8944de71 100644 --- a/tests/write-zarr-v2-compressed-with-chunking.cpp +++ b/tests/write-zarr-v2-compressed-with-chunking.cpp @@ -171,6 +171,10 @@ validate() std::ifstream f(zarray_path); json zarray = json::parse(f); + const std::string dtype = + std::endian::native == std::endian::little ? "u1"; + CHECK(dtype == zarray["dtype"].get()); + auto shape = zarray["shape"]; ASSERT_EQ(int, "%d", chunk_planes, shape[0]); ASSERT_EQ(int, "%d", 1, shape[1]); diff --git a/tests/write-zarr-v2-raw-chunk-size-larger-than-frame-size.cpp b/tests/write-zarr-v2-raw-chunk-size-larger-than-frame-size.cpp index 6f576b71..0e732a49 100644 --- a/tests/write-zarr-v2-raw-chunk-size-larger-than-frame-size.cpp +++ b/tests/write-zarr-v2-raw-chunk-size-larger-than-frame-size.cpp @@ -233,6 +233,10 @@ validate() std::ifstream f(zarray_path); json zarray = json::parse(f); + const std::string dtype = + std::endian::native == std::endian::little ? "u1"; + CHECK(dtype == zarray["dtype"].get()); + auto shape = zarray["shape"]; ASSERT_EQ(int, "%d", frames_per_chunk, shape[0]); ASSERT_EQ(int, "%d", 1, shape[1]); diff --git a/tests/write-zarr-v2-raw-multiscale-with-trivial-tile-size.cpp b/tests/write-zarr-v2-raw-multiscale-with-trivial-tile-size.cpp index d638719d..6a1b6c1f 100644 --- a/tests/write-zarr-v2-raw-multiscale-with-trivial-tile-size.cpp +++ b/tests/write-zarr-v2-raw-multiscale-with-trivial-tile-size.cpp @@ -185,6 +185,10 @@ verify_layer(const LayerTestCase& test_case) std::ifstream f(zarray_path); json zarray = json::parse(f); + const std::string dtype = + std::endian::native == std::endian::little ? "u1"; + CHECK(dtype == zarray["dtype"].get()); + const auto shape = zarray["shape"]; ASSERT_EQ(int, "%d", frames_per_layer, shape[0]); ASSERT_EQ(int, "%d", 1, shape[1]); diff --git a/tests/write-zarr-v2-raw-multiscale.cpp b/tests/write-zarr-v2-raw-multiscale.cpp index 1108ee88..88a8e5b8 100644 --- a/tests/write-zarr-v2-raw-multiscale.cpp +++ b/tests/write-zarr-v2-raw-multiscale.cpp @@ -192,6 +192,10 @@ verify_layer(const LayerTestCase& test_case) std::ifstream f(zarray_path); json zarray = json::parse(f); + const std::string dtype = + std::endian::native == std::endian::little ? "u1"; + CHECK(dtype == zarray["dtype"].get()); + const auto shape = zarray["shape"]; ASSERT_EQ(int, "%d", frames_per_layer, shape[0]); ASSERT_EQ(int, "%d", 1, shape[1]); diff --git a/tests/write-zarr-v2-raw-with-even-chunking-and-rollover.cpp b/tests/write-zarr-v2-raw-with-even-chunking-and-rollover.cpp index 7f0e33d8..18379436 100644 --- a/tests/write-zarr-v2-raw-with-even-chunking-and-rollover.cpp +++ b/tests/write-zarr-v2-raw-with-even-chunking-and-rollover.cpp @@ -162,6 +162,10 @@ validate() std::ifstream f(zarray_path); json zarray = json::parse(f); + const std::string dtype = + std::endian::native == std::endian::little ? "u1"; + CHECK(dtype == zarray["dtype"].get()); + auto shape = zarray["shape"]; ASSERT_EQ(int, "%d", chunk_planes + 1, shape[0]); ASSERT_EQ(int, "%d", 1, shape[1]); diff --git a/tests/write-zarr-v2-raw-with-even-chunking.cpp b/tests/write-zarr-v2-raw-with-even-chunking.cpp index 8d9ddfb9..58fbdb9b 100644 --- a/tests/write-zarr-v2-raw-with-even-chunking.cpp +++ b/tests/write-zarr-v2-raw-with-even-chunking.cpp @@ -161,6 +161,10 @@ validate() std::ifstream f(zarray_path); json zarray = json::parse(f); + const std::string dtype = + std::endian::native == std::endian::little ? "u1"; + CHECK(dtype == zarray["dtype"].get()); + auto shape = zarray["shape"]; ASSERT_EQ(int, "%d", chunk_planes, shape[0]); ASSERT_EQ(int, "%d", 1, shape[1]); diff --git a/tests/write-zarr-v2-raw-with-ragged-chunking.cpp b/tests/write-zarr-v2-raw-with-ragged-chunking.cpp index c7232038..971eb66e 100644 --- a/tests/write-zarr-v2-raw-with-ragged-chunking.cpp +++ b/tests/write-zarr-v2-raw-with-ragged-chunking.cpp @@ -237,6 +237,10 @@ validate() std::ifstream f(zarray_path); json zarray = json::parse(f); + const std::string dtype = + std::endian::native == std::endian::little ? "u1"; + CHECK(dtype == zarray["dtype"].get()); + auto shape = zarray["shape"]; ASSERT_EQ(int, "%d", max_frame_count, shape[0]); ASSERT_EQ(int, "%d", 1, shape[1]); diff --git a/tests/write-zarr-v2-raw.cpp b/tests/write-zarr-v2-raw.cpp index 835f2590..d213ecad 100644 --- a/tests/write-zarr-v2-raw.cpp +++ b/tests/write-zarr-v2-raw.cpp @@ -3,6 +3,7 @@ #include "platform.h" // clock #include "logger.h" +#include #include #include #include @@ -233,6 +234,10 @@ validate() std::ifstream f(zarray_path); json zarray = json::parse(f); + const std::string dtype = + std::endian::native == std::endian::little ? "u1"; + CHECK(dtype == zarray["dtype"].get()); + auto shape = zarray["shape"]; ASSERT_EQ(int, "%d", frames_per_chunk, shape[0]); ASSERT_EQ(int, "%d", 1, shape[1]); diff --git a/tests/write-zarr-v2-with-lz4-compression.cpp b/tests/write-zarr-v2-with-lz4-compression.cpp index d87526c8..686d0419 100644 --- a/tests/write-zarr-v2-with-lz4-compression.cpp +++ b/tests/write-zarr-v2-with-lz4-compression.cpp @@ -169,6 +169,10 @@ validate() std::ifstream f(zarray_path); json zarray = json::parse(f); + const std::string dtype = + std::endian::native == std::endian::little ? "u1"; + CHECK(dtype == zarray["dtype"].get()); + auto shape = zarray["shape"]; ASSERT_EQ(int, "%d", frames_per_chunk, shape[0]); ASSERT_EQ(int, "%d", 1, shape[1]); diff --git a/tests/write-zarr-v2-with-zstd-compression.cpp b/tests/write-zarr-v2-with-zstd-compression.cpp index 1cd3a013..5430d558 100644 --- a/tests/write-zarr-v2-with-zstd-compression.cpp +++ b/tests/write-zarr-v2-with-zstd-compression.cpp @@ -169,6 +169,10 @@ validate() std::ifstream f(zarray_path); json zarray = json::parse(f); + const std::string dtype = + std::endian::native == std::endian::little ? "u1"; + CHECK(dtype == zarray["dtype"].get()); + auto shape = zarray["shape"]; ASSERT_EQ(int, "%d", frames_per_chunk, shape[0]); ASSERT_EQ(int, "%d", 1, shape[1]); diff --git a/tests/write-zarr-v3-compressed.cpp b/tests/write-zarr-v3-compressed.cpp index 391e639d..734e2521 100644 --- a/tests/write-zarr-v3-compressed.cpp +++ b/tests/write-zarr-v3-compressed.cpp @@ -270,7 +270,7 @@ validate() ASSERT_EQ(int, "%d", chunk_width, chunk_shape[3]); CHECK("C" == metadata["chunk_memory_layout"]); - CHECK("u1" == metadata["data_type"]); + CHECK("uint8" == metadata["data_type"]); CHECK(metadata["extensions"].empty()); const auto array_shape = metadata["shape"]; diff --git a/tests/write-zarr-v3-raw-chunk-exceeds-array.cpp b/tests/write-zarr-v3-raw-chunk-exceeds-array.cpp index bd9a223d..cfc3d7b0 100644 --- a/tests/write-zarr-v3-raw-chunk-exceeds-array.cpp +++ b/tests/write-zarr-v3-raw-chunk-exceeds-array.cpp @@ -261,7 +261,7 @@ validate() ASSERT_EQ(int, "%d", chunk_width, chunk_shape[2]); CHECK("C" == metadata["chunk_memory_layout"]); - CHECK("u1" == metadata["data_type"]); + CHECK("uint8" == metadata["data_type"]); CHECK(metadata["extensions"].empty()); const auto array_shape = metadata["shape"]; diff --git a/tests/write-zarr-v3-raw-with-ragged-sharding.cpp b/tests/write-zarr-v3-raw-with-ragged-sharding.cpp index 92632fa7..b3ccd51a 100644 --- a/tests/write-zarr-v3-raw-with-ragged-sharding.cpp +++ b/tests/write-zarr-v3-raw-with-ragged-sharding.cpp @@ -260,7 +260,7 @@ validate() ASSERT_EQ(int, "%d", chunk_width, chunk_shape[2]); CHECK("C" == metadata["chunk_memory_layout"]); - CHECK("u1" == metadata["data_type"]); + CHECK("uint8" == metadata["data_type"]); CHECK(metadata["extensions"].empty()); const auto array_shape = metadata["shape"]; diff --git a/tests/write-zarr-v3-raw.cpp b/tests/write-zarr-v3-raw.cpp index 5c4df05c..64d84197 100644 --- a/tests/write-zarr-v3-raw.cpp +++ b/tests/write-zarr-v3-raw.cpp @@ -270,7 +270,7 @@ validate() ASSERT_EQ(int, "%d", chunk_width, chunk_shape[3]); CHECK("C" == metadata["chunk_memory_layout"]); - CHECK("u1" == metadata["data_type"]); + CHECK("uint8" == metadata["data_type"]); CHECK(metadata["extensions"].empty()); const auto array_shape = metadata["shape"]; From fbc109482c7fb93f1b638a578f97f1a1abd39943 Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Tue, 9 Jul 2024 15:06:14 -0400 Subject: [PATCH 4/6] Trim whitespace from dimension names. --- src/common/dimension.cpp | 59 ++++++++++++++++++++++++++++++++++++---- src/common/dimension.hh | 6 ++-- tests/unit-tests.cpp | 1 + 3 files changed, 57 insertions(+), 9 deletions(-) diff --git a/src/common/dimension.cpp b/src/common/dimension.cpp index 4e3154cb..e724acdd 100644 --- a/src/common/dimension.cpp +++ b/src/common/dimension.cpp @@ -1,19 +1,37 @@ #include "dimension.hh" -#include -#include -#include "utilities.hh" -#include "zarr.hh" -#include "platform.h" +#include namespace zarr = acquire::sink::zarr; +namespace { +inline std::string +trim(const std::string& s) +{ + // trim left + std::string trimmed = s; + trimmed.erase(trimmed.begin(), + std::find_if(trimmed.begin(), trimmed.end(), [](char c) { + return !std::isspace(c); + })); + + // trim right + trimmed.erase(std::find_if(trimmed.rbegin(), + trimmed.rend(), + [](char c) { return !std::isspace(c); }) + .base(), + trimmed.end()); + + return trimmed; +} +} // namespace + zarr::Dimension::Dimension(const std::string& name, DimensionType kind, uint32_t array_size_px, uint32_t chunk_size_px, uint32_t shard_size_chunks) - : name{ name } + : name{ trim(name) } , kind{ kind } , array_size_px{ array_size_px } , chunk_size_px{ chunk_size_px } @@ -22,6 +40,7 @@ zarr::Dimension::Dimension(const std::string& name, EXPECT(kind < DimensionTypeCount, "Invalid dimension type."); EXPECT(!name.empty(), "Dimension name cannot be empty."); } + zarr::Dimension::Dimension(const StorageDimension& dim) : Dimension(dim.name.str, dim.kind, @@ -30,3 +49,31 @@ zarr::Dimension::Dimension(const StorageDimension& dim) dim.shard_size_chunks) { } + +#ifndef NO_UNIT_TESTS +#ifdef _WIN32 +#define acquire_export __declspec(dllexport) +#else +#define acquire_export +#endif // _WIN32 + +extern "C" acquire_export int +unit_test__trim() +{ + try { + EXPECT(trim(" foo") == "foo", "Failed to trim left."); + EXPECT(trim("foo ") == "foo", "Failed to trim right."); + EXPECT(trim(" foo ") == "foo", "Failed to trim both."); + EXPECT(trim("foo") == "foo", "Failed to trim either."); + EXPECT(trim("").empty(), "Failed to trim empty."); + return 1; + } catch (const std::exception& e) { + LOGE("Exception: %s", e.what()); + } catch (...) { + LOGE("Unknown exception"); + } + + return 0; +} + +#endif // NO_UNIT_TESTS diff --git a/src/common/dimension.hh b/src/common/dimension.hh index 646cd19f..7cf14188 100644 --- a/src/common/dimension.hh +++ b/src/common/dimension.hh @@ -1,6 +1,6 @@ -#ifndef H_ACQUIRE_STORAGE_ZARR_COMMON_DIMENSION_V0 -#define H_ACQUIRE_STORAGE_ZARR_COMMON_DIMENSION_V0 +#pragma once +#include "macros.hh" #include "device/props/storage.h" #include @@ -14,6 +14,7 @@ struct Dimension unsigned int array_size_px, unsigned int chunk_size_px, unsigned int shard_size_chunks); + explicit Dimension(const StorageDimension& dim); const std::string name; @@ -23,4 +24,3 @@ struct Dimension const unsigned int shard_size_chunks; }; } // namespace acquire::sink::zarr -#endif // H_ACQUIRE_STORAGE_ZARR_COMMON_DIMENSION_V0 diff --git a/tests/unit-tests.cpp b/tests/unit-tests.cpp index 11310e26..84a30d15 100644 --- a/tests/unit-tests.cpp +++ b/tests/unit-tests.cpp @@ -82,6 +82,7 @@ main() }; const std::vector tests{ #define CASE(e) { .name = #e, .test = (int (*)())lib_load(&lib, #e) } + CASE(unit_test__trim), CASE(unit_test__average_frame), CASE(unit_test__thread_pool__push_to_job_queue), CASE(unit_test__sink_creator__create_chunk_file_sinks), From eef0db7d073fb982a24218d331cb044b55916865 Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Tue, 9 Jul 2024 15:21:49 -0400 Subject: [PATCH 5/6] Use a `switch` statement in `common::sample_type_to_string()`. --- src/common/utilities.cpp | 32 +++++++++++++------------------- src/common/utilities.hh | 7 ------- 2 files changed, 13 insertions(+), 26 deletions(-) diff --git a/src/common/utilities.cpp b/src/common/utilities.cpp index af4bf976..90d8e70d 100644 --- a/src/common/utilities.cpp +++ b/src/common/utilities.cpp @@ -166,28 +166,22 @@ common::bytes_per_chunk(const std::vector& dimensions, return n_bytes; } -const char* -common::sample_type_to_dtype(SampleType t) - -{ - static const char* table[] = { "u1", "u2", "i1", "i2", - "f4", "u2", "u2", "u2" }; - if (t < countof(table)) { - return table[t]; - } else { - throw std::runtime_error("Invalid sample type."); - } -} - const char* common::sample_type_to_string(SampleType t) noexcept { - static const char* table[] = { "u8", "u16", "i8", "i16", - "f32", "u16", "u16", "u16" }; - if (t < countof(table)) { - return table[t]; - } else { - return "unrecognized pixel type"; + switch (t) { + case SampleType_u8: + return "u8"; + case SampleType_u16: + return "u16"; + case SampleType_i8: + return "i8"; + case SampleType_i16: + return "i16"; + case SampleType_f32: + return "f32"; + default: + return "unrecognized pixel type"; } } diff --git a/src/common/utilities.hh b/src/common/utilities.hh index e2241c89..1290c53e 100644 --- a/src/common/utilities.hh +++ b/src/common/utilities.hh @@ -80,13 +80,6 @@ size_t bytes_per_chunk(const std::vector& dimensions, const SampleType& dtype); -/// @brief Get the Zarr dtype for a given SampleType. -/// @param t An enumerated sample type. -/// @throw std::runtime_error if @par t is not a valid SampleType. -/// @return A representation of the SampleType @par t expected by a Zarr reader. -const char* -sample_type_to_dtype(SampleType t); - /// @brief Get a string representation of the SampleType enum. /// @param t An enumerated sample type. /// @return A human-readable representation of the SampleType @par t. From 1377ed413818f92848c459b66b751db2960ef8bd Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Tue, 9 Jul 2024 16:04:23 -0400 Subject: [PATCH 6/6] Respond to PR comments. --- src/common/thread.pool.cpp | 16 ++++++++-------- src/common/thread.pool.hh | 2 +- src/common/utilities.cpp | 6 +----- 3 files changed, 10 insertions(+), 14 deletions(-) diff --git a/src/common/thread.pool.cpp b/src/common/thread.pool.cpp index 79a57eb5..534b0849 100644 --- a/src/common/thread.pool.cpp +++ b/src/common/thread.pool.cpp @@ -4,15 +4,14 @@ namespace zarr = acquire::sink::zarr; namespace common = zarr::common; -common::ThreadPool::ThreadPool(size_t n_threads, +common::ThreadPool::ThreadPool(unsigned int n_threads, std::function err) : error_handler_{ err } , is_accepting_jobs_{ true } { + const unsigned int one = 1; n_threads = std::clamp( - n_threads, - (size_t)1, - (size_t)std::max(std::thread::hardware_concurrency(), (unsigned)1)); + n_threads, one, std::max(std::thread::hardware_concurrency(), one)); for (auto i = 0; i < n_threads; ++i) { threads_.emplace_back([this] { thread_worker_(); }); @@ -32,11 +31,12 @@ common::ThreadPool::~ThreadPool() noexcept void common::ThreadPool::push_to_job_queue(JobT&& job) { - std::unique_lock lock(jobs_mutex_); - CHECK(is_accepting_jobs_); + { + std::unique_lock lock(jobs_mutex_); + CHECK(is_accepting_jobs_); - jobs_.push(std::move(job)); - lock.unlock(); + jobs_.push(std::move(job)); + } cv_.notify_one(); } diff --git a/src/common/thread.pool.hh b/src/common/thread.pool.hh index ac29ae9d..a5e24c01 100644 --- a/src/common/thread.pool.hh +++ b/src/common/thread.pool.hh @@ -20,7 +20,7 @@ struct ThreadPool final // std::string& argument to the error handler is a diagnostic message from // the failing job and is logged to the error stream by the Zarr driver when // the next call to `append()` is made. - ThreadPool(size_t n_threads, std::function err); + ThreadPool(unsigned int n_threads, std::function err); ~ThreadPool() noexcept; void push_to_job_queue(JobT&& job); diff --git a/src/common/utilities.cpp b/src/common/utilities.cpp index 90d8e70d..63b98f94 100644 --- a/src/common/utilities.cpp +++ b/src/common/utilities.cpp @@ -201,15 +201,11 @@ common::split_uri(const std::string& uri) auto end = uri.find_first_of(delim); std::vector out; - while (end <= std::string::npos) { + while (end != std::string::npos) { if (end > begin) { out.emplace_back(uri.substr(begin, end - begin)); } - if (end == std::string::npos) { - break; - } - begin = end + 1; end = uri.find_first_of(delim, begin); }