Flesh out storage properties dimensions init and destroy (#38)

* Make storage_properties_destroy void. Make sure to destroy all the dimensions in the storage array.

* Copy acquisition dimensions in storage_properties_copy.

* Document storage_properties_dimensions_s::init. Return int from this function rather than void.

* Protect `max` definition with an #ifndef guard in acquire.c.

* Initialize StorageDimension array in storage_properties_init().

* Fix segfault in storage_properties_dimensions_destroy unit test.

* Update acquire-video-runtime/src/acquire.c

Co-authored-by: Nathan Clack <[email protected]>

* Simplify the interface around initializing StorageDimension structs.

* Respond to PR comments

---------

Co-authored-by: Nathan Clack <[email protected]>

acquire-core-libs/src/acquire-device-properties/device/props/storage.h

@@ -57,12 +57,6 @@ extern "C"
 
             // The number of dimensions in the output array.
             size_t size;
-
-            // Allocate storage for the dimensions.
-            void (*init)(struct StorageDimension**, size_t);
-
-            // Free storage for the dimensions.
-            void (*destroy)(struct StorageDimension*);
         } acquisition_dimensions;
 
         /// Enable multiscale storage if true.
@@ -91,13 +85,17 @@ extern "C"
     /// @param[in] bytes_of_metadata Number of bytes in the `metadata` buffer
     ///                              including the terminating null.
     /// @param[in] pixel_scale_um The pixel scale or size in microns.
+    /// @param[in] dimension_count The number of dimensions in the storage
+    /// array. Each of the @p dimension_count dimensions will be initialized
+    /// to zero.
     int storage_properties_init(struct StorageProperties* out,
                                 uint32_t first_frame_id,
                                 const char* filename,
                                 size_t bytes_of_filename,
                                 const char* metadata,
                                 size_t bytes_of_metadata,
-                                struct PixelScale pixel_scale_um);
+                                struct PixelScale pixel_scale_um,
+                                uint8_t dimension_count);
 
     /// Copies contents, reallocating string storage if necessary.
     /// @returns 1 on success, otherwise 0
@@ -129,19 +127,11 @@ extern "C"
                                                  const char* metadata,
                                                  size_t bytes_of_metadata);
 
-    /// @brief Set multiscale properties for `out`.
-    /// Convenience function to enable multiscale.
-    /// @returns 1 on success, otherwise 0
-    /// @param[in, out] out The storage properties to change.
-    /// @param[in] enable A flag to enable or disable multiscale.
-    int storage_properties_set_enable_multiscale(struct StorageProperties* out,
-                                                 uint8_t enable);
-
-    /// Free's allocated string storage.
-    void storage_properties_destroy(struct StorageProperties* self);
-
-    /// @brief Initialize the Dimension struct in `out`.
-    /// @param[out] out The Dimension struct to initialize.
+    /// @brief Set the value of the StorageDimension struct at index `index` in
+    /// `out`.
+    /// @param[out] out The StorageProperties struct containing the
+    /// StorageDimension array.
+    /// @param[in] index The index of the dimension to set.
     /// @param[in] name The name of the dimension.
     /// @param[in] bytes_of_name The number of bytes in the name buffer.
     ///                          Should include the terminating NULL.
@@ -151,38 +141,25 @@ extern "C"
     /// @param[in] shard_size_chunks The number of chunks in a shard along this
     ///                              dimension.
     /// @returns 1 on success, otherwise 0
-    int storage_dimension_init(struct StorageDimension* out,
-                               const char* name,
-                               size_t bytes_of_name,
-                               enum DimensionType kind,
-                               uint32_t array_size_px,
-                               uint32_t chunk_size_px,
-                               uint32_t shard_size_chunks);
-
-    /// @brief Copy the Dimension struct in `src` to `dst`.
-    /// @param[out] dst The Dimension struct to copy to.
-    /// @param[in] src The Dimension struct to copy from.
-    /// @returns 1 on success, otherwise 0
-    int storage_dimension_copy(struct StorageDimension* dst,
-                               const struct StorageDimension* src);
-
-    /// @brief Destroy the Dimension struct in `self`.
-    /// @param[out] self The Dimension struct to destroy.
-    void storage_dimension_destroy(struct StorageDimension* self);
+    int storage_properties_set_dimension(struct StorageProperties* out,
+                                         int index,
+                                         const char* name,
+                                         size_t bytes_of_name,
+                                         enum DimensionType kind,
+                                         uint32_t array_size_px,
+                                         uint32_t chunk_size_px,
+                                         uint32_t shard_size_chunks);
 
-    /// @brief Initialize the acquisition_dimensions array in `self`.
-    /// @param[out] self The StorageProperties struct containing the array to
-    ///                  initialize.
-    /// @param[in] size The number of dimensions to allocate.
+    /// @brief Set multiscale properties for `out`.
+    /// Convenience function to enable multiscale.
     /// @returns 1 on success, otherwise 0
-    int storage_properties_dimensions_init(struct StorageProperties* self,
-                                           size_t size);
+    /// @param[in, out] out The storage properties to change.
+    /// @param[in] enable A flag to enable or disable multiscale.
+    int storage_properties_set_enable_multiscale(struct StorageProperties* out,
+                                                 uint8_t enable);
 
-    /// @brief Free the acquisition_dimensions array in `self`.
-    /// @param[out] self The StorageProperties struct containing the array to
-    ///                  destroy.
-    /// @returns 1 on success, otherwise 0
-    int storage_properties_dimensions_destroy(struct StorageProperties* self);
+    /// Free allocated string storage.
+    void storage_properties_destroy(struct StorageProperties* self);
 
 #ifdef __cplusplus
 }

acquire-driver-common/src/storage/raw.c

@@ -132,7 +132,8 @@ raw_init()
                                   sizeof("out.raw"),
                                   0,
                                   0,
-                                  pixel_scale_um));
+                                  pixel_scale_um,
+                                  0));
     self->writer =
       (struct Storage){ .state = DeviceState_AwaitingConfiguration,
                         .set = raw_set,

acquire-video-runtime/src/acquire.c

@@ -12,6 +12,7 @@
 #include <stdio.h>
 #include <string.h>
 
+#undef max
 #define max(a, b) ((a) < (b) ? (b) : (a))
 
 #define containerof(ptr, T, V) ((T*)(((char*)(ptr)) - offsetof(T, V)))
@@ -262,7 +263,8 @@ configure_video_stream(struct video_s* const video,
 
     int is_ok = 1;
     if (pcamera->identifier.kind == DeviceKind_None) {
-        is_ok &= (Device_Ok == device_manager_select_default(
+        is_ok &= (Device_Ok ==
+                  device_manager_select_default(
                     device_manager, DeviceKind_Camera, &pcamera->identifier));
     }
 
@@ -277,8 +279,9 @@ configure_video_stream(struct video_s* const video,
                                      pvideo->frame_average_count) == Device_Ok);
 
     if (pstorage->identifier.kind == DeviceKind_None) {
-        is_ok &= (Device_Ok == device_manager_select_default(
-                                 device_manager, DeviceKind_Storage, &pstorage->identifier));
+        is_ok &= (Device_Ok ==
+                  device_manager_select_default(
+                    device_manager, DeviceKind_Storage, &pstorage->identifier));
     }
     is_ok &= (video_sink_configure(&video->sink,
                                    device_manager,

acquire-video-runtime/tests/change-external-metadata.cpp

@@ -58,7 +58,8 @@ acquire(AcquireRuntime* runtime,
                             0,
                             external_metadata_json,
                             strlen(external_metadata_json) + 1,
-                            { 0, 0 });
+                            { 0, 0 },
+                            0);
 
     OK(acquire_configure(runtime, props));
     OK(acquire_start(runtime));
@@ -105,6 +106,8 @@ main()
     acquire(runtime, &props, R"({"hurley": "burley"})");
     acquire(runtime, &props, R"({})");
 
+    storage_properties_destroy(&props.video[0].storage.settings);
+
     LOG("DONE (OK)");
     acquire_shutdown(runtime);
     return 0;

acquire-video-runtime/tests/change-file-name.cpp

@@ -91,14 +91,16 @@ main()
 
     const char filename[] = "";
     storage_properties_init(
-      &props.video[0].storage.settings, 0, SIZED(filename), 0, 0, { 1, 1 });
+      &props.video[0].storage.settings, 0, SIZED(filename), 0, 0, { 1, 1 }, 0);
 
     acquire(runtime, &props, "out1.tif");
     acquire(runtime, &props, "quite a bit longer.tif");
     acquire(runtime, &props, "s.tif");
     acquire(runtime, &props, "quite a bit longer.tif"); // overwrite?
 
     LOG("DONE (OK)");
+    storage_properties_destroy(&props.video[0].storage.settings);
+
     acquire_shutdown(runtime);
     return 0;
 }

acquire-video-runtime/tests/one-video-stream.cpp

@@ -68,7 +68,7 @@ main()
                                 &props.video[0].storage.identifier));
 
     storage_properties_init(
-      &props.video[0].storage.settings, 0, SIZED("out.tif"), 0, 0, { 0 });
+      &props.video[0].storage.settings, 0, SIZED("out.tif"), 0, 0, { 0 }, 0);
 
     OK(acquire_configure(runtime, &props));
 
@@ -137,6 +137,7 @@ main()
 
         CHECK(nframes == props.video[0].max_frame_count);
     }
+    storage_properties_destroy(&props.video[0].storage.settings);
 
     OK(acquire_stop(runtime));
     OK(acquire_shutdown(runtime));

acquire-video-runtime/tests/two-video-streams.cpp

@@ -85,15 +85,17 @@ main()
                                   sizeof(filenames[0]),
                                   external_metadata,
                                   sizeof(external_metadata),
-                                  px_scale_um));
+                                  px_scale_um,
+                                  0));
 
     CHECK(storage_properties_init(&props.video[1].storage.settings,
                                   0,
                                   filenames[1],
                                   sizeof(filenames[1]),
                                   external_metadata,
                                   sizeof(external_metadata),
-                                  { .x = 0, .y = 0 }));
+                                  { .x = 0, .y = 0 },
+                                  0));
 
     props.video[0].camera.settings.binning = 1;
     props.video[0].camera.settings.pixel_type = SampleType_u8;
@@ -167,6 +169,9 @@ main()
     }
 
     OK(acquire_stop(runtime));
+    storage_properties_destroy(&props.video[0].storage.settings);
+    storage_properties_destroy(&props.video[1].storage.settings);
+
     acquire_shutdown(runtime);
     return 0;
 }